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

Jason Wang
2024-07-03T08:47:58Z

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?

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

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