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

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

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.

> >
> >
> > > >
> > > > >
> > > > > >   2. According to virtio-spec, for example, if used_event is 0, then a device
> > > > > with
> > > > > >   VIRTIO_F_EVENT_IDX would send a used buffer notification to the driver
> > > > > after
> > > > > >   the first buffer is used (and again after the 65536th buffer), should this
> > > > > behavior
> > > > > >   be strictly followed?
> > > > >
> > > > > Used index is best effort, and the driver must handle spurious interrupt.
> > > > OK, I'll discuss this info with my colleague, to see whether it could help.
> > >
> > > Or if you found a bug in the event index, we need to fix it in the spec first.
> >
> > I don't think there's a bug but it's designed to reside in same memory
> > as the used ring.
> 
> Actually, I don't fully understand what Lege said, that's why I'm
> saying "if it's a bug" ...
> 
> >
> > > >
> > > > > > >
> > > > > > > 1) compare disabling via register against memory
> > > > > > > 2) compare auto disable against no auto
> > > > > > I see, but sorry, currently we don't have any performance numbers yet,
> > > > > > but theoretical analysis
> > > > >
> > > > > This might be also helpful, for example, without this proposal x pci
> > > > > transactions per packet, with this proposal y pci transactions per
> > > > > packet.
> > > > I see, later I'll need to see if there's any performance data based on our simulation
> > > > environment to share.
> > >
> > > Great.
> > >
> > > >
> > > > >
> > > > > >  and think it'll help improve performance. Some
> > > > > > popular nic also supports this feature, which disables interrupt after sending
> > > > > > one and needs driver to enable it again. When enabling interrupt again, drivers
> > > > > > can attach some extra information.
> > > > >
> > > > > I'm not saying it doesn't make sense, but t
> > > > >
> > > > > 1) the movitiaton needs to be well explained
> > > > Totally agree.
> > > >
> > > > > 2) we need to know if the existing mechanism can be reused (e.g place
> > > > > used_event in a register)
> > > > That would be better, here the "register" you mentioned is located in a virtio device's
> > > > pci bar?
> > >
> > > Something like this or other place or we can say it could be placed in
> > > a transport specific place.
> >
> > Moving things around changes ordering rules.
> 
> Yes, something needs to be addressed.
> 
> Thanks