Re: [PATCH v5] virtio-net: clarify coalescing parameters settings

Jason Wang
2024-07-03T06:09:42Z

On Sat, Jun 29, 2024 at 3:32 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 6/28/2024 1:23 AM, Jason Wang wrote:
> > On Fri, Jun 28, 2024 at 2:57 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 6/27/2024 3:18 PM, Michael S. Tsirkin wrote:
> >>> On Thu, Jun 27, 2024 at 10:14:49AM -0700, Si-Wei Liu wrote:
> >>>> On 6/27/2024 5:35 AM, Michael S. Tsirkin wrote:
> >>>>> On Thu, Jun 27, 2024 at 12:37:32PM +0200, Halil Pasic wrote:
> >>>>>> On Tue, 25 Jun 2024 18:14:15 -0700
> >>>>>> Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>>>>>
> >>>>>>> On 6/24/2024 10:56 PM, Parav Pandit wrote:
> >>>>>> [..]
> >>>>>>>> I saw the need of this proposal slightly differently in the discussion with Heng in v4.
> >>>>>>>> The way I understood is, proposed relaxation enables below Linux driver flow to work as equally as without device offering VIRTIO_NET_F_VQ_NOTF_COAL.
> >>>>>>>>
> >>>>>>>> Flow is:
> >>>>>>>> 1. The device offered feature VIRTIO_NET_F_VQ_NOTF_COAL
> >>>>>>>> 2. The virtio-net driver negotiated VIRTNET_FEATURES that has VIRTIO_NET_F_VQ_NOTF_COAL
> >>>>>>>>
> >>>>>>>> 3. Because VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, device is not applying any coalescing on the VQ, in a good hope that driver will perform VQ notification coalescing.
> >>>>>> I have certainly understood this differently. When
> >>>>>> VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated then the device is not supposed/allowed to do any interrupt coalescing (notification suppression may still apply).
> >>>>>>
> >>>>>> If VIRTIO_NET_F_VQ_NOTF_COAL is negotiated the device is supposed
> >>>>>> to/MUST do the coalescing according to the parameters as described by
> >>>>>> the virtio spec.
> >>>>>>
> >>>>>> Michael, Jason: Can you guys weigh in on this?
> >>>>> I still don't understand why this change is needed.
> >>>>> We have this text:
> >>>>>
> >>>>> The device may generate notifications more or less frequently than
> >>>>> specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> >>>>>
> >>>>> and
> >>>>>
> >>>>> The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> >>>>> the device MAY generate notifications more or less frequently than specified.
> >>>>>
> >>>>> So with no spec changes, devices already can do what this patch says they can do -
> >>>>> send notifications less frequently.
> >>>> Right. If I still recall it correctly, the above text was to accommodate the
> >>>> interaction with existing used buffer notification mechanism, F_EVENT_IDX or
> >>>> whatnot.
> >>>> When coalescing is in place and effective, interrupt delivery is a
> >>>> collaborative consideration based on the coalescing parameters specified
> >>>> *AND * event index / NO_INTERRUPT flag as well.
> >>> No, not really.
> >>>
> >>> For that we have the next sentence:
> >>>
> >>> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> >>>
> >>>
> >>> And of course event index never makes you send interrupts
> >>> more frequently.
> >> Hmmm, this next sentence you reference above was indeed for the
> >> interaction between coalescing and used buffer suppression. Then what's
> >> the best-effort part was about, really? Round-up or round down the set
> >> value to the power of 2 to save space? How is it relevant to our
> >> discussion? I think even with rounding it shouldn't be too off? (as
> >> said, by best-effort v.s. give up)
> > Would it help if we add something like below in the guidance?
> >
> > When the device wants to send a notification it needs:
> >
> > 1) check notification suppression, don't send interrupt if it is
> > suppressed otherwise goto 2)
> > 2) check event index, don't send interrupt if event index is not
> > crossed otherwise goto 3)
> > 3) check coalescing, don't send interrupt if the coalescing limit is
> > not exceed, otherwise send the interrupt
> >
> > (Just to demonstrate the idea, needs tweaking for sure)
> Yeah, this looks good to me, in principle. I recall there was detailed
> description like this in the early revisions from the first set of
> coalescing proposal, but those useful descriptions were gone for some
> reason. We should really come up with clear clarifications to help
> readers set expectation rather than being vague or worrying too much
> about verbosity.

Yes.

> I was having a hard time in understanding what the
> best-effort part is about...

Being verbose might be helpful for vendors. Otherwise the may guess
(since we don't have a reference software implementation so far).

Thanks

>
> -Siwei
> >
> > THanks
> >
> >
> >> -Siwei
> >>
> >>>>> Re-reading this spec text, maybe the confusion is that it mentions set
> >>>>> commands specifically? And it's also stuck in the middle where it's easy
> >>>>> to miss.
> >>>>>
> >>>>> So it would seem that the following should be sufficient, and it looks
> >>>>> like a small clarification we could just apply and include in the
> >>>>> vote for the csd. What do you guys think?
> >>>> The revised text below looks good to me.
> >>>>
> >>>> Thanks,
> >>>> -Siwei
> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>
> >>>>> ----
> >>>>>
> >>>>>
> >>>>> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> >>>>> index 76585b0..d6788df 100644
> >>>>> --- a/device-types/net/description.tex
> >>>>> +++ b/device-types/net/description.tex
> >>>>> @@ -1711,8 +1711,6 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >>>>> for an enabled transmit/receive virtqueue whose index is \field{vq_index}.
> >>>>> \end{enumerate}
> >>>>> -The device may generate notifications more or less frequently than specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> >>>>> -
> >>>>> If coalescing parameters are being set, the device applies the last coalescing parameters set for a
> >>>>> virtqueue, regardless of the command used to set the parameters. Use the following command sequence
> >>>>> with two pairs of virtqueues as an example:
> >>>>> @@ -1726,6 +1724,9 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >>>>> \item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vq_index} = 1, the device responds with coalescing parameters of index 1 set by command5.
> >>>>> \end{itemize}
> >>>>> +The device can generate notifications more or less frequently
> >>>>> +than specified by the coalescing parameters.
> >>>>> +
> >>>>> \subparagraph{Operation}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / Operation}
> >>>>> The device sends a used buffer notification once the notification conditions are met and if the notifications are not suppressed as explained in \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Used Buffer Notification Suppression}.
> >>>>> @@ -1798,7 +1799,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >>>>> Upon disabling and re-enabling a receive virtqueue, the device MUST set the coalescing parameters of the virtqueue
> >>>>> to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command, or, if the driver did not set any RX coalescing parameters, to 0.
> >>>>> -The behavior of the device in response to set commands of the VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> >>>>> +The behavior of the device in response to specific coalescing parameters is best-effort:
> >>>>> the device MAY generate notifications more or less frequently than specified.
> >>>>> A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> >>>>>
>