[virtio-comment] Re: [virtio-dev] RE: [virtio-comment] RE: [virtio-dev] RE: [virtio-comment] [PATCH v2] virtio-net: support setting coalescing params for multiple vqs

[Heng Qi <hengqi@linux.alibaba.com>]

在 2024/1/22 下午1:03, Parav Pandit 写道:
>
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Monday, January 22, 2024 8:27 AM
>>
>> 在 2024/1/20 下午5:59, Parav Pandit 写道:
>>>> From: Heng Qi <hengqi@linux.alibaba.com>
>>>> Sent: Wednesday, January 17, 2024 10:22 AM
>>>>
>>>> 在 2024/1/15 下午9:21, Parav Pandit 写道:
>>>>>> From: virtio-comment@lists.oasis-open.org
>>>>>> <virtio-comment@lists.oasis- open.org> On Behalf Of Heng Qi
>>>>>> Sent: Monday, January 15, 2024 6:36 PM
>>>>>>
>>>>>> Currently, when each time the driver attempts to update the
>>>>>> coalescing parameters for a vq, it needs to kick the device and
>>>>>> wait for the ctrlq response to return.
>>>>> It does not need to wait. This is some driver limitation that does
>>>>> not use
>>>> the queue as "queue".
>>>>> Such driver limitation should be removed in the driver. It does not
>>>>> qualify
>>>> as limitation.
>>>>
>>>> Yes, we don't have to wait.
>>>>
>>>> But in general, for user commands, it is necessary to obtain the
>>>> final results synchronously.
>>> Yes. Use initiated command can enqueue the request to cvq. Go to sleep
>> for several micro to milliseconds.
>>>> The user command cannot return before the final result is obtained.
>>>> And wait is not the problem this patch solves.
>>>>
>>> By not holding the rtnl lock, rest of the context that needs to enqueue the
>> request can progress such as that of netdim.
>>
>> Would like to see the using of rtnl lock changed.
>>
> Inside the virtnet_rx_dim_work() there should be rtnl lock call.
> A virtio_device level lock to be used for cvq. :)
>
>> In addition, I have made batching and asynchronousization of the netdim
>> command, you can refer to this patch:
>> https://lore.kernel.org/all/1705410693-118895-4-git-send-email-
>> hengqi@linux.alibaba.com/
>>
> In the listed above driver patch the motivation "to optimize the
> CPU overhead of the DIM worker caused by the guest being busy
> waiting for the command response result."
>
> Is not right.
> Because guest is still busy waiting.

There is no busy wait for guests, see get_cvq_work().

> Without batching, due to rtnl lock every VQ command is serialized as one outstanding command at a time in virtnet_rx_dim_work().
> Due to this device is unable to take benefit of DMA batching at large scale.

Adding dim commands is now asynchronous, and the device will receive 
batches of commands.

>   
>>>>> This will enable driver to enqueue multiple cvq commands without
>>>>> waiting
>>>> for previous one.
>>>>> This will also enable device to find natural coalescing done on
>>>>> multiple
>>>> commands.
>>>>
>>>> When batch user commands occur, ensuring synchronization is a concern.
>>>>
>>>>>> The following path is observed: 1. Driver kicks the device; 2.
>>>>>> After the device receives the kick, CPU scheduling occurs and DMA
>>>>>> multiple buffers multiple times; 3. The device completes processing
>>>>>> and replies
>>>> with a response.
>>>>>> When large-queue devices issue multiple requests and kick the
>>>>>> device frequently, this often interrupt the work of the device-side CPU.
>>>>> When there is large devices and multiple driver notifications by a
>>>>> cpu that is N times faster than the device side cpu, the device may
>>>>> find natural
>>>> coalescing on the commands of a given cvq.
>>>>
>>>> First we have to solve the ctrlq batch adding user (ethtool) command.
>>>> Even if processed in a batch way on device side, the number of kicks
>>>> and the number of backend DMAs has not been reduced.
>>> Driver notifications are PCI writes so it should not hamper device side,
>> which can ignore them when they do not bother about it.
>>
>> Driver notifications need to be processed by the DPU, which interferes with
>> the CPU on the DPU.
>>
> I was asking, if there is anyway to disable for your DPU to ignore these notifications while previous one is pending?
>  From your above description, it seems there isn’t.

