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

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

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