Re: [PATCH v2 1/4] net: ntb_netdev: Introduce per-queue context

[Koichiro Den]

On Wed, Mar 04, 2026 at 05:13:37PM -0800, Jakub Kicinski wrote:
> On Sat, 28 Feb 2026 23:55:35 +0900 Koichiro Den wrote:
> > @@ -99,7 +114,9 @@ static void ntb_netdev_event_handler(void *data, int link_is_up)
> >  static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
> >  				  void *data, int len)
> >  {
> > -	struct net_device *ndev = qp_data;
> > +	struct ntb_netdev_queue *q = qp_data;
> > +	struct ntb_netdev *dev = q->ntdev;
> > +	struct net_device *ndev = dev->ndev;
> >  	struct sk_buff *skb;
> >  	int rc;
> >  
> > @@ -118,6 +135,7 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
> >  	skb_put(skb, len);
> >  	skb->protocol = eth_type_trans(skb, ndev);
> >  	skb->ip_summed = CHECKSUM_NONE;
> > +	skb_record_rx_queue(skb, q->qid);
> >  
> >  	if (netif_rx(skb) == NET_RX_DROP) {
> >  		ndev->stats.rx_errors++;
> > @@ -135,7 +153,8 @@ static void ntb_netdev_rx_handler(struct ntb_transport_qp *qp, void *qp_data,
> >  	}
> >  
> >  enqueue_again:
> > -	rc = ntb_transport_rx_enqueue(qp, skb, skb->data, ndev->mtu + ETH_HLEN);
> > +	rc = ntb_transport_rx_enqueue(q->qp, skb, skb->data,
> 
> nit: I think in this case you don't have to replace qp qith q->qp ?
> qp is an argument of ntb_netdev_rx_handler()
> 
> > @@ -208,16 +227,26 @@ static netdev_tx_t ntb_netdev_start_xmit(struct sk_buff *skb,
> >  					 struct net_device *ndev)
> >  {
> >  	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	u16 qid = skb_get_queue_mapping(skb);
> > +	struct ntb_netdev_queue *q;
> >  	int rc;
> >  
> > -	ntb_netdev_maybe_stop_tx(ndev, dev->qp, tx_stop);
> > +	if (unlikely(!dev->num_queues))
> 
> please avoid such defensive checks, this should never happen
> 
> > +		goto err;
> >  
> > -	rc = ntb_transport_tx_enqueue(dev->qp, skb, skb->data, skb->len);
> > +	if (unlikely(qid >= dev->num_queues))
> 
> same, the stack should not sent patches for disabled queues
> 
> > +		qid = 0;
> > +
> 
> >  static void ntb_netdev_tx_timer(struct timer_list *t)
> >  {
> > -	struct ntb_netdev *dev = timer_container_of(dev, t, tx_timer);
> > +	struct ntb_netdev_queue *q = timer_container_of(q, t, tx_timer);
> > +	struct ntb_netdev *dev = q->ntdev;
> >  	struct net_device *ndev = dev->ndev;
> 
> nit: if you can't maintain the longest to shortest order because of
> dependencies the init should be moved out-of-line
> 
> >  static int ntb_netdev_open(struct net_device *ndev)
> >  {
> >  	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	struct ntb_netdev_queue *queue;
> >  	struct sk_buff *skb;
> > -	int rc, i, len;
> > +	unsigned int q;
> > +	int rc = 0, i, len;
> 
> nit: sort variable declaration lines longest to shortest (there's at
> least one more of such issues in this patch)
> 
> > @@ -420,6 +485,16 @@ static int ntb_netdev_probe(struct device *client_dev)
> >  	dev = netdev_priv(ndev);
> >  	dev->ndev = ndev;
> >  	dev->pdev = pdev;
> > +	dev->client_dev = client_dev;
> > +	dev->num_queues = 0;
> > +
> > +	dev->queues = kcalloc(NTB_NETDEV_MAX_QUEUES, sizeof(*dev->queues),
> > +			      GFP_KERNEL);
> 
> I think we may be expected to use kzalloc_objs() now
> 
> 
> > @@ -464,9 +560,14 @@ static void ntb_netdev_remove(struct device *client_dev)
> >  {
> >  	struct net_device *ndev = dev_get_drvdata(client_dev);
> >  	struct ntb_netdev *dev = netdev_priv(ndev);
> > +	unsigned int q;
> > +
> >  
> 
> nit double new line

Thanks for the review. All you pointed out makes sense. I've addressed these
locally and re-tested the series. I'll send v3 later.

Best regards,
Koichiro

> -- 
> pw-bot: cr