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

Michael S. Tsirkin
2024-07-03T08:36:29Z

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.
The current notification suppression works because the read
of the notification by the device flushes out used buffer writes by
the device.
If you move it to a separate domain (such as the pci bar of the device)
this no longer holds.

> >
> > >
> > > > 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.

> >
> > > > >
> > > > > 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.

> >
> > > 3) or how the new method interact with existing interrupt moderation
> > > features (event index, interrupt coalescing etc)
> >
> > Regards,
> > Xiaoguang Wang
>
> Thanks
>
> >
> > >
> > > >
> > > > >
> > > > > > + The driver may benefit from just informing device the latest used
> > > > > ring entry
> > > > > > + it has processed, but not enabling used buffer notification. This
> > > field
> > > > > only
> > > > > > + occupies one bit, if set to 1, it will enable used buffer notification,
> > > > > otherwise not.
> > > > > > +\end{description}
> > > > >
> > > > > Any reason we can't reuse the existing meachsim like used index /
> > > > > event suppression here?
> > > > Please see my comments above.
> > > >
> > > > Regards,
> > > > Xiaoguang Wang
> > > > >
> > > > > Basically it just sounds like we need a transport specific method for
> > > > > them besides being part of the virtqueue.
> > > > >
> > > > > Thanks
> > >
> > > Thanks
> > >
> > > >
> >