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

[Si-Wei Liu <si-wei.liu@oracle.com>]

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)

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