diff mbox series

[net-next,v5,5/6] virtio_net: Add a lock for per queue RX coalesce

Message ID 20240423035746.699466-6-danielj@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Remove RTNL lock protection of CVQ | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-24--15-00 (tests: 995)

Commit Message

Dan Jurgens April 23, 2024, 3:57 a.m. UTC 
Once the RTNL locking around the control buffer is removed there can be
contention on the per queue RX interrupt coalescing data. Use a mutex
per queue. A mutex is required because virtnet_send_command can sleep.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
---
 drivers/net/virtio_net.c | 53 +++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 12 deletions(-)

Comments

Paolo Abeni April 26, 2024, 9:47 a.m. UTC | #1 
On Tue, 2024-04-23 at 06:57 +0300, Daniel Jurgens wrote:
> Once the RTNL locking around the control buffer is removed there can be
> contention on the per queue RX interrupt coalescing data. Use a mutex
> per queue. A mutex is required because virtnet_send_command can sleep.
> 
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> ---
>  drivers/net/virtio_net.c | 53 +++++++++++++++++++++++++++++++---------
>  1 file changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index af9048ddc3c1..033e1d6ea31b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -184,6 +184,9 @@ struct receive_queue {
>  	/* Is dynamic interrupt moderation enabled? */
>  	bool dim_enabled;
>  
> +	/* Used to protect dim_enabled and inter_coal */
> +	struct mutex dim_lock;
> +
>  	/* Dynamic Interrupt Moderation */
>  	struct dim dim;
>  
> @@ -2218,6 +2221,10 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>  	/* Out of packets? */
>  	if (received < budget) {
>  		napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
> +		/* Intentionally not taking dim_lock here. This could result
> +		 * in a net_dim call with dim now disabled. But virtnet_rx_dim_work
> +		 * will take the lock not update settings if dim is now disabled.

Minor nit: the above comment looks confusing/mangled to me ?!?

		   will take the lock and will not update settings...

Thanks,

Paolo
Dan Jurgens April 26, 2024, 1:14 p.m. UTC | #2 
> From: Paolo Abeni <pabeni@redhat.com>
> Sent: Friday, April 26, 2024 4:48 AM
> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
> virtualization@lists.linux.dev; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; Jiri Pirko <jiri@nvidia.com>
> Subject: Re: [PATCH net-next v5 5/6] virtio_net: Add a lock for per queue RX
> coalesce
> 
> On Tue, 2024-04-23 at 06:57 +0300, Daniel Jurgens wrote:
> > Once the RTNL locking around the control buffer is removed there can
> > be contention on the per queue RX interrupt coalescing data. Use a
> > mutex per queue. A mutex is required because virtnet_send_command
> can sleep.
> >
> > Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> > ---
> >  drivers/net/virtio_net.c | 53
> > +++++++++++++++++++++++++++++++---------
> >  1 file changed, 41 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > af9048ddc3c1..033e1d6ea31b 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -184,6 +184,9 @@ struct receive_queue {
> >  	/* Is dynamic interrupt moderation enabled? */
> >  	bool dim_enabled;
> >
> > +	/* Used to protect dim_enabled and inter_coal */
> > +	struct mutex dim_lock;
> > +
> >  	/* Dynamic Interrupt Moderation */
> >  	struct dim dim;
> >
> > @@ -2218,6 +2221,10 @@ static int virtnet_poll(struct napi_struct *napi, int
> budget)
> >  	/* Out of packets? */
> >  	if (received < budget) {
> >  		napi_complete = virtqueue_napi_complete(napi, rq->vq,
> received);
> > +		/* Intentionally not taking dim_lock here. This could result
> > +		 * in a net_dim call with dim now disabled. But
> virtnet_rx_dim_work
> > +		 * will take the lock not update settings if dim is now disabled.
> 
> Minor nit: the above comment looks confusing/mangled to me ?!?

I wanted to note that dim_lock is being accessed here, without the lock. But it's intentional. If there is racing a spurious net dim call can happen. But the dim_work handler will take the lock, see the correct value, and do nothing if dim is now disabled.
> 
> 		   will take the lock and will not update settings...
> 
> Thanks,
> 
> Paolo
diff mbox series

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index af9048ddc3c1..033e1d6ea31b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -184,6 +184,9 @@  struct receive_queue {
 	/* Is dynamic interrupt moderation enabled? */
 	bool dim_enabled;
 
+	/* Used to protect dim_enabled and inter_coal */
+	struct mutex dim_lock;
+
 	/* Dynamic Interrupt Moderation */
 	struct dim dim;
 
@@ -2218,6 +2221,10 @@  static int virtnet_poll(struct napi_struct *napi, int budget)
 	/* Out of packets? */
 	if (received < budget) {
 		napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
+		/* Intentionally not taking dim_lock here. This could result
+		 * in a net_dim call with dim now disabled. But virtnet_rx_dim_work
+		 * will take the lock not update settings if dim is now disabled.
+		 */
 		if (napi_complete && rq->dim_enabled)
 			virtnet_rx_dim_update(vi, rq);
 	}
@@ -3091,9 +3098,11 @@  static int virtnet_set_ringparam(struct net_device *dev,
 				return err;
 
 			/* The reason is same as the transmit virtqueue reset */
+			mutex_lock(&vi->rq[i].dim_lock);
 			err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i,
 							       vi->intr_coal_rx.max_usecs,
 							       vi->intr_coal_rx.max_packets);
+			mutex_unlock(&vi->rq[i].dim_lock);
 			if (err)
 				return err;
 		}
@@ -3472,6 +3481,7 @@  static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
 	struct virtio_net_ctrl_coal_rx *coal_rx __free(kfree) = NULL;
 	bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce;
 	struct scatterlist sgs_rx;
+	int ret = 0;
 	int i;
 
 	if (rx_ctrl_dim_on && !virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
@@ -3481,16 +3491,22 @@  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++)
 			vi->rq[i].dim_enabled = true;
-		return 0;
+		goto unlock;
 	}
 
 	coal_rx = kzalloc(sizeof(*coal_rx), GFP_KERNEL);
-	if (!coal_rx)
-		return -ENOMEM;
+	if (!coal_rx) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
 
 	if (!rx_ctrl_dim_on && vi->rx_dim_enabled) {
 		vi->rx_dim_enabled = false;
@@ -3508,8 +3524,10 @@  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))
-		return -EINVAL;
+				  &sgs_rx)) {
+		ret = -EINVAL;
+		goto unlock;
+	}
 
 	vi->intr_coal_rx.max_usecs = ec->rx_coalesce_usecs;
 	vi->intr_coal_rx.max_packets = ec->rx_max_coalesced_frames;
