netdev - Re: [PATCH net-next v3 5/6] virtio_net: Add a lock for per queue RX coalesce

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <80934787-ea3c-4b6d-a236-5185430ac92b@linux.alibaba.com>
Date: Mon, 15 Apr 2024 21:41:44 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: Daniel Jurgens <danielj@...dia.com>, netdev@...r.kernel.org
Cc: mst@...hat.com, jasowang@...hat.com, xuanzhuo@...ux.alibaba.com,
 virtualization@...ts.linux.dev, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, jiri@...dia.com
Subject: Re: [PATCH net-next v3 5/6] virtio_net: Add a lock for per queue RX
 coalesce



在 2024/4/13 上午3:53, Daniel Jurgens 写道:
> Once the RTNL locking around the control buffer is removed there can be
> contention on the per queue RX interrupt coalescing data. Use a spin
> lock per queue.
>
> Signed-off-by: Daniel Jurgens <danielj@...dia.com>
> ---
>   drivers/net/virtio_net.c | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b3aa4d2a15e9..8724caa7c2ed 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -190,6 +190,7 @@ struct receive_queue {
>   	u32 packets_in_napi;
>   
>   	struct virtnet_interrupt_coalesce intr_coal;
> +	spinlock_t intr_coal_lock;
>   
>   	/* Chain pages by the private ptr. */
>   	struct page *pages;
> @@ -3087,11 +3088,13 @@ static int virtnet_set_ringparam(struct net_device *dev,
>   				return err;
>   
>   			/* The reason is same as the transmit virtqueue reset */
> -			err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i,
> -							       vi->intr_coal_rx.max_usecs,
> -							       vi->intr_coal_rx.max_packets);
> -			if (err)
> -				return err;
> +			scoped_guard(spinlock, &vi->rq[i].intr_coal_lock) {
> +				err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i,
> +								       vi->intr_coal_rx.max_usecs,
> +								       vi->intr_coal_rx.max_packets);
> +				if (err)
> +					return err;
> +			}
>   		}
>   	}
>   
> @@ -3510,8 +3513,10 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   	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++) {
> -		vi->rq[i].intr_coal.max_usecs = ec->rx_coalesce_usecs;
> -		vi->rq[i].intr_coal.max_packets = ec->rx_max_coalesced_frames;
> +		scoped_guard(spinlock, &vi->rq[i].intr_coal_lock) {
> +			vi->rq[i].intr_coal.max_usecs = ec->rx_coalesce_usecs;
> +			vi->rq[i].intr_coal.max_packets = ec->rx_max_coalesced_frames;
> +		}
>   	}
>   
>   	return 0;
> @@ -3542,6 +3547,7 @@ static int virtnet_send_rx_notf_coal_vq_cmds(struct virtnet_info *vi,
>   	u32 max_usecs, max_packets;
>   	int err;
>   
> +	guard(spinlock)(&vi->rq[queue].intr_coal_lock);
>   	max_usecs = vi->rq[queue].intr_coal.max_usecs;
>   	max_packets = vi->rq[queue].intr_coal.max_packets;
>   
> @@ -3606,6 +3612,7 @@ static void virtnet_rx_dim_work(struct work_struct *work)
>   	if (!rq->dim_enabled)
>   		goto out;

We should also protect rq->dim_enabled access, incorrect values may be 
read in
rx_dim_worker because it is modified in set_coalesce/set_per_queue_coalesce.

Thanks.

>   
> +	guard(spinlock)(&rq->intr_coal_lock);
>   	update_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
>   	if (update_moder.usec != rq->intr_coal.max_usecs ||
>   	    update_moder.pkts != rq->intr_coal.max_packets) {
> @@ -3756,6 +3763,7 @@ static int virtnet_get_per_queue_coalesce(struct net_device *dev,
>   		return -EINVAL;
>   
>   	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) {
> +		guard(spinlock)(&vi->rq[queue].intr_coal_lock);
>   		ec->rx_coalesce_usecs = vi->rq[queue].intr_coal.max_usecs;
>   		ec->tx_coalesce_usecs = vi->sq[queue].intr_coal.max_usecs;
>   		ec->tx_max_coalesced_frames = vi->sq[queue].intr_coal.max_packets;
> @@ -4501,6 +4509,7 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
>   
>   		u64_stats_init(&vi->rq[i].stats.syncp);
>   		u64_stats_init(&vi->sq[i].stats.syncp);
> +		spin_lock_init(&vi->rq[i].intr_coal_lock);
>   	}
>   
>   	return 0;