Re: [PATCH fyi 1/1] perf trace beauty: Sync linux/vhost.h with the kernel sources

From: Michael S. Tsirkin
Date: Tue Apr 02 2024 - 17:16:15 EST

Next message: Kuppuswamy Sathyanarayanan: "Re: [PATCH v2 4/4] platform/x86: wmi: Add driver development guide"
Previous message: Thomas Weißschuh: "[PATCH v3] fs/proc/proc_sysctl.c: always initialize i_uid/i_gid"
In reply to: Namhyung Kim: "Re: [PATCH fyi 1/1] perf trace beauty: Sync linux/vhost.h with the kernel sources"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH fyi 1/1] perf trace beauty: Sync linux/vhost.h with the kernel sources"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Apr 02, 2024 at 02:04:39PM -0700, Namhyung Kim wrote:
> On Thu, Mar 21, 2024 at 1:41 PM Arnaldo Carvalho de Melo
> <acme@xxxxxxxxxx> wrote:
> >
> > tldr; Just FYI, I'm carrying this on the perf tools tree.
>
> Have you applied this to perf-tools-next?
>
> >
> > Full explanation:
> >
> > There used to be no copies, with tools/ code using kernel headers
> > directly. From time to time tools/perf/ broke due to legitimate kernel
> > hacking. At some point Linus complained about such direct usage. Then we
> > adopted the current model.
> >
> > The way these headers are used in perf are not restricted to just
> > including them to compile something.
> >
> > There are sometimes used in scripts that convert defines into string
> > tables, etc, so some change may break one of these scripts, or new MSRs
> > may use some different #define pattern, etc.
> >
> > E.g.:
> >
> > $ ls -1 tools/perf/trace/beauty/*.sh | head -5
> > tools/perf/trace/beauty/arch_errno_names.sh
> > tools/perf/trace/beauty/drm_ioctl.sh
> > tools/perf/trace/beauty/fadvise.sh
> > tools/perf/trace/beauty/fsconfig.sh
> > tools/perf/trace/beauty/fsmount.sh
> > $
> > $ tools/perf/trace/beauty/fadvise.sh
> > static const char *fadvise_advices[] = {
> > [0] = "NORMAL",
> > [1] = "RANDOM",
> > [2] = "SEQUENTIAL",
> > [3] = "WILLNEED",
> > [4] = "DONTNEED",
> > [5] = "NOREUSE",
> > };
> > $
> >
> > The tools/perf/check-headers.sh script, part of the tools/ build
> > process, points out changes in the original files.
> >
> > So its important not to touch the copies in tools/ when doing changes in
> > the original kernel headers, that will be done later, when
> > check-headers.sh inform about the change to the perf tools hackers.
> >
> > To get the changes in:
> >
> > 1496c47065f9f841 ("vhost-vdpa: uapi to support reporting per vq size")
> >
> > To pick up these changes and support them:
> >
> > $ tools/perf/trace/beauty/vhost_virtio_ioctl.sh > before
> > $ cp include/uapi/linux/vhost.h tools/perf/trace/beauty/include/uapi/linux/vhost.h
> > $ tools/perf/trace/beauty/vhost_virtio_ioctl.sh > after
> > $ diff -u before after
> > --- before 2024-03-21 17:33:58.878173388 -0300
> > +++ after 2024-03-21 17:34:07.687563333 -0300
> > @@ -49,6 +49,7 @@
> > [0x7B] = "VDPA_GET_VRING_GROUP",
> > [0x7F] = "VDPA_GET_VRING_DESC_GROUP",
> > [0x80] = "VDPA_GET_VQS_COUNT",
> > + [0x80] = "VDPA_GET_VRING_SIZE",
>
> This gives me a build error:
>
> CC trace/beauty/ioctl.o
> In file included from trace/beauty/ioctl.c:93:
> linux/tools/perf/trace/beauty/generated/ioctl/vhost_virtio_ioctl_array.c:
> In function ‘ioctl__scnprintf_vhost_virtio_cmd’:
> linux/tools/perf/trace/beauty/generated/ioctl/vhost_virtio_ioctl_array.c:53:18:
> error: initialized field overwritten [-Werror=override-init]
> 53 | [0x80] = "VDPA_GET_VRING_SIZE",
> | ^~~~~~~~~~~~~~~~~~~~~
> linux/tools/perf/trace/beauty/generated/ioctl/vhost_virtio_ioctl_array.c:53:18:
> note: (near initialization for ‘vhost_virtio_ioctl_read_cmds[128]’)
>
> Because there are two entries for 0x80 now.
>
> Is it ok for vhost to have the same number for VQS_COUNT
> and VRING_SIZE?
>
> Thanks,
> Namhyung

Oh!
This is a bug in:

commit 1496c47065f9f8413296780c63374daee9bdae20
Author: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
Date: Sat Feb 3 00:38:56 2024 +0800

vhost-vdpa: uapi to support reporting per vq size

The size of a virtqueue is a per vq configuration.
This commit introduce a new ioctl uAPI to support this flexibility.

Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
Message-Id: <20240202163905.8834-2-lingshan.zhu@xxxxxxxxx>
Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>

It's great that you noticed. It works by chance because one is IOR the
other IOWR and the size is different.

But yes we must fix it - it is not too late this is just in an rc not
in any released kernels.

Will send a fix soon!

>
> > [0x81] = "VDPA_GET_GROUP_NUM",
> > [0x8] = "NEW_WORKER",
> > };
> > $
> >
> > For instance, see how those 'cmd' ioctl arguments get translated, now
> > VDPA_GET_VRING_SIZE will be as well.
> >
> > # perf trace -a -e ioctl --max-events=10
> > 0.000 ( 0.011 ms): pipewire/2261 ioctl(fd: 60, cmd: SNDRV_PCM_HWSYNC, arg: 0x1) = 0
> > 21.353 ( 0.014 ms): pipewire/2261 ioctl(fd: 60, cmd: SNDRV_PCM_HWSYNC, arg: 0x1) = 0
> > 25.766 ( 0.014 ms): gnome-shell/2196 ioctl(fd: 14, cmd: DRM_I915_IRQ_WAIT, arg: 0x7ffe4a22c740) = 0
> > 25.845 ( 0.034 ms): gnome-shel:cs0/2212 ioctl(fd: 14, cmd: DRM_I915_IRQ_EMIT, arg: 0x7fd43915dc70) = 0
> > 25.916 ( 0.011 ms): gnome-shell/2196 ioctl(fd: 9, cmd: DRM_MODE_ADDFB2, arg: 0x7ffe4a22c8a0) = 0
> > 25.941 ( 0.025 ms): gnome-shell/2196 ioctl(fd: 9, cmd: DRM_MODE_ATOMIC, arg: 0x7ffe4a22c840) = 0
> > 32.915 ( 0.009 ms): gnome-shell/2196 ioctl(fd: 9, cmd: DRM_MODE_RMFB, arg: 0x7ffe4a22cf9c) = 0
> > 42.522 ( 0.013 ms): gnome-shell/2196 ioctl(fd: 14, cmd: DRM_I915_IRQ_WAIT, arg: 0x7ffe4a22c740) = 0
> > 42.579 ( 0.031 ms): gnome-shel:cs0/2212 ioctl(fd: 14, cmd: DRM_I915_IRQ_EMIT, arg: 0x7fd43915dc70) = 0
> > 42.644 ( 0.010 ms): gnome-shell/2196 ioctl(fd: 9, cmd: DRM_MODE_ADDFB2, arg: 0x7ffe4a22c8a0) = 0
> > #
> >
> > Cc: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > Cc: Ian Rogers <irogers@xxxxxxxxxx>
> > Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
> > Cc: Zhu Lingshan <lingshan.zhu@xxxxxxxxx>
> > Link: https://lore.kernel.org/lkml/
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
> > ---
> > tools/perf/trace/beauty/include/uapi/linux/vhost.h | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/tools/perf/trace/beauty/include/uapi/linux/vhost.h b/tools/perf/trace/beauty/include/uapi/linux/vhost.h
> > index 649560c685f13b73..bea6973906134656 100644
> > --- a/tools/perf/trace/beauty/include/uapi/linux/vhost.h
> > +++ b/tools/perf/trace/beauty/include/uapi/linux/vhost.h
> > @@ -227,4 +227,11 @@
> > */
> > #define VHOST_VDPA_GET_VRING_DESC_GROUP _IOWR(VHOST_VIRTIO, 0x7F, \
> > struct vhost_vring_state)
> > +
> > +/* Get the queue size of a specific virtqueue.
> > + * userspace set the vring index in vhost_vring_state.index
> > + * kernel set the queue size in vhost_vring_state.num
> > + */
> > +#define VHOST_VDPA_GET_VRING_SIZE _IOWR(VHOST_VIRTIO, 0x80, \
> > + struct vhost_vring_state)
> > #endif
> > --
> > 2.44.0
> >

Next message: Kuppuswamy Sathyanarayanan: "Re: [PATCH v2 4/4] platform/x86: wmi: Add driver development guide"
Previous message: Thomas Weißschuh: "[PATCH v3] fs/proc/proc_sysctl.c: always initialize i_uid/i_gid"
In reply to: Namhyung Kim: "Re: [PATCH fyi 1/1] perf trace beauty: Sync linux/vhost.h with the kernel sources"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH fyi 1/1] perf trace beauty: Sync linux/vhost.h with the kernel sources"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]