While the device is processing the request, additional kicks are ignored.

Thanks.

>
>>> Backend DMAs should be reduced by avoiding the LIFO pattern followed by
>> the splitq driver.
>>> Placing the descriptors contiguously like packedq reduces amount of DMA
>> naturally.
>>
>> splitq is widely used, migrating to packedq is not that easy, especially when
>> there are many components and hardware involved.
> I am not suggesting to packed_VQ.
> I am suggesting fixing the driver to not do LIFO on descriptors for splitq.
> In other words, using contiguous set of descriptors on splitq will improve the splitq for DMA.
> This will allow using splitq more efficiently for dma as short-term solution for DMA until more efficient queues are defined.
>
>>> The second predicable DMA to avoid is having 8Bytes of data inline in the
>> descriptor, instead of 16B indirection and extra dma.
>>
>> Looking forward to working inline!
>> But I think this does not conflict with batch work, and combining the two will
>> be more beneficial.
>>
> It does not conflict. However, batching for large number of queues may not use the inline as the data bytes may not fit in the inline.
>   
>>>>> For multiple DMAs, we need to way to send 8 bytes of data without 16
>>>> bytes of indirection via a descriptor.
>>>>> This is what we discussed a while back to do in txq and Stefan
>>>>> suggested to
>>>> generalize for more queues, which is also a good idea.
>>>>
>>>> Yes, this sounds good.
>>>>
>>> This the next item to focus as soon as flow filters are stable/merged.
>> Greate!
>>
>>>>>> In addition,
>>>>>> each vq request is processed separately, causing more delays for
>>>>>> the CPU to wait for the DMA request to complete.
>>>>>>
>>>>>> These interruptions and overhead will strain the CPU responsible
>>>>>> for controlling the path of the DPU, especially in multi-device and
>>>>>> large-queue scenarios.
>>>>>>
>>>>>> To solve the above problems, we internally tried batch request,
>>>>>> which merges requests from multiple queues and sends them at once.
>>>>>> We conservatively tested
>>>>> The batching done may end up modifying the given VQ's parameters
>>>> multiple times.
>>>>
>>>> In practice, we do not try to accumulate multiple parameter
>>>> modifications for a specific vqn.
>>> I meant to accumulate parameters of multiple VQs to batch in single
>> command.
>>
>> Yes, this is what is done now, practical example:
>> https://lore.kernel.org/all/1705410693-118895-3-git-send-email-
>> hengqi@linux.alibaba.com/
>> In our scenario, only benefits are seen, no negative effects.
>>
> The right way to test is without the rtnl lock and enqueue the command to cvq without being blocked.
>
> The new optional batch command with new feature bit may be useful for devices to which CVQ kicks are interruptions.
>   
>>>>> The right test on Linux to do without rtnl lock which is anyway ugly
>>>>> and
>>>> wrong semantic to use blocking the whole netdev stack.
>>>>> (in case if you used that).
>>>> Do you have any good directions and attempts to remove rtnl_lock?
>>>>
>>> I think per device lock instead of rtnl is first step that we can start with.
> Wil check internally who if someone already started working on it.
>
>> Totally looking forward to it.
>>
>> Thanks,
>> Heng
>>
>>>>>> 8 queue commands and sent them together. The DPU processing
>>>>>> efficiency can be improved by 8 times, which greatly eases the
>>>>>> DPU's support for multi- device and multi-queue DIM.
>>>>>>
>>>>> This is good.
>>>> YES. Makes sense for our DPUs.
>>>>
>>>>>> Maintainers may be concerned about whether the batch command
>>>> method
>>>>>> can optimize the above problems: accumulate multiple request
>>>> commands
>>>>>> to kick the device once, and obtain the processing results of the
>>>>>> corresponding commands asynchronously.
>>>>> This is unlikely to improve, rather it will have negative impact as
>>>>> it only
>>>> means that moderation parameters are just delayed by the driver.
>>>> Why is it delayed by the driver? It is not delayed by the driver, the
>>>> kick still happens for every command.
>>> Since the kick is delayed, the driver lost the amount of time and device
>> reaction time is driven by this wait time.
>>> The guest driver is not aware how much a shared DPU is busy.
>>> So if it waits too much, the VQs moderation is slow.
>>>
>>>> In theory and practice, it will not affect DIM performance, but it
>>>> will significantly reduce CPU consumption caused by waiting.
>>>>
>>> Once the guest driver sleeps after enqueuing the request, it should be fine.
>>> The DIM calls are in highly parallel worker threads, where/if the thread
>> sleep, a new worker thread is invoked.
>>> But for sure I agree that vq notification moderation should happen quickly
>> enough under < 100usec or so.
>>> May be now that we bring the aq resources, we can start migrating on it.
>>>
>>>>>> The batch command method is used by us to optimize the CPU
>> overhead
>>>>>> of the DIM worker caused by the guest being busy waiting for the
>>>>>> command response result.
>>>>> In that case fixing the guest driver which is not yet written is the right fix.
>>>>>
>>>>>> This is a different focus than batch request to solve the problem.
>>>>>>
>>>>>> Suggested-by: Xiaoming Zhao <zxm377917@alibaba-inc.com>
>>>>>> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
>>>>>> ---
>>>>>> v1->v2: Updated commit log. Due to sensitivity, sorry that can not
>>>>>> v1->give the
>>>>>>        absolute value directly. @Michael
>>>>>>
>>>>>>     device-types/net/description.tex | 26 ++++++++++++++++++++------
>>>>>>     1 file changed, 20 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/device-types/net/description.tex b/device-
>>>>>> types/net/description.tex index aff5e08..b3766c4 100644
>>>>>> --- a/device-types/net/description.tex
>>>>>> +++ b/device-types/net/description.tex
>>>>>> @@ -1667,8 +1667,8 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi  for
>>>>>> notification coalescing.
>>>>>>
>>>>>>     If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated, the
>>>>>> driver can - send commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
>> and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET -for virtqueue notification
>>>>>> coalescing.
>>>>>> +send commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
>>>>>> +VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET for virtqueue notification
>>>>>> coalescing.
>>>>>>
>>>>> A new feature bit is needed for this extra functionality.
>>>> I tried to extend it to the command VIRTIO_NET_F_VQ_NOTF_COAL, is it
>>>> too late?
>>>>
>>> For existing command it is not.
>>> But there may be a device which may not support coalesced command.
>>> Better to have the feature bit for this coalesce command, instead of
>> random NOSUPP failure.
>>>
>>>> Thanks!
>>>>
>>>>>>     \begin{lstlisting}
>>>>>>     struct virtio_net_ctrl_coal {
>>>>>> @@ -1682,11 +1682,17 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>         struct virtio_net_ctrl_coal coal;
>>>>>>     };
>>>>>>
>>>>>> +struct virtio_net_ctrl_mrg_coal_vq {
>>>>>> +        le16 num_entries; /* indicates number of valid entries */
>>>>>> +        struct virtio_net_ctrl_coal_vq entries[]; };
>>>>>> +
>>>>>>     #define VIRTIO_NET_CTRL_NOTF_COAL 6
>>>>>>      #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
>>>>>>      #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
>>>>>>      #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
>>>>>>      #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
>>>>>> + #define VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET 4
>>>>>>     \end{lstlisting}
>>>>>>
>>>>>>     Coalescing parameters:
>>>>>> @@ -1706,6 +1712,7 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi  \item
>>>>>> For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the
>> structure
>>>>>> virtio_net_ctrl_coal_vq is write-only for the driver.
>>>>>>     \item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET,
>>>>>> \field{vq_index} and \field{reserved} are write-only
>>>>>>           for the driver, and the structure virtio_net_ctrl_coal is
>>>>>> read-only for the driver.
>>>>>> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET, the
>>>>>> structure virtio_net_ctrl_mrg_coal_vq is write-only for the driver.
>>>>>>     \end{itemize}
>>>>>>
>>>>>>     The class VIRTIO_NET_CTRL_NOTF_COAL has the following
>> commands:
>>>>>> @@ -1716,6 +1723,9 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>                                             for an enabled
>>>>>> transmit/receive virtqueue whose index is \field{vq_index}.
>>>>>>     \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: use the structure
>>>>>> virtio_net_ctrl_coal_vq to get the \field{max_usecs} and
>>>>>> \field{max_packets} parameters
>>>>>>                                             for an enabled
>>>>>> transmit/receive virtqueue whose index is \field{vq_index}.
>>>>>> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET: use the structure
>>>>>> virtio_net_ctrl_mrg_coal_vq to set the \field{max_usecs} and
>>>>>> \field{max_packets} parameters
>>>>>> +                                         for \field{num_entries}
>>>>>> + enabled transmit/receive
>>>>>> virtqueues. The corresponding index value
>>>>>> +                                         of each configured virtqueue 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.
>>>>>> @@ -1782,9 +1792,13 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>
>>>>>>     The driver MUST set \field{vq_index} to the virtqueue index of
>>>>>> an enabled transmit or receive virtqueue.
>>>>>>
>>>>>> +The driver MUST set \field{num_entries} to a non-zero value and
>>>>>> +MUST NOT set \field{num_entries} to a value greater than the
>>>>>> +number of enabled
>>>>>> transmit and receive virtqueues.
>>>>>> +
>>>>>>     The driver MUST have negotiated the VIRTIO_NET_F_NOTF_COAL
>>>> feature
>>>>>> when issuing commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
>>>>>>
>>>>>> -The driver MUST have negotiated the VIRTIO_NET_F_VQ_NOTF_COAL
>>>>>> feature when issuing commands
>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
>>>> and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET.
>>>>>> +The driver MUST have negotiated the VIRTIO_NET_F_VQ_NOTF_COAL
>>>>>> feature
>>>>>> +when issuing commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET.
>>>>>>
>>>>>>     The driver MUST ignore the values of coalescing parameters
>>>>>> received from the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command if
>> the
>>>>>> device responds with VIRTIO_NET_ERR.
>>>>>>
>>>>>> @@ -1794,10 +1808,10 @@ \subsubsection{Control
>>>>>> Virtqueue}\label{sec:Device Types / Network Device / Devi
>>>>>>
>>>>>>     The device SHOULD respond to
>> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>>> and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands with
>>>> VIRTIO_NET_ERR if it
>>>>>> was not able to change the parameters.
>>>>>>
>>>>>> -The device MUST respond to the
>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
>>>>>> command with VIRTIO_NET_ERR if it was not able to change the
>>>> parameters.
>>>>>> +The device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
>>>> and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET commands with
>>>> VIRTIO_NET_ERR if it
>>>>>> was not able to change the parameters.
>>>>>>
>>>>>> -The device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET
>>>> and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with -
>>>> VIRTIO_NET_ERR if the
>>>>>> designated virtqueue is not an enabled transmit or receive virtqueue.
>>>>>> +The device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
>>>>>> +VIRTIO_NET_CTRL_NOTF_COAL_VQS_SET and
>>>>>> VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with
>>>> VIRTIO_NET_ERR if the
>>>>>> designated virtqueue is not an enabled transmit or receive virtqueue.
>>>>>>
>>>>>>     Upon disabling and re-enabling a transmit virtqueue, the device
>>>>>> MUST set the coalescing parameters of the virtqueue  to those
>>>>>> configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
>>>> command, or,
>>>>>> if the driver did not set any TX coalescing parameters, to 0.
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>>>> This publicly archived list offers a means to provide input to the
>>>>>> OASIS Virtual I/O Device (VIRTIO) TC.
>>>>>>
>>>>>> In order to verify user consent to the Feedback License terms and
>>>>>> to minimize spam in the list archive, subscription is required
>>>>>> before posting.
>>>>>>
>>>>>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>>>>>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>>>>>> List help: virtio-comment-help@lists.oasis-open.org
>>>>>> List archive: https://lists.oasis-open.org/archives/virtio-comment/
>>>>>> Feedback License: https://www.oasis-
>>>>>> open.org/who/ipr/feedback_license.pdf
>>>>>> List Guidelines:
>>>>>> https://www.oasis-open.org/policies-guidelines/mailing-lists
>>>>>> Committee: https://www.oasis-open.org/committees/virtio/
>>>>>> Join OASIS: https://www.oasis-open.org/join/
>>>>> --------------------------------------------------------------------
>>>>> - To unsubscribe, e-mail:
>>>>> virtio-dev-unsubscribe@lists.oasis-open.org
>>>>> For additional commands, e-mail:
>>>>> virtio-dev-help@lists.oasis-open.org


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/