| Message ID | 20240731120717.49955-1-hengqi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] virtio-net: unbreak vq resizing when coalescing is not negotiated | expand |
On Wed, Jul 31, 2024 at 08:07:17PM +0800, Heng Qi 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> > Acked-by: Eugenio Pé rez <eperezma@redhat.com> > Acked-by: Jason Wang <jasowang@redhat.com> > --- > v1->v2: > - Rephrase the subject. > - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). > > drivers/net/virtio_net.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 0383a3e136d6..2b566d893ea3 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > { > int err; > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > + return -EOPNOTSUPP; > + > err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), > max_usecs, max_packets); > if (err) > @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > { > int err; > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > + return -EOPNOTSUPP; > + > err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), > max_usecs, max_packets); > if (err) > @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > vi->intr_coal_tx.max_usecs, > vi->intr_coal_tx.max_packets); > - if (err) > + if (err && err != -EOPNOTSUPP) > return err; > } > So far so good. > @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > vi->intr_coal_rx.max_usecs, > vi->intr_coal_rx.max_packets); > mutex_unlock(&vi->rq[i].dim_lock); > - if (err) > + if (err && err != -EOPNOTSUPP) > return err; > } > } I don't get this one. If resize is not supported, we pretend it was successful? Why? > -- > 2.32.0.3.g01195cf9f
On Wed, 31 Jul 2024 08:14:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 31, 2024 at 08:07:17PM +0800, Heng Qi 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> > > Acked-by: Eugenio Pé rez <eperezma@redhat.com> > > Acked-by: Jason Wang <jasowang@redhat.com> > > --- > > v1->v2: > > - Rephrase the subject. > > - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). > > > > drivers/net/virtio_net.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 0383a3e136d6..2b566d893ea3 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > { > > int err; > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > + return -EOPNOTSUPP; > > + > > err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), > > max_usecs, max_packets); > > if (err) > > @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > { > > int err; > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > + return -EOPNOTSUPP; > > + > > err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), > > max_usecs, max_packets); > > if (err) > > @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > > vi->intr_coal_tx.max_usecs, > > vi->intr_coal_tx.max_packets); > > - if (err) > > + if (err && err != -EOPNOTSUPP) > > return err; > > } > > > > > So far so good. > > > @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > vi->intr_coal_rx.max_usecs, > > vi->intr_coal_rx.max_packets); > > mutex_unlock(&vi->rq[i].dim_lock); > > - if (err) > > + if (err && err != -EOPNOTSUPP) > > return err; > > } > > } > > I don't get this one. If resize is not supported, Here means that the *dim feature* is not supported, not the *resize* feature. > we pretend it was successful? Why? During a resize, if the dim feature is not supported, the driver does not need to try to recover any coalescing values, since the device does not have these parameters. Therefore, the resize should continue without interruption. Thanks. > > > -- > > 2.32.0.3.g01195cf9f >
On Wed, Jul 31, 2024 at 08:25:23PM +0800, Heng Qi wrote: > On Wed, 31 Jul 2024 08:14:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Jul 31, 2024 at 08:07:17PM +0800, Heng Qi 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> > > > Acked-by: Eugenio Pé rez <eperezma@redhat.com> > > > Acked-by: Jason Wang <jasowang@redhat.com> > > > --- > > > v1->v2: > > > - Rephrase the subject. > > > - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). > > > > > > drivers/net/virtio_net.c | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index 0383a3e136d6..2b566d893ea3 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > { > > > int err; > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > + return -EOPNOTSUPP; > > > + > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), > > > max_usecs, max_packets); > > > if (err) > > > @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > { > > > int err; > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > + return -EOPNOTSUPP; > > > + > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), > > > max_usecs, max_packets); > > > if (err) > > > @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > > > vi->intr_coal_tx.max_usecs, > > > vi->intr_coal_tx.max_packets); > > > - if (err) > > > + if (err && err != -EOPNOTSUPP) > > > return err; > > > } > > > > > > > > > So far so good. > > > > > @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > vi->intr_coal_rx.max_usecs, > > > vi->intr_coal_rx.max_packets); > > > mutex_unlock(&vi->rq[i].dim_lock); > > > - if (err) > > > + if (err && err != -EOPNOTSUPP) > > > return err; > > > } > > > } > > > > I don't get this one. If resize is not supported, > > Here means that the *dim feature* is not supported, not the *resize* feature. > > > we pretend it was successful? Why? > > During a resize, if the dim feature is not supported, the driver does not > need to try to recover any coalescing values, since the device does not have > these parameters. > Therefore, the resize should continue without interruption. > > Thanks. you mean it's a separate bugfix? > > > > > -- > > > 2.32.0.3.g01195cf9f > >
On Wed, 31 Jul 2024 08:46:42 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Jul 31, 2024 at 08:25:23PM +0800, Heng Qi wrote: > > On Wed, 31 Jul 2024 08:14:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Jul 31, 2024 at 08:07:17PM +0800, Heng Qi 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> > > > > Acked-by: Eugenio Pé rez <eperezma@redhat.com> > > > > Acked-by: Jason Wang <jasowang@redhat.com> > > > > --- > > > > v1->v2: > > > > - Rephrase the subject. > > > > - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). > > > > > > > > drivers/net/virtio_net.c | 10 ++++++++-- > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index 0383a3e136d6..2b566d893ea3 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > > { > > > > int err; > > > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > > + return -EOPNOTSUPP; > > > > + > > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), > > > > max_usecs, max_packets); > > > > if (err) > > > > @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > > { > > > > int err; > > > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > > + return -EOPNOTSUPP; > > > > + > > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), > > > > max_usecs, max_packets); > > > > if (err) > > > > @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > > err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > > > > vi->intr_coal_tx.max_usecs, > > > > vi->intr_coal_tx.max_packets); > > > > - if (err) > > > > + if (err && err != -EOPNOTSUPP) > > > > return err; > > > > } > > > > > > > > > > > > > So far so good. > > > > > > > @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > > vi->intr_coal_rx.max_usecs, > > > > vi->intr_coal_rx.max_packets); > > > > mutex_unlock(&vi->rq[i].dim_lock); > > > > - if (err) > > > > + if (err && err != -EOPNOTSUPP) > > > > return err; > > > > } > > > > } > > > > > > I don't get this one. If resize is not supported, > > > > Here means that the *dim feature* is not supported, not the *resize* feature. > > > > > we pretend it was successful? Why? > > > > During a resize, if the dim feature is not supported, the driver does not > > need to try to recover any coalescing values, since the device does not have > > these parameters. > > Therefore, the resize should continue without interruption. > > > > Thanks. > > > you mean it's a separate bugfix? Right. Don't break resize when coalescing is not negotiated. Thanks. > > > > > > > > -- > > > > 2.32.0.3.g01195cf9f > > > >
On Thu, Aug 01, 2024 at 02:07:43PM +0800, Heng Qi wrote: > On Wed, 31 Jul 2024 08:46:42 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > On Wed, Jul 31, 2024 at 08:25:23PM +0800, Heng Qi wrote: > > > On Wed, 31 Jul 2024 08:14:43 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > On Wed, Jul 31, 2024 at 08:07:17PM +0800, Heng Qi 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> > > > > > Acked-by: Eugenio Pé rez <eperezma@redhat.com> > > > > > Acked-by: Jason Wang <jasowang@redhat.com> > > > > > --- > > > > > v1->v2: > > > > > - Rephrase the subject. > > > > > - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). > > > > > > > > > > drivers/net/virtio_net.c | 10 ++++++++-- > > > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > > index 0383a3e136d6..2b566d893ea3 100644 > > > > > --- a/drivers/net/virtio_net.c > > > > > +++ b/drivers/net/virtio_net.c > > > > > @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > > > { > > > > > int err; > > > > > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > > > + return -EOPNOTSUPP; > > > > > + > > > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), > > > > > max_usecs, max_packets); > > > > > if (err) > > > > > @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, > > > > > { > > > > > int err; > > > > > > > > > > + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) > > > > > + return -EOPNOTSUPP; > > > > > + > > > > > err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), > > > > > max_usecs, max_packets); > > > > > if (err) > > > > > @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > > > err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, > > > > > vi->intr_coal_tx.max_usecs, > > > > > vi->intr_coal_tx.max_packets); > > > > > - if (err) > > > > > + if (err && err != -EOPNOTSUPP) > > > > > return err; > > > > > } > > > > > > > > > > > > > > > > > So far so good. > > > > > > > > > @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, > > > > > vi->intr_coal_rx.max_usecs, > > > > > vi->intr_coal_rx.max_packets); > > > > > mutex_unlock(&vi->rq[i].dim_lock); > > > > > - if (err) > > > > > + if (err && err != -EOPNOTSUPP) > > > > > return err; > > > > > } > > > > > } > > > > > > > > I don't get this one. If resize is not supported, > > > > > > Here means that the *dim feature* is not supported, not the *resize* feature. > > > > > > > we pretend it was successful? Why? > > > > > > During a resize, if the dim feature is not supported, the driver does not > > > need to try to recover any coalescing values, since the device does not have > > > these parameters. > > > Therefore, the resize should continue without interruption. > > > > > > Thanks. > > > > > > you mean it's a separate bugfix? > > Right. > > Don't break resize when coalescing is not negotiated. > > Thanks. Let's make this a separate patch then, please. > > > > > > > > > > > -- > > > > > 2.32.0.3.g01195cf9f > > > > > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 0383a3e136d6..2b566d893ea3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3658,6 +3658,9 @@ static int virtnet_send_rx_ctrl_coal_vq_cmd(struct virtnet_info *vi, { int err; + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) + return -EOPNOTSUPP; + err = virtnet_send_ctrl_coal_vq_cmd(vi, rxq2vq(queue), max_usecs, max_packets); if (err) @@ -3675,6 +3678,9 @@ static int virtnet_send_tx_ctrl_coal_vq_cmd(struct virtnet_info *vi, { int err; + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) + return -EOPNOTSUPP; + err = virtnet_send_ctrl_coal_vq_cmd(vi, txq2vq(queue), max_usecs, max_packets); if (err) @@ -3743,7 +3749,7 @@ static int virtnet_set_ringparam(struct net_device *dev, err = virtnet_send_tx_ctrl_coal_vq_cmd(vi, i, vi->intr_coal_tx.max_usecs, vi->intr_coal_tx.max_packets); - if (err) + if (err && err != -EOPNOTSUPP) return err; } @@ -3758,7 +3764,7 @@ static int virtnet_set_ringparam(struct net_device *dev, vi->intr_coal_rx.max_usecs, vi->intr_coal_rx.max_packets); mutex_unlock(&vi->rq[i].dim_lock); - if (err) + if (err && err != -EOPNOTSUPP) 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> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com> Acked-by: Eugenio Pé rez <eperezma@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> --- v1->v2: - Rephrase the subject. - Put the feature check inside the virtnet_send_{r,t}x_ctrl_coal_vq_cmd(). drivers/net/virtio_net.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)