Re: [PATCH net-next] virtio-net: switch napi_tx without downing nic

[Heng Qi]

在 2023/12/20 下午10:45, Willem de Bruijn 写道:
Heng Qi wrote:
virtio-net has two ways to switch napi_tx: one is through the
module parameter, and the other is through coalescing parameter
settings (provided that the nic status is down).

Sometimes we face performance regression caused by napi_tx,
then we need to switch napi_tx when debugging. However, the
existing methods are a bit troublesome, such as needing to
reload the driver or turn off the network card. So try to make
this update.

Signed-off-by: Heng Qi <hen...@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
The commit does not explain why it is safe to do so.

virtnet_napi_tx_disable ensures that already scheduled tx napi ends and 
no new tx napi will be scheduled.

Afterwards, if the __netif_tx_lock_bh lock is held, the stack cannot 
send the packet.

Then we can safely toggle the weight to indicate where to clear the buffers.


The tx-napi weights are not really weights: it is a boolean whether
napi is used for transmit cleaning, or whether packets are cleaned
in ndo_start_xmit.

Right.


There certainly are some subtle issues with regard to pausing/waking
queues when switching between modes.

What are "subtle issues" and if there are any, we find them.
So far my test results show it's working fine.


Calling napi_enable/napi_disable without bringing down the device is
allowed. The actually napi.weight field is only updated when neither
napi nor ndo_start_xmit is running.

YES.

So I don't see an immediate issue.

Switching napi_tx requires reloading the driver or downing the nic, 
which is troublesome.
I think it would be better if we could find a better way.

Thanks!



---
  drivers/net/virtio_net.c | 81 ++++++++++++++++++----------------------
  1 file changed, 37 insertions(+), 44 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 10614e9f7cad..12f8e1f9971c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3559,16 +3559,37 @@ static int virtnet_coal_params_supported(struct 
ethtool_coalesce *ec)
        return 0;
  }
  
-static int virtnet_should_update_vq_weight(int dev_flags, int weight,
-                                          int vq_weight, bool *should_update)
+static void virtnet_switch_napi_tx(struct virtnet_info *vi, u32 qstart,
+                                  u32 qend, u32 tx_frames)
  {
-       if (weight ^ vq_weight) {
-               if (dev_flags & IFF_UP)
-                       return -EBUSY;
-               *should_update = true;
-       }
+       struct net_device *dev = vi->dev;
+       int new_weight, cur_weight;
+       struct netdev_queue *txq;
+       struct send_queue *sq;
  
-	return 0;
+       new_weight = tx_frames ? NAPI_POLL_WEIGHT : 0;
+       for (; qstart < qend; qstart++) {
+               sq = &vi->sq[qstart];
+               cur_weight = sq->napi.weight;
+               if (!(new_weight ^ cur_weight))
+                       continue;
+
+               if (!(dev->flags & IFF_UP)) {
+                       sq->napi.weight = new_weight;
+                       continue;
+               }
+
+               if (cur_weight)
+                       virtnet_napi_tx_disable(&sq->napi);
+
+               txq = netdev_get_tx_queue(dev, qstart);
+               __netif_tx_lock_bh(txq);
+               sq->napi.weight = new_weight;
+               __netif_tx_unlock_bh(txq);
+
+               if (!cur_weight)
+                       virtnet_napi_tx_enable(vi, sq->vq, &sq->napi);
+       }
  }
  
  static int virtnet_set_coalesce(struct net_device *dev,
@@ -3577,25 +3598,11 @@ static int virtnet_set_coalesce(struct net_device *dev,
                                struct netlink_ext_ack *extack)
  {
        struct virtnet_info *vi = netdev_priv(dev);
-       int ret, queue_number, 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;
-       for (queue_number = 0; queue_number < vi->max_queue_pairs; 
queue_number++) {
-               ret = virtnet_should_update_vq_weight(dev->flags, napi_weight,
-                                                     
vi->sq[queue_number].napi.weight,
-                                                     &update_napi);
-               if (ret)
-                       return ret;
-
-               if (update_napi) {
-                       /* All queues that belong to [queue_number, 
vi->max_queue_pairs] will be
-                        * updated for the sake of simplicity, which might not 
be necessary
-                        */
-                       break;
-               }
-       }
+       int ret;
+
+       /* Param tx_frames can be used to switch napi_tx */
+       virtnet_switch_napi_tx(vi, 0, vi->max_queue_pairs,
+                              ec->tx_max_coalesced_frames);
  
  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL))
                ret = virtnet_send_notf_coal_cmds(vi, ec);
@@ -3605,11 +3612,6 @@ static int virtnet_set_coalesce(struct net_device *dev,
        if (ret)
                return ret;
  
-	if (update_napi) {
-               for (; queue_number < vi->max_queue_pairs; queue_number++)
-                       vi->sq[queue_number].napi.weight = napi_weight;
-       }
-
        return ret;
  }
  
@@ -3641,19 +3643,13 @@ static int virtnet_set_per_queue_coalesce(struct net_device *dev,
                                          struct ethtool_coalesce *ec)
  {
        struct virtnet_info *vi = netdev_priv(dev);
-       int ret, napi_weight;
-       bool update_napi = false;
+       int ret;
  
  	if (queue >= vi->max_queue_pairs)
                return -EINVAL;
  
-	/* Can't change NAPI weight if the link is up */
-       napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
-       ret = virtnet_should_update_vq_weight(dev->flags, napi_weight,
-                                             vi->sq[queue].napi.weight,
-                                             &update_napi);
-       if (ret)
-               return ret;
+       /* Param tx_frames can be used to switch napi_tx */
+       virtnet_switch_napi_tx(vi, queue, queue, ec->tx_max_coalesced_frames);
  
  	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
                ret = virtnet_send_notf_coal_vq_cmds(vi, ec, queue);
@@ -3663,9 +3659,6 @@ static int virtnet_set_per_queue_coalesce(struct 
net_device *dev,
        if (ret)
                return ret;
  
-	if (update_napi)
-               vi->sq[queue].napi.weight = napi_weight;
-
        return 0;
  }
  
--
2.19.1.6.gb485710b