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

Jason Wang
2024-07-03T07:59:26Z

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.

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

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

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