| Message ID | 20240614220422.42733-1-jain.abhinav177@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | virtio_net: Eliminate OOO packets during switching | expand |
On Sat, Jun 15, 2024 at 6:05 AM Abhinav Jain <jain.abhinav177@gmail.com> wrote: > > Disable the network device & turn off carrier before modifying the > number of queue pairs. > Process all the in-flight packets and then turn on carrier, followed > by waking up all the queues on the network device. > > Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com> > --- > drivers/net/virtio_net.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 61a57d134544..d0a655a3b4c6 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3447,7 +3447,6 @@ static void virtnet_get_drvinfo(struct net_device *dev, > > } > > -/* TODO: Eliminate OOO packets during switching */ > static int virtnet_set_channels(struct net_device *dev, > struct ethtool_channels *channels) > { > @@ -3471,6 +3470,15 @@ static int virtnet_set_channels(struct net_device *dev, > if (vi->rq[0].xdp_prog) > return -EINVAL; > > + /* Disable network device to prevent packet processing during > + * the switch. > + */ > + netif_tx_disable(dev); > + netif_carrier_off(dev); Any reason we don't need to synchronize with NAPI here? Thanks > + > + /* Make certain that all in-flight packets are processed. */ > + synchronize_net(); > + > cpus_read_lock(); > err = virtnet_set_queues(vi, queue_pairs); > if (err) { > @@ -3482,7 +3490,12 @@ static int virtnet_set_channels(struct net_device *dev, > > netif_set_real_num_tx_queues(dev, queue_pairs); > netif_set_real_num_rx_queues(dev, queue_pairs); > - err: > + > + /* Restart the network device */ > + netif_carrier_on(dev); > + netif_tx_wake_all_queues(dev); > + > +err: > return err; > } > > -- > 2.34.1 >
On Fri, Jun 14, 2024 at 10:04:22PM +0000, Abhinav Jain wrote: > Disable the network device & turn off carrier before modifying the > number of queue pairs. > Process all the in-flight packets and then turn on carrier, followed > by waking up all the queues on the network device. Did you test that there's a workload with OOO and this patch actually prevents that? > > Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com> > --- > drivers/net/virtio_net.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 61a57d134544..d0a655a3b4c6 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -3447,7 +3447,6 @@ static void virtnet_get_drvinfo(struct net_device *dev, > > } > > -/* TODO: Eliminate OOO packets during switching */ > static int virtnet_set_channels(struct net_device *dev, > struct ethtool_channels *channels) > { > @@ -3471,6 +3470,15 @@ static int virtnet_set_channels(struct net_device *dev, > if (vi->rq[0].xdp_prog) > return -EINVAL; > > + /* Disable network device to prevent packet processing during > + * the switch. > + */ > + netif_tx_disable(dev); > + netif_carrier_off(dev); Won't turning off carrier cause a lot of damage such as changing IP and so on? > + > + /* Make certain that all in-flight packets are processed. */ > + synchronize_net(); > + The comment seems to say what the code does not do. Also, doing this under rtnl is a heavy weight operation. > cpus_read_lock(); > err = virtnet_set_queues(vi, queue_pairs); > if (err) { > @@ -3482,7 +3490,12 @@ static int virtnet_set_channels(struct net_device *dev, > > netif_set_real_num_tx_queues(dev, queue_pairs); > netif_set_real_num_rx_queues(dev, queue_pairs); > - err: > + > + /* Restart the network device */ > + netif_carrier_on(dev); > + netif_tx_wake_all_queues(dev); > + > +err: > return err; > } > Given the result is, presumably, improved performance with less packet loss due to OOO, I'd like to see some actual testing results, hopefully also measuring the effect on CPU load. > -- > 2.34.1
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 61a57d134544..d0a655a3b4c6 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -3447,7 +3447,6 @@ static void virtnet_get_drvinfo(struct net_device *dev, } -/* TODO: Eliminate OOO packets during switching */ static int virtnet_set_channels(struct net_device *dev, struct ethtool_channels *channels) { @@ -3471,6 +3470,15 @@ static int virtnet_set_channels(struct net_device *dev, if (vi->rq[0].xdp_prog) return -EINVAL; + /* Disable network device to prevent packet processing during + * the switch. + */ + netif_tx_disable(dev); + netif_carrier_off(dev); + + /* Make certain that all in-flight packets are processed. */ + synchronize_net(); + cpus_read_lock(); err = virtnet_set_queues(vi, queue_pairs); if (err) { @@ -3482,7 +3490,12 @@ static int virtnet_set_channels(struct net_device *dev, netif_set_real_num_tx_queues(dev, queue_pairs); netif_set_real_num_rx_queues(dev, queue_pairs); - err: + + /* Restart the network device */ + netif_carrier_on(dev); + netif_tx_wake_all_queues(dev); + +err: return err; }
Disable the network device & turn off carrier before modifying the number of queue pairs. Process all the in-flight packets and then turn on carrier, followed by waking up all the queues on the network device. Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com> --- drivers/net/virtio_net.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)