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

Jason Wang
2024-07-03T05:11:03Z

On Wed, Jul 3, 2024 at 12:33 PM Lege Wang <lege.wang@jaguarmicro.com> wrote:
>
> hi
>
> Thanks for your review.
>
> >
> > > DPU(Data Processing Unit), DPU would need to submit DMAs to copy guest
> > > suppression informations to DPU internal memory, then check whether
> > > used event should be notified by examing this internal memory, which
> > > is obvious expensive, and this DMA operation is needed every time
> > > DPU attempts to issue used event,
> >
> > Several things could be used to help here:
> >
> > 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?

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

But basically, I meant putting used_event in a cap/register other than
inventing something completely new.

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

> For example, driver with VIRTIO_F_EVENT_IDX negotiated
> sets used_event to 0, used buffer notification still could be sent for any used buffer event.

Only once in every #vring_size, no?

>
> > 2) Interrupt coalescing may help to throttle the interrupt rate
> > 3) Spec doesn't forbid implementation specific heuristics for
> > interrupt moderation
> Yes, indeed we're trying to use the information about the latest used ring entry
> driver has processed, which is introduced in this patch, to do interrupt coalescing or moderation.
>
> > > now, but it may choose appropriate timing to send used events according to
> > > its notification coalescing policy , now it also does not need to DMA guest
> > > memory to obtain notification suppression.
> >
> > Performance numbers are welcomed:
> >
> > 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.

> 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
2) we need to know if the existing mechanism can be reused (e.g place
used_event in a register)
3) or how the new method interact with existing interrupt moderation
features (event index, interrupt coalescing etc)

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

>