Re: [PATCH net v2 2/2] Revert "virtio_net: Add a lock for per queue RX coalesce"

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

在 2024/5/28 下午6:04, Paolo Abeni 写道:
> On Tue, 2024-05-28 at 11:06 +0800, Heng Qi wrote:
>> On Mon, 27 May 2024 12:42:43 +0200, Paolo Abeni <pabeni@redhat.com> wrote:
>>> On Thu, 2024-05-23 at 15:46 +0800, Heng Qi wrote:
>>>> This reverts commit 4d4ac2ececd3c42a08dd32a6e3a4aaf25f7efe44.
>>>>
>>>> When the following snippet is run, lockdep will report a deadlock[1].
>>>>
>>>>    /* Acquire all queues dim_locks */
>>>>    for (i = 0; i < vi->max_queue_pairs; i++)
>>>>            mutex_lock(&vi->rq[i].dim_lock);
>>>>
>>>> There's no deadlock here because the vq locks are always taken
>>>> in the same order, but lockdep can not figure it out, and we
>>>> can not make each lock a separate class because there can be more
>>>> than MAX_LOCKDEP_SUBCLASSES of vqs.
>>>>
>>>> However, dropping the lock is harmless:
>>>>    1. If dim is enabled, modifications made by dim worker to coalescing
>>>>       params may cause the user's query results to be dirty data.
>>> It looks like the above can confuse the user-space/admin?
>> Maybe, but we don't seem to guarantee this --
>> the global query interface (.get_coalesce) cannot
>> guarantee correct results when the DIM and .get_per_queue_coalesce are present:
>>
>> 1. DIM has been around for a long time (it will modify the per-queue parameters),
>>     but many nics only have interfaces for querying global parameters.
>> 2. Some nics provide the .get_per_queue_coalesce interface, it is not
>>     synchronized with DIM.
>>
>> So I think this is acceptable.
> Yes, the above sounds acceptable to me.
>
>>> Have you considered instead re-factoring
>>> virtnet_send_rx_notf_coal_cmds() to avoid acquiring all the mutex in
>>> sequence?
>> Perhaps it is a way to not traverse and update the parameters of each queue
>> in the global settings interface.
> I'm wondering if something as dumb as the following would suffice? Not
> even compile-tested.

This alleviates the problem, and l would like to repost this fix.

Thanks.
> ---
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4a802c0ea2cb..d844f4c89152 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -4267,27 +4267,27 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   			       ec->rx_max_coalesced_frames != vi->intr_coal_rx.max_packets))
>   		return -EINVAL;
>   
> -	/* Acquire all queues dim_locks */
> -	for (i = 0; i < vi->max_queue_pairs; i++)
> -		mutex_lock(&vi->rq[i].dim_lock);
> -
>   	if (rx_ctrl_dim_on && !vi->rx_dim_enabled) {
>   		vi->rx_dim_enabled = true;
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
> +			mutex_lock(&vi->rq[i].dim_lock);
>   			vi->rq[i].dim_enabled = true;
> -		goto unlock;
> +			mutex_unlock(&vi->rq[i].dim_lock);
> +		}
> +		return 0;
>   	}
>   
>   	coal_rx = kzalloc(sizeof(*coal_rx), GFP_KERNEL);
> -	if (!coal_rx) {
> -		ret = -ENOMEM;
> -		goto unlock;
> -	}
> +	if (!coal_rx)
> +		return -ENOMEM;
>   
>   	if (!rx_ctrl_dim_on && vi->rx_dim_enabled) {
>   		vi->rx_dim_enabled = false;
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> +		for (i = 0; i < vi->max_queue_pairs; i++) {
> +			mutex_lock(&vi->rq[i].dim_lock);
>   			vi->rq[i].dim_enabled = false;
> +			mutex_unlock(&vi->rq[i].dim_lock);
> +		}
>   	}
>   
>   	/* Since the per-queue coalescing params can be set,
> @@ -4300,21 +4300,17 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
>   				  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> -				  &sgs_rx)) {
> -		ret = -EINVAL;
> -		goto unlock;
> -	}
> +				  &sgs_rx))
> +		return -EINVAL;
>   
>   	vi->intr_coal_rx.max_usecs = ec->rx_coalesce_usecs;
>   	vi->intr_coal_rx.max_packets = ec->rx_max_coalesced_frames;
>   	for (i = 0; i < vi->max_queue_pairs; i++) {
> +		mutex_lock(&vi->rq[i].dim_lock);
>   		vi->rq[i].intr_coal.max_usecs = ec->rx_coalesce_usecs;
>   		vi->rq[i].intr_coal.max_packets = ec->rx_max_coalesced_frames;
> -	}
> -unlock:
> -	for (i = vi->max_queue_pairs - 1; i >= 0; i--)
>   		mutex_unlock(&vi->rq[i].dim_lock);
> -
> +	}
>   	return ret;
>   }
> ---
>
> Otherwise I think you need to add {READ,WRITE}_ONCE annotations while
> touching the dim fields to avoid data races.
>
> Thanks,
>
> Paolo
>
>
>
>