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.

Rather than invent a new mechanism, reuse write_padded but pass an
empty buffer as what to write. As no alignment may be necessary,
update write_padded to be tolerant of 0 as a count and count_aligned
value.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/header.c           | 7 ++++---
 tools/perf/util/synthetic-events.c | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 03e43a9894d4..d10703090e83 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -168,11 +168,12 @@ int write_padded(struct feat_fd *ff, const void *bf,
 		 size_t count, size_t count_aligned)
 {
 	static const char zero_buf[NAME_ALIGN];
-	int err = do_write(ff, bf, count);
+	int err = count > 0 ? do_write(ff, bf, count) : 0;
 
-	if (!err)
+	if (!err && count_aligned > count) {
+		assert(count_aligned - count < sizeof(zero_buf));
 		err = do_write(ff, zero_buf, count_aligned - count);
-
+	}
 	return err;
 }
 
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index f8ac2ac2da45..ee6e9c2fb11b 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2401,6 +2401,8 @@ int perf_event__synthesize_features(const struct perf_tool *tool, struct perf_se
 			pr_debug("Error writing feature\n");
 			continue;
 		}
+		write_padded(&ff, /*bf=*/NULL, /*count=*/0,
+			     /*count_aligned=*/PERF_ALIGN(ff.offset, sizeof(u64)) - ff.offset);
 		/* ff.buf may have changed due to realloc in do_write() */
 		fe = ff.buf;
 		memset(fe, 0, sizeof(*fe));
-- 
2.47.1.613.gc27f4b7a9f-goog

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.

thanks,
Namhyung

> 
> Rather than invent a new mechanism, reuse write_padded but pass an
> empty buffer as what to write. As no alignment may be necessary,
> update write_padded to be tolerant of 0 as a count and count_aligned
> value.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/header.c           | 7 ++++---
>  tools/perf/util/synthetic-events.c | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 03e43a9894d4..d10703090e83 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -168,11 +168,12 @@ int write_padded(struct feat_fd *ff, const void *bf,
>  		 size_t count, size_t count_aligned)
>  {
>  	static const char zero_buf[NAME_ALIGN];
> -	int err = do_write(ff, bf, count);
> +	int err = count > 0 ? do_write(ff, bf, count) : 0;
>  
> -	if (!err)
> +	if (!err && count_aligned > count) {
> +		assert(count_aligned - count < sizeof(zero_buf));
>  		err = do_write(ff, zero_buf, count_aligned - count);
> -
> +	}
>  	return err;
>  }
>  
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index f8ac2ac2da45..ee6e9c2fb11b 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -2401,6 +2401,8 @@ int perf_event__synthesize_features(const struct perf_tool *tool, struct perf_se
>  			pr_debug("Error writing feature\n");
>  			continue;
>  		}
> +		write_padded(&ff, /*bf=*/NULL, /*count=*/0,
> +			     /*count_aligned=*/PERF_ALIGN(ff.offset, sizeof(u64)) - ff.offset);
>  		/* ff.buf may have changed due to realloc in do_write() */
>  		fe = ff.buf;
>  		memset(fe, 0, sizeof(*fe));
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
>

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.

Thanks,
Ian

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