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

Si-Wei Liu
2024-06-27T17:27:24Z

On 6/27/2024 5:52 AM, Michael S. Tsirkin wrote:
> On Thu, Jun 27, 2024 at 12:45:49PM +0000, Parav Pandit wrote:
>> Hi Michael,
>>
>>> From: Michael S. Tsirkin <mst@redhat.com>
>>> Sent: Thursday, June 27, 2024 6:05 PM
>>>
>>> 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.
>>>
>>> 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?
>>>
>>>
>>> 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.
>>>
>> Your changes above are good and they are applicable when the specific coalescing parameters are set by the driver.
> What makes you say this?
> No they apply universally whenever parameters are specified by the spec:
> whether parameters are set by driver or by device.
> Is that unclear somehow?
> I suspect you are just reading the wrong part of the patch ...
>
>
>> The issue that is being resolved is, when these parameters are NOT set by the driver, when feature is negotiated.
>> What should device do?
>> The parameters are not specified...
> They are specified - set to 0.
>
>> The spec says " Upon reset, a device MUST initialize all coalescing parameters to 0.".
> yes. and device can then coalesce if it wants to.
Well, maybe coalesce here in this context is a little confusing but I
guess I got what you're saying: setting 0 effectively disables the
driver's control of coalescing, right. And device may have its own way
to suppress notification...

>
>> And Heng attempt is to clarify that by adding a changing above line something like,
>>
>> "when driver does not set coalescing params, but have negotiated the feature, device may generate notifications more or less frequently than specified".
>
> Maybe that is the intention, but the proposed text is instead saying the
> parameters themselves are !0. Which is what triggered the whole
> mess with a misvote: commit log says it's a clarification when it is actually
> a behaviour change.
>

+1.

Whatever beautified wording there was, the proposed spec fundamentally
leads to a behavior change.

-Siwei