@@ -3517,8 +3535,11 @@  static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
 		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 0;
+	return ret;
 }
 
 static int virtnet_send_notf_coal_cmds(struct virtnet_info *vi,
@@ -3542,19 +3563,24 @@  static int virtnet_send_rx_notf_coal_vq_cmds(struct virtnet_info *vi,
 					     u16 queue)
 {
 	bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce;
-	bool cur_rx_dim = vi->rq[queue].dim_enabled;
 	u32 max_usecs, max_packets;
+	bool cur_rx_dim;
 	int err;
 
+	mutex_lock(&vi->rq[queue].dim_lock);
+	cur_rx_dim = vi->rq[queue].dim_enabled;
 	max_usecs = vi->rq[queue].intr_coal.max_usecs;
 	max_packets = vi->rq[queue].intr_coal.max_packets;
 
 	if (rx_ctrl_dim_on && (ec->rx_coalesce_usecs != max_usecs ||
-			       ec->rx_max_coalesced_frames != max_packets))
+			       ec->rx_max_coalesced_frames != max_packets)) {
+		mutex_unlock(&vi->rq[queue].dim_lock);
 		return -EINVAL;
+	}
 
 	if (rx_ctrl_dim_on && !cur_rx_dim) {
 		vi->rq[queue].dim_enabled = true;
+		mutex_unlock(&vi->rq[queue].dim_lock);
 		return 0;
 	}
 
@@ -3567,10 +3593,8 @@  static int virtnet_send_rx_notf_coal_vq_cmds(struct virtnet_info *vi,
 	err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, queue,
 					       ec->rx_coalesce_usecs,
 					       ec->rx_max_coalesced_frames);
-	if (err)
-		return err;
-
-	return 0;
+	mutex_unlock(&vi->rq[queue].dim_lock);
+	return err;
 }
 
 static int virtnet_send_notf_coal_vq_cmds(struct virtnet_info *vi,
@@ -3607,6 +3631,7 @@  static void virtnet_rx_dim_work(struct work_struct *work)
 
 	qnum = rq - vi->rq;
 
+	mutex_lock(&rq->dim_lock);
 	if (!rq->dim_enabled)
 		goto out;
 
@@ -3622,6 +3647,7 @@  static void virtnet_rx_dim_work(struct work_struct *work)
 		dim->state = DIM_START_MEASURE;
 	}
 out:
+	mutex_unlock(&rq->dim_lock);
 	rtnl_unlock();
 }
 
@@ -3760,11 +3786,13 @@  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)) {
+		mutex_lock(&vi->rq[queue].dim_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;
 		ec->rx_max_coalesced_frames = vi->rq[queue].intr_coal.max_packets;
 		ec->use_adaptive_rx_coalesce = vi->rq[queue].dim_enabled;
+		mutex_unlock(&vi->rq[queue].dim_lock);
 	} else {
 		ec->rx_max_coalesced_frames = 1;
 
@@ -4505,6 +4533,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);
+		mutex_init(&vi->rq[i].dim_lock);
 	}
 
 	return 0;