Re: [PATCH] VIRTIO_F_USED_EVENT_AUTO_DISABLE: add new used buffer notification suppression mechanism

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 04, 2024 at 01:37:44PM +0800, Jason Wang wrote:
> On Wed, Jul 3, 2024 at 5:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 03, 2024 at 04:47:41PM +0800, Jason Wang wrote:
> > > On Wed, Jul 3, 2024 at 4:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Wed, Jul 03, 2024 at 03:59:11PM +0800, Jason Wang wrote:
> > > > > On Wed, Jul 3, 2024 at 3:37 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
> > > > > >
> > > > > > hi,
> > > > > >
> > > > > > > > >
> > > > > > > > > 1) With the event index, as long as the used index doesn't pass used
> > > > > > > > > events you don't need to fetch even index every time
> > > > > > > > Yeah, I agree VIRTIO_F_EVENT_IDX could help here, but I think it's a relatively
> > > > > > > > crude mechanism, I have two questions below:
> > > > > > > >   1. Used event notification suppression structure is still located in
> > > > > > > >   host memory(in dpu case), I'm not sure whether used_event would
> > > > > > > >   be allowed to update in the running of one virtio device,
> > > > > > >
> > > > > > > What did you mean by "update" here?
> > > > > > I mean "modify".
> > > > > >
> > > > > > >
> > > > > > > >   If it's allowed,
> > > > > > > >   seems devices still need to fetch newest used_event info timely.
> > > > > > >
> > > > > > > It depends on how you define "timely", I mean unless the used event is
> > > > > > > not crossed, you don't need to fetch it from the main memory?
> > > > > > Yes, I got your point here.
> > > > > > >
> > > > > > > But basically, I meant putting used_event in a cap/register other than
> > > > > > > inventing something completely new.
> > > > > > Sorry, I don't get your point here. What does " cap/register " mean, used_event
> > > > > > Is located in main memory, right?
> > > > >
> > > > > I meant something like this.
> > > > >
> > > > > Introduce a capability to allow the driver to duplicate used_event in
> > > > > the register. And say when the feature is negotiated, the driver MUST
> > > > > update both used_event in the memory and the register.
> > > > >
> > > > > Not saying it can work, but we need to know why it can't work like this.
> > > >
> > > > Well I feel if you are proposing a mechanism it's up to you to
> > > > explain how it works without races.
> > >
> > > I agree, that's why I'm saying "Not saying it can work". But what I
> > > meant is really to find a way to reuse the event index instead of
> > > introducing something completely new.
> > >
> > > > The current notification suppression works because the read
> > > > of the notification by the device flushes out used buffer writes by
> > > > the device.
> > >
> > > You meant read after write is ordered by PCI?
> >
> > pci read responses do not bypass writes, yes.
> >
> > > > If you move it to a separate domain (such as the pci bar of the device)
> > > > this no longer holds.
> > >
> > > Would this be implementation specific details or could it be done by PCI?
> >
> > what do you want done by PCI? Generally if things are in one place
> > they are easier to synchronize, if you spread them around you
> > need to synchronize them.
> 
> I meant the synchronization looks more like an implementation detail
> in the device.  Synchronizing with device internal logic should be
> simpler than with PCI/memory.
> 
> For example, did you mean the synchronization between driver write to
> register (PCI write) and device read from that (internal logic). If
> needed, an implementation needs to serialize those two, then we are
> probably fine.
> 
> Thanks

I mean synchronization between driver write to device and
device write to memory.



What I see working, is something I proposed a long time ago:
notify devices about changes to the notification suppression
area. This adds more overhead
driver notify -> device read -> memory read response
but I think it works.

What I couldn't decide, is whether it's worth sending a notification
when switching from enable to disable with packed ring.



-- 
MST