RE: [PATCH v1 1/1] Extended events (platform-specific) support in perf

[Tomasz Fujak]

> -----Original Message-----
> From: linux-arm-kernel-bounces@xxxxxxxxxxxxxxxxxxx [mailto:linux-arm-
> kernel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Arnaldo Carvalho de
> Melo
> Sent: Friday, January 22, 2010 1:26 PM
> To: Tomasz Fujak
> Cc: jpihet@xxxxxxxxxx; peterz@xxxxxxxxxxxxx; p.osciak@xxxxxxxxxxx;
> jamie.iles@xxxxxxxxxxxx; will.deacon@xxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; mingo@xxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; m.szyprowski@xxxxxxxxxxx
> Subject: Re: [PATCH v1 1/1] Extended events (platform-specific) support
> in perf
> 
> Em Fri, Jan 22, 2010 at 01:08:59PM +0100, Tomasz Fujak escreveu:
> > Signed-off-by: Tomasz Fujak <t.fujak@xxxxxxxxxxx>
> > Reviewed-by: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> > Reviewed-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > Reviewed-by: Kyungmin Park <kuyngmin.park@xxxxxxxxxxx>
> 
> "Extended" seems vague, I think in this context "platform_" would be a
> better namespace specifier.

Right.

> > +#define	BYTES_PER_CHUNK 4096
> > +/* returns the number of lines read;
> > +   on fail the return is negative and no memory is allocated
> > +   otherwise the *buf contains a memory chunk of which first
> > +   *size bytes are used for file contents
> > + */
> > +static int read_file(int fd, char **buf, unsigned *size)
> > +{
> > +	unsigned bytes = BYTES_PER_CHUNK;
> > +	int lines = 0;
> > +	char *ptr = malloc(bytes);
> 
> malloc can fail... Also why is that you can't process the lines one by
> one instead of reading the whole (albeit small) file at once?

Right, no malloc check.
The purpose of not processing line-by-line is that then the collection needs
to be dynamic.

Since the file buffer gets dropped after it's processed, the overallocated
memory is returned to the system.
In case of the collection (here an array and a counter) it'd require tricks
not to overallocate, and still be O(#events).

But you're right I didn't do it right - in case the a line does not parse,
memory reserved for it is not freed. Fixing.

> > +
> > +		if (name && description) {
> > +			symbols[count].symbol = name;
> > +			symbols[count].alias = "";
> > +			++count;
> > +			/* description gets lost here */
> > +			free(description);
> > +		} else {
> > +			free(description);
> > +			free(name);
> > +		}
> 
> Having "free(description);" in both cases seems funny :-)

I wasn't so bold as to upgrade the perf with human-readable event
description.
Otherwise yes, moving it outside.

> 
> - Arnaldo
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/