Re: [PATCH net-next v2] netdevsim: implement peer queue flow control

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 9 Jul 2025 03:34:20 -0700 Breno Leitao wrote:

> On Tue, Jul 08, 2025 at 06:27:18PM -0700, Jakub Kicinski wrote:
> > On Thu, 03 Jul 2025 06:09:31 -0700 Breno Leitao wrote:  
> > > +static int nsim_napi_rx(struct net_device *dev, struct nsim_rq *rq,
> > > +			struct sk_buff *skb)
> > >  {
> > >  	if (skb_queue_len(&rq->skb_queue) > NSIM_RING_SIZE) {
> > > +		nsim_stop_peer_tx_queue(dev, rq, skb_get_queue_mapping(skb));
> > >  		dev_kfree_skb_any(skb);
> > >  		return NET_RX_DROP;
> > >  	}  
> > 
> > we should probably add:
> > 
> > 	if (skb_queue_len(&rq->skb_queue) > NSIM_RING_SIZE)
> > 		nsim_stop_tx_queue(dev, rq, skb_get_queue_mapping(skb));
> > 
> > after enqueuing the skb, so that we stop the queue before any drops
> > happen  
> 
> Agree, we can stop the queue when queueing the packets instead. Since we
> need to check for the queue numbers, we cannot call nsim_stop_tx_queue()
> straight away. I think we still need to have a helper
> (nsim_stop_tx_queue). This is what I have in mind:

LGTM!

> > > +	if (dev->real_num_tx_queues != peer_dev->num_rx_queues)  
> > 
> > given that we compare real_num_tx_queues I think we should also kick
> > the queues in nsim_set_channels(), like we do in unlink_device_store()  
> 
> Sure. I suppose something like the following. What do you think?
> 
> 	nsim_set_channels(struct net_device *dev, struct ethtool_channels *ch)
> 	{
> 		struct netdevsim *ns = netdev_priv(dev);
> 	+       struct netdevsim *peer;
> 		int err;
> 
> 		err = netif_set_real_num_queues(dev, ch->combined_count,
> 	@@ -113,6 +114,14 @@ nsim_set_channels(struct net_device *dev, struct ethtool_channels *ch)
> 			return err;
> 
> 		ns->ethtool.channels = ch->combined_count;
> 	+
> 	+	synchronize_net();
> 	+       netif_tx_wake_all_queues(dev);
> 	+       rcu_read_lock();
> 	+       peer = rcu_dereference(ns->peer);
> 	+       if (peer)
> 	+               netif_tx_wake_all_queues(peer->netdev);
> 	+       rcu_read_unlock();

That's sufficiently orthogonal to warrant a dedicated function / helper.

In terms of code I think we can skip the whole dance if peer is NULL?

> 		return 0;
> 	}
> 
> 
> Also, with this patch, we will eventually get the following critical
> message:
> 
> 	net_crit_ratelimited("Virtual device %s asks to queue packet!\n", dev->name);
> 
> I am wondering if that alert is not valid anymore, and I can simply
> remove it.

Ah. In nsim_setup() we should remove IFF_NO_QUEUE and stop setting
tx_queue_len to 0