| Message ID | 20240412195309.737781-6-danielj@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Remove RTNL lock protection of CVQ | expand |
On Fri, 12 Apr 2024 14:53:08 -0500 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 spin > lock per queue. Does not compile on Clang. > + 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; > + } Do you really think this needs a scoped guard and 4th indentation level, instead of just: ..lock(..); err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i, vi->intr_coal_rx.max_usecs, vi->intr_coal_rx.max_packets); ..unlock(..); if (err) return err; > + 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; > + } :-|
在 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@nvidia.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;
> From: Heng Qi <hengqi@linux.alibaba.com> > Sent: Monday, April 15, 2024 8:42 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; pabeni@redhat.com; Jiri Pirko > <jiri@nvidia.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@nvidia.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. Good point. Thanks > > 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;
> From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, April 12, 2024 9:21 PM > To: Dan Jurgens <danielj@nvidia.com> > Cc: netdev@vger.kernel.org; mst@redhat.com; jasowang@redhat.com; > xuanzhuo@linux.alibaba.com; virtualization@lists.linux.dev; > davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; Jiri > Pirko <jiri@nvidia.com> > Subject: Re: [PATCH net-next v3 5/6] virtio_net: Add a lock for per queue RX > coalesce > > On Fri, 12 Apr 2024 14:53:08 -0500 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 > > spin lock per queue. > > Does not compile on Clang. Which version? It compiles for me with: $ clang -v clang version 15.0.7 (Fedora 15.0.7-2.fc37) > > > + 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; > > + } > > Do you really think this needs a scoped guard and 4th indentation level, > instead of just: > > ..lock(..); > err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i, > vi- > >intr_coal_rx.max_usecs, > vi- > >intr_coal_rx.max_packets); > ..unlock(..); I'll change it in the next version. > if (err) > return err; > > > + 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; > > + } > > :-| > -- > pw-bot: cr
On Tue, 16 Apr 2024 03:15:34 +0000 Dan Jurgens wrote: > Which version? It compiles for me with: > $ clang -v > clang version 15.0.7 (Fedora 15.0.7-2.fc37) clang version 17.0.6 (Fedora 17.0.6-2.fc39) allmodconfig The combination of UNIQUE() goto and guard seems to make it unhappy: ../drivers/net/virtio_net.c:3613:3: error: cannot jump from this goto statement to its label 3613 | goto out; | ^ ../drivers/net/virtio_net.c:3615:2: note: jump bypasses initialization of variable with __attribute__((cleanup)) 3615 | guard(spinlock)(&rq->intr_coal_lock); | ^ ../include/linux/cleanup.h:164:15: note: expanded from macro 'guard' 164 | CLASS(_name, __UNIQUE_ID(guard)) | ^ ../include/linux/compiler.h:189:29: note: expanded from macro '__UNIQUE_ID' 189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) | ^ ./../include/linux/compiler_types.h:84:22: note: expanded from macro '__PASTE' 84 | #define __PASTE(a,b) ___PASTE(a,b) | ^ ./../include/linux/compiler_types.h:83:23: note: expanded from macro '___PASTE' 83 | #define ___PASTE(a,b) a##b | ^ <scratch space>:18:1: note: expanded from here 18 | __UNIQUE_ID_guard2044 | ^ 1 error generated.
> From: Jakub Kicinski <kuba@kernel.org> > Sent: Tuesday, April 16, 2024 9:27 AM > To: Dan Jurgens <danielj@nvidia.com> > On Tue, 16 Apr 2024 03:15:34 +0000 Dan Jurgens wrote: > > Which version? It compiles for me with: > > $ clang -v > > clang version 15.0.7 (Fedora 15.0.7-2.fc37) > > clang version 17.0.6 (Fedora 17.0.6-2.fc39) > Thanks, I was able to see this with the newer version. The changes to address Heng's comment resolves it as well. > allmodconfig > > The combination of UNIQUE() goto and guard seems to make it unhappy: > > ../drivers/net/virtio_net.c:3613:3: error: cannot jump from this goto > statement to its label 3613 | goto out; | ^ > ../drivers/net/virtio_net.c:3615:2: note: jump bypasses initialization of > variable with __attribute__((cleanup)) > 3615 | guard(spinlock)(&rq->intr_coal_lock); > | ^ > ../include/linux/cleanup.h:164:15: note: expanded from macro 'guard' > 164 | CLASS(_name, __UNIQUE_ID(guard)) > | ^ > ../include/linux/compiler.h:189:29: note: expanded from macro > '__UNIQUE_ID' > 189 | #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, > prefix), __COUNTER__) > | ^ > ./../include/linux/compiler_types.h:84:22: note: expanded from macro > '__PASTE' > 84 | #define __PASTE(a,b) ___PASTE(a,b) > | ^ > ./../include/linux/compiler_types.h:83:23: note: expanded from macro > '___PASTE' > 83 | #define ___PASTE(a,b) a##b > | ^ > <scratch space>:18:1: note: expanded from here > 18 | __UNIQUE_ID_guard2044 > | ^ > 1 error generated.
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; + 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;
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@nvidia.com> --- drivers/net/virtio_net.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)