Re: [PATCH 1/4] vfio/migration: Add save_{iterate,complete_precopy}_started trace events

[Maciej S. Szmigiero]

On 31.10.2024 23:17, Maciej S. Szmigiero wrote:
> Hi Avihai,
>
> On 31.10.2024 15:21, Avihai Horon wrote:
> > Hi Maciej,
> >
> > On 29/10/2024 16:58, Maciej S. Szmigiero wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > From: "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>
> > >
> > > This way both the start and end points of migrating a particular VFIO
> > > device are known.
> > >
> > > Add also a vfio_save_iterate_empty_hit trace event so it is known when
> > > there's no more data to send for that device.
> > >
> > > Signed-off-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
> > > ---
> > >   hw/vfio/migration.c           | 13 +++++++++++++
> > >   hw/vfio/trace-events          |  3 +++
> > >   include/hw/vfio/vfio-common.h |  3 +++
> > >   3 files changed, 19 insertions(+)
> > >
> > > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > > index 992dc3b10257..1b1ddf527d69 100644
> > > --- a/hw/vfio/migration.c
> > > +++ b/hw/vfio/migration.c
(..)
> > In any case, I think the above could fit better in vfio_save_block(), where 
> > ENOMSG indicates that the device has no more data to send during pre-copy phase:
> >
> > ...
> > if (data_size < 0) {
> >      /*
> >       * Pre-copy emptied all the device state for now. For more information,
> >       * please refer to the Linux kernel VFIO uAPI.
> >       */
> >      if (errno == ENOMSG) {
> > trace_vfio_save_iterate_empty_hit(vbasedev->name) <--------------- move it here
> >          return 0;
> >      }
> >
> >      return -errno;
> > }
> > ...
> >
> > If you move the trace there, maybe renaming it to 
> > trace_vfio_precopy_empty_hit() will be more accurate?
>
> This move and rename seems sensible to me.
>
> > And trying to avoid adding the extra VFIOMigration->save_iterate_empty_hit 
> > flag, can we simply trace it every time?
>
> Will have to do some tests to be sure but if there's possibility that
> we get ENOMSG many times then obviously we don't want to flood logs with
> this trace event in this case - we want to only log the
> "data present" -> "data not present" edge/change.

Indeed - it needs to be triggered only on the "non-empty"/"empty" change
otherwise there are 3 repeats of this trace event per VFIO device in
a rapid succession.

So some kind of a flag here is necessary: with the trace event being
"re-armed" by a non-empty read from the VFIO device I get just one such
event per device.

Thanks,
Maciej