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

Lege Wang
2024-07-03T07:37:16Z

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?

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

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

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

> 3) or how the new method interact with existing interrupt moderation
> features (event index, interrupt coalescing etc)

Regards,
Xiaoguang Wang

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