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

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

On Fri, Jul 05, 2024 at 01:48:36PM +0800, Jason Wang wrote:
> On Thu, Jul 4, 2024 at 4:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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.
> 
> Not sure I get you, I meant put used_event in a register. Driver will
> only write to that and the device will only read from that..

But the ring is in driver memory.


> >
> >
> >
> > What I see working, is something I proposed a long time ago:
> > notify devices about changes to the notification suppression
> > area.
> 
> I think this is something similar to my proposal, but the difference
> is that I propose to carry the notification suppression into the write
> (e.g used event).

Then it becomes racy.

> > 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.
> 
> It needs a feature so for software devices it can choose to not have
> those overheads.
> 
> Thanks

In short, it's not a trivial thing.

> >
> >
> >
> > --
> > MST
> >