| Message ID | 20240729124755.35719-1-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] virtio_net: Avoid sending unnecessary vq coalescing commands | expand |
On Mon, 29 Jul 2024 20:47:55 +0800, Heng Qi <hengqi@linux.alibaba.com> wrote: > From the virtio spec: > > 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 not send vq notification coalescing commands if > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course > applies to vq resize. > > Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0383a3e136d6..eb115e807882 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3708,6 +3708,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > u32 rx_pending, tx_pending; > struct receive_queue *rq; > struct send_queue *sq; > + u32 pkts, usecs; > int i, err; > > if (ring->rx_mini_pending || ring->rx_jumbo_pending) > @@ -3740,11 +3741,13 @@ static int virtnet_set_ringparam(struct net_device *dev, > * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver > * did not set any TX coalescing parameters, to 0. > */ > - err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > - vi->intr_coal_tx.max_usecs, > - vi->intr_coal_tx.max_packets); > - if (err) > - return err; > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > + usecs = vi->intr_coal_tx.max_usecs; > + pkts = vi->intr_coal_tx.max_packets; > + err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > + if (err) > + return err; > + } > } > > if (ring->rx_pending != rx_pending) { > @@ -3753,13 +3756,15 @@ 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; > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > + usecs = vi->intr_coal_rx.max_usecs; > + pkts = vi->intr_coal_rx.max_packets; > + mutex_lock(&vi->rq[i].dim_lock); > + err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > + mutex_unlock(&vi->rq[i].dim_lock); > + if (err) > + return err; > + } > } > } > > -- > 2.32.0.3.g01195cf9f >
On Mon, Jul 29, 2024 at 2:48 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > From the virtio spec: > > 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 not send vq notification coalescing commands if > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course > applies to vq resize. > > Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") Acked-by: Eugenio Pérez <eperezma@redhat.com> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0383a3e136d6..eb115e807882 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3708,6 +3708,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > u32 rx_pending, tx_pending; > struct receive_queue *rq; > struct send_queue *sq; > + u32 pkts, usecs; > int i, err; > > if (ring->rx_mini_pending || ring->rx_jumbo_pending) > @@ -3740,11 +3741,13 @@ static int virtnet_set_ringparam(struct net_device *dev, > * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver > * did not set any TX coalescing parameters, to 0. > */ > - err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > - vi->intr_coal_tx.max_usecs, > - vi->intr_coal_tx.max_packets); > - if (err) > - return err; > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > + usecs = vi->intr_coal_tx.max_usecs; > + pkts = vi->intr_coal_tx.max_packets; > + err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > + if (err) > + return err; > + } > } > > if (ring->rx_pending != rx_pending) { > @@ -3753,13 +3756,15 @@ 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; > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > + usecs = vi->intr_coal_rx.max_usecs; > + pkts = vi->intr_coal_rx.max_packets; > + mutex_lock(&vi->rq[i].dim_lock); > + err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > + mutex_unlock(&vi->rq[i].dim_lock); > + if (err) > + return err; > + } > } > } > > -- > 2.32.0.3.g01195cf9f >
On Mon, 29 Jul 2024 20:47:55 +0800 Heng Qi wrote: > Subject: [PATCH net] virtio_net: Avoid sending unnecessary vq coalescing commands subject currently reads like this is an optimization, could you rephrase? > From the virtio spec: > > 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 not send vq notification coalescing commands if > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course > applies to vq resize. > > Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > --- > drivers/net/virtio_net.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0383a3e136d6..eb115e807882 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3708,6 +3708,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > u32 rx_pending, tx_pending; > struct receive_queue *rq; > struct send_queue *sq; > + u32 pkts, usecs; > int i, err; > > if (ring->rx_mini_pending || ring->rx_jumbo_pending) > @@ -3740,11 +3741,13 @@ static int virtnet_set_ringparam(struct net_device *dev, > * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver > * did not set any TX coalescing parameters, to 0. > */ > - err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > - vi->intr_coal_tx.max_usecs, > - vi->intr_coal_tx.max_packets); > - if (err) > - return err; > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > + usecs = vi->intr_coal_tx.max_usecs; > + pkts = vi->intr_coal_tx.max_packets; > + err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > + if (err) > + return err; Can you check the feature inside the virtnet_send_.x_ctrl_coal_vq_cmd() helpers? 5 levels of indentation is a bit much
On Mon, Jul 29, 2024 at 8:48 PM Heng Qi <hengqi@linux.alibaba.com> wrote: > > From the virtio spec: > > 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 not send vq notification coalescing commands if > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course > applies to vq resize. > > Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > --- Acked-by: Jason Wang <jasowang@redhat.com> Thanks
On Wed, Jul 31, 2024 at 9:20 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 29 Jul 2024 20:47:55 +0800 Heng Qi wrote: > > Subject: [PATCH net] virtio_net: Avoid sending unnecessary vq coalescing commands > > subject currently reads like this is an optimization, could you > rephrase? It might be "virtio-net: unbreak vq resizing when coalescing is not negotiated" ? Thanks
On Wed, 31 Jul 2024 11:13:03 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Wed, Jul 31, 2024 at 9:20 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 29 Jul 2024 20:47:55 +0800 Heng Qi wrote: > > > Subject: [PATCH net] virtio_net: Avoid sending unnecessary vq coalescing commands > > > > subject currently reads like this is an optimization, could you > > rephrase? > > It might be "virtio-net: unbreak vq resizing when coalescing is not > negotiated" ? It's great. Thanks. > > Thanks >
On Tue, 30 Jul 2024 18:20:20 -0700, Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 29 Jul 2024 20:47:55 +0800 Heng Qi wrote: > > Subject: [PATCH net] virtio_net: Avoid sending unnecessary vq coalescing commands > > subject currently reads like this is an optimization, could you > rephrase? Jason's rephrase will be used. > > > From the virtio spec: > > > > 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 not send vq notification coalescing commands if > > VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course > > applies to vq resize. > > > > Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 29 +++++++++++++++++------------ > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0383a3e136d6..eb115e807882 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -3708,6 +3708,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > u32 rx_pending, tx_pending; > > struct receive_queue *rq; > > struct send_queue *sq; > > + u32 pkts, usecs; > > int i, err; > > > > if (ring->rx_mini_pending || ring->rx_jumbo_pending) > > @@ -3740,11 +3741,13 @@ static int virtnet_set_ringparam(struct net_device *dev, > > * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver > > * did not set any TX coalescing parameters, to 0. > > */ > > - err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > > - vi->intr_coal_tx.max_usecs, > > - vi->intr_coal_tx.max_packets); > > - if (err) > > - return err; > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { > > + usecs = vi->intr_coal_tx.max_usecs; > > + pkts = vi->intr_coal_tx.max_packets; > > + err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); > > + if (err) > > + return err; > > Can you check the feature inside the > virtnet_send_.x_ctrl_coal_vq_cmd() helpers? > 5 levels of indentation is a bit much Makes sense. Will update in the next version. Thanks. > -- > pw-bot: cr
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0383a3e136d6..eb115e807882 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3708,6 +3708,7 @@ static int virtnet_set_ringparam(struct net_device *dev, u32 rx_pending, tx_pending; struct receive_queue *rq; struct send_queue *sq; + u32 pkts, usecs; int i, err; if (ring->rx_mini_pending || ring->rx_jumbo_pending) @@ -3740,11 +3741,13 @@ static int virtnet_set_ringparam(struct net_device *dev, * through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver * did not set any TX coalescing parameters, to 0. */ - err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, - vi->intr_coal_tx.max_usecs, - vi->intr_coal_tx.max_packets); - if (err) - return err; + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { + usecs = vi->intr_coal_tx.max_usecs; + pkts = vi->intr_coal_tx.max_packets; + err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); + if (err) + return err; + } } if (ring->rx_pending != rx_pending) { @@ -3753,13 +3756,15 @@ 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; + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) { + usecs = vi->intr_coal_rx.max_usecs; + pkts = vi->intr_coal_rx.max_packets; + mutex_lock(&vi->rq[i].dim_lock); + err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i, usecs, pkts); + mutex_unlock(&vi->rq[i].dim_lock); + if (err) + return err; + } } }
From the virtio spec: 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 not send vq notification coalescing commands if VIRTIO_NET_F_VQ_NOTF_COAL is not negotiated. This limitation of course applies to vq resize. Fixes: f61fe5f081cf ("virtio-net: fix the vq coalescing setting for vq resize") Signed-off-by: Heng Qi <hengqi@linux.alibaba.com> --- drivers/net/virtio_net.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)