Re: [PATCH net-next v11] virtio_net: add page_pool support for buffer allocation

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Mar 16, 2026 at 05:27:57PM +0530, Vishwanath Seshagiri wrote:
> On 3/16/26 4:13 PM, Michael S. Tsirkin wrote:
> > On Mon, Mar 16, 2026 at 05:56:20AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Mar 10, 2026 at 11:31:04AM -0700, Vishwanath Seshagiri wrote:
> > > > @@ -5857,7 +5863,7 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
> > > >   	/* In big_packets mode, xdp cannot work, so there is no need to
> > > >   	 * initialize xsk of rq.
> > > >   	 */
> > > > -	if (vi->big_packets && !vi->mergeable_rx_bufs)
> > > > +	if (!vi->rq[qid].page_pool)
> > > >   		return -ENOENT;
> > > >   	if (qid >= vi->curr_queue_pairs)
> > > 
> > > 
> > > 
> > > It seems that a qid that exceeds curr_queue_pairs would previously get
> > > -EINVAL and now gets -ENOENT.
> > 
> > Or maybe this if (qid >= vi->curr_queue_pairs) is dead code?
> > I looked at it some more and I can't find a path where this
> > triggers.
> > 
> > > Maybe reorder the checks:
> > > 
> > >          if (qid >= vi->curr_queue_pairs)
> > >                  return -EINVAL;
> > > 
> > >          /* In big_packets mode, xdp cannot work, so there is no need to
> > >           * initialize xsk of rq.
> > >           */
> > >         if (!vi->rq[qid].page_pool)
> > >                 return -ENOENT;
> > > 
> > > 
> > > Alternatively I think we can completely drop this chunk: we already seem
> > > to init page_pull at all times except for big packets mode, so the
> > > current code is fine I think.
> 
> Yes, I agree qid >= curr_queue_pairs appears to be dead code.
> xsk_reg_pool_at_qid() in the XSK core already validates
> queue_id < max(real_num_rx_queues, real_num_tx_queues) before
> ndo_bpf is called, and real_num_rx_queues == curr_queue_pairs is an
> invariant in virtio_net. Both paths hold rtnl_lock so no race is
> possible. That said, I'll adopt your suggested reorder for v12 to ensure
> vi->rq[qid] isn't accessed before a bounds check, even if the check
> is currently redundant.

I think we can just take this as-is, and do more cleanups on top.
Acked-by: Michael S. Tsirkin <mst@redhat.com>


> > > 
> > > 
> > > -- 
> > > MST
> >