| Message ID | 20230605210237.60988-1-brett.creeley@amd.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net] virtio_net: Prevent napi_weight changes with VIRTIO_NET_F_NOTF_COAL support | expand |
On Mon, 5 Jun 2023 14:02:36 -0700, Brett Creeley <brett.creeley@amd.com> wrote: > Commit 699b045a8e43 ("net: virtio_net: notifications coalescing > support") added support for VIRTIO_NET_F_NOTF_COAL. The get_coalesce > call made changes to report "1" in tx_max_coalesced_frames if > VIRTIO_NET_F_NOTF_COAL is not supported and napi.weight is non-zero. > However, the napi_weight value could still be changed by the > set_coalesce call regardless of whether or not the device supports > VIRTIO_NET_F_NOTF_COAL. > > It seems like the tx_max_coalesced_frames value should not control more > than 1 thing (i.e. napi_weight and the device's tx_max_packets). So, fix > this by only allowing the napi_weight change if VIRTIO_NET_F_NOTF_COAL > is not supported by the virtio device. @Jason I wonder should we keep this function to change the napi weight by the coalesec command. Thanks. > > It wasn't clear to me if this was the intended behavior, so that's why > I'm sending this as an RFC patch initially. Based on the feedback, I > will resubmit as an official patch. > > Fixes: 699b045a8e43 ("net: virtio_net: notifications coalescing support") > Signed-off-by: Brett Creeley <brett.creeley@amd.com> > --- > drivers/net/virtio_net.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 486b5849033d..e28387866909 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2990,19 +2990,21 @@ static int virtnet_set_coalesce(struct net_device *dev, > int ret, i, napi_weight; > bool update_napi = false; > > - /* Can't change NAPI weight if the link is up */ > - napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0; > - if (napi_weight ^ vi->sq[0].napi.weight) { > - if (dev->flags & IFF_UP) > - return -EBUSY; > - else > - update_napi = true; > - } > - > - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) { > ret = virtnet_send_notf_coal_cmds(vi, ec); > - else > + } else { > + /* Can't change NAPI weight if the link is up */ > + napi_weight = ec->tx_max_coalesced_frames ? > + NAPI_POLL_WEIGHT : 0; > + if (napi_weight ^ vi->sq[0].napi.weight) { > + if (dev->flags & IFF_UP) > + return -EBUSY; > + else > + update_napi = true; > + } > + > ret = virtnet_coal_params_supported(ec); > + } > > if (ret) > return ret; > -- > 2.17.1 >
On Tue, Jun 6, 2023 at 5:03 AM Brett Creeley <brett.creeley@amd.com> wrote: > > Commit 699b045a8e43 ("net: virtio_net: notifications coalescing > support") added support for VIRTIO_NET_F_NOTF_COAL. The get_coalesce > call made changes to report "1" in tx_max_coalesced_frames if > VIRTIO_NET_F_NOTF_COAL is not supported and napi.weight is non-zero. > However, the napi_weight value could still be changed by the > set_coalesce call regardless of whether or not the device supports > VIRTIO_NET_F_NOTF_COAL. > > It seems like the tx_max_coalesced_frames value should not control more > than 1 thing (i.e. napi_weight and the device's tx_max_packets). So, fix > this by only allowing the napi_weight change if VIRTIO_NET_F_NOTF_COAL > is not supported by the virtio device. > > It wasn't clear to me if this was the intended behavior, so that's why > I'm sending this as an RFC patch initially. Based on the feedback, I > will resubmit as an official patch. It seems the current code is fine since: Before tx coalescing, we have two modes for tx interrupt: 1) TX NAPI mode, using NAPI to recycle xmit packets 2) TX no-NAPI mode, depends on the start_xmit() to recycle xmit packets Each has their own use cases. E.g 1) seems to have better buffer interaction with TCP. But 2) seems to behave better if user cares about PPS and it can gives us 2x PPS when using a vhost-user backend. So we leave an option to switch between those two via sq.napi_weight ethtool -C tx-frames-irq 0 // To disable tx interrupts ethtool -C tx-frames-irq 1 // To enable tx interrupts After tx intr coleasing, we want to stick to this API. ethtool -C tx-frames-irq 0 // To disable tx interrupts ethtool -C tx-frames-irq N (N>=1) // To enable tx interrupts Thanks > > Fixes: 699b045a8e43 ("net: virtio_net: notifications coalescing support") > Signed-off-by: Brett Creeley <brett.creeley@amd.com> > --- > drivers/net/virtio_net.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 486b5849033d..e28387866909 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2990,19 +2990,21 @@ static int virtnet_set_coalesce(struct net_device *dev, > int ret, i, napi_weight; > bool update_napi = false; > > - /* Can't change NAPI weight if the link is up */ > - napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0; > - if (napi_weight ^ vi->sq[0].napi.weight) { > - if (dev->flags & IFF_UP) > - return -EBUSY; > - else > - update_napi = true; > - } > - > - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) { > ret = virtnet_send_notf_coal_cmds(vi, ec); > - else > + } else { > + /* Can't change NAPI weight if the link is up */ > + napi_weight = ec->tx_max_coalesced_frames ? > + NAPI_POLL_WEIGHT : 0; > + if (napi_weight ^ vi->sq[0].napi.weight) { > + if (dev->flags & IFF_UP) > + return -EBUSY; > + else > + update_napi = true; > + } > + > ret = virtnet_coal_params_supported(ec); > + } > > if (ret) > return ret; > -- > 2.17.1 >
On Tue, Jun 6, 2023 at 9:57 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Mon, 5 Jun 2023 14:02:36 -0700, Brett Creeley <brett.creeley@amd.com> wrote: > > Commit 699b045a8e43 ("net: virtio_net: notifications coalescing > > support") added support for VIRTIO_NET_F_NOTF_COAL. The get_coalesce > > call made changes to report "1" in tx_max_coalesced_frames if > > VIRTIO_NET_F_NOTF_COAL is not supported and napi.weight is non-zero. > > However, the napi_weight value could still be changed by the > > set_coalesce call regardless of whether or not the device supports > > VIRTIO_NET_F_NOTF_COAL. > > > > It seems like the tx_max_coalesced_frames value should not control more > > than 1 thing (i.e. napi_weight and the device's tx_max_packets). So, fix > > this by only allowing the napi_weight change if VIRTIO_NET_F_NOTF_COAL > > is not supported by the virtio device. > > > @Jason I wonder should we keep this function to change the napi weight by the > coalesec command. I think so, explained in another thread. Thanks > > Thanks. > > > > > It wasn't clear to me if this was the intended behavior, so that's why > > I'm sending this as an RFC patch initially. Based on the feedback, I > > will resubmit as an official patch. > > > > Fixes: 699b045a8e43 ("net: virtio_net: notifications coalescing support") > > Signed-off-by: Brett Creeley <brett.creeley@amd.com> > > --- > > drivers/net/virtio_net.c | 24 +++++++++++++----------- > > 1 file changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index 486b5849033d..e28387866909 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -2990,19 +2990,21 @@ static int virtnet_set_coalesce(struct net_device *dev, > > int ret, i, napi_weight; > > bool update_napi = false; > > > > - /* Can't change NAPI weight if the link is up */ > > - napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0; > > - if (napi_weight ^ vi->sq[0].napi.weight) { > > - if (dev->flags & IFF_UP) > > - return -EBUSY; > > - else > > - update_napi = true; > > - } > > - > > - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) { > > ret = virtnet_send_notf_coal_cmds(vi, ec); > > - else > > + } else { > > + /* Can't change NAPI weight if the link is up */ > > + napi_weight = ec->tx_max_coalesced_frames ? > > + NAPI_POLL_WEIGHT : 0; > > + if (napi_weight ^ vi->sq[0].napi.weight) { > > + if (dev->flags & IFF_UP) > > + return -EBUSY; > > + else > > + update_napi = true; > > + } > > + > > ret = virtnet_coal_params_supported(ec); > > + } > > > > if (ret) > > return ret; > > -- > > 2.17.1 > > >
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 486b5849033d..e28387866909 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2990,19 +2990,21 @@ static int virtnet_set_coalesce(struct net_device *dev, int ret, i, napi_weight; bool update_napi = false; - /* Can't change NAPI weight if the link is up */ - napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0; - if (napi_weight ^ vi->sq[0].napi.weight) { - if (dev->flags & IFF_UP) - return -EBUSY; - else - update_napi = true; - } - - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) { ret = virtnet_send_notf_coal_cmds(vi, ec); - else + } else { + /* Can't change NAPI weight if the link is up */ + napi_weight = ec->tx_max_coalesced_frames ? + NAPI_POLL_WEIGHT : 0; + if (napi_weight ^ vi->sq[0].napi.weight) { + if (dev->flags & IFF_UP) + return -EBUSY; + else + update_napi = true; + } + ret = virtnet_coal_params_supported(ec); + } if (ret) return ret;
Commit 699b045a8e43 ("net: virtio_net: notifications coalescing support") added support for VIRTIO_NET_F_NOTF_COAL. The get_coalesce call made changes to report "1" in tx_max_coalesced_frames if VIRTIO_NET_F_NOTF_COAL is not supported and napi.weight is non-zero. However, the napi_weight value could still be changed by the set_coalesce call regardless of whether or not the device supports VIRTIO_NET_F_NOTF_COAL. It seems like the tx_max_coalesced_frames value should not control more than 1 thing (i.e. napi_weight and the device's tx_max_packets). So, fix this by only allowing the napi_weight change if VIRTIO_NET_F_NOTF_COAL is not supported by the virtio device. It wasn't clear to me if this was the intended behavior, so that's why I'm sending this as an RFC patch initially. Based on the feedback, I will resubmit as an official patch. Fixes: 699b045a8e43 ("net: virtio_net: notifications coalescing support") Signed-off-by: Brett Creeley <brett.creeley@amd.com> --- drivers/net/virtio_net.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)