Re: [PATCH v3 3/5] perf synthetic-events: Ensure features are aligned

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Dec 18, 2024 at 05:29:01PM -0800, Ian Rogers wrote:
> On Wed, Dec 18, 2024 at 5:08 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Dec 16, 2024 at 03:24:57PM -0800, Ian Rogers wrote:
> > > Features like hostname have arbitrary size and break the assumed
> > > 8-byte alignment of perf events. Pad all feature events until 8-byte
> > > alignment is restored.
> >
> > But it seems write_hostname() and others use do_write_string() which
> > handles the padding already.
> 
> Well it does something :-) (I agree my description isn't 100%)
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.c?h=perf-tools-next#n188
> ```
> static int do_write_string(struct feat_fd *ff, const char *str)
> {
> u32 len, olen;
> int ret;
> 
> olen = strlen(str) + 1;
> len = PERF_ALIGN(olen, NAME_ALIGN);
> 
> /* write len, incl. \0 */
> ret = do_write(ff, &len, sizeof(len));
> if (ret < 0)
> return ret;
> 
> return write_padded(ff, str, olen, len);
> }
> ```
> but NAME_ALIGN is 64:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/header.h?h=perf-tools-next#n187
> whereas the data file is expecting things aligned to sizeof(u64) which
> is 8-bytes. We're potentially writing out 7 u64s worth of 0s for no
> alignment gain. It should be safe, given I don't see other use of this
> alignment value, to switch the string alignment to being sizeof(u64)
> but following this change we could also just get rid of the padding
> knowing it will be fixed in the caller.

Probably, but it seems simpler just to change NAME_ALIGN to 8. :)

Thanks,
Namhyung