Re: [PATCH net-next v11 1/4] tun/tap: add ptr_ring consume helper with netdev queue wakeup

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

On Sun, May 10, 2026 at 06:22:15PM +0200, Simon Schippers wrote:
> On 5/10/26 17:44, Michael S. Tsirkin wrote:
> > On Sun, May 10, 2026 at 04:01:39PM +0200, Simon Schippers wrote:
> >> On 5/10/26 15:40, Michael S. Tsirkin wrote:
> >>> On Sun, May 10, 2026 at 10:55:34AM +0200, Simon Schippers wrote:
> >>>> On 5/10/26 09:03, Simon Schippers wrote:
> >>>>> On 5/10/26 00:44, Michael S. Tsirkin wrote:
> >>>>>> On Sat, May 09, 2026 at 06:31:47PM +0200, Simon Schippers wrote:
> >>>>>>> On 5/8/26 17:10, Simon Schippers wrote:
> >>>>>>>> +static void tun_queue_purge(struct tun_struct *tun, struct tun_file *tfile)
> >>>>>>>>  {
> >>>>>>>>  	void *ptr;
> >>>>>>>>  
> >>>>>>>> -	while ((ptr = ptr_ring_consume(&tfile->tx_ring)) != NULL)
> >>>>>>>> +	while ((ptr = tun_ring_consume(tun, tfile)) != NULL)
> >>>>>>>>  		tun_ptr_free(ptr);
> >>>>>>>>  
> >>>>>>>>  	skb_queue_purge(&tfile->sk.sk_write_queue);
> >>>>>>>
> >>>>>>> Sashiko is right once again. tun_ring_consume() in tun_queue_purge()
> >>>>>>> operates on a tfile that is being torn down. Its queue_index is no
> >>>>>>> longer valid. After the swap in __tun_detach(), it points to the
> >>>>>>> netdev subqueue of a different tfile.
> >>>>>>> --> We should not wake there.
> >>>>>>
> >>>>>> Does it not exactly point at ntfile which is what we want to wake?
> >>>>>>
> >>>>>
> >>>>> I see your point. But calling tun_ring_consume() as done here is
> >>>>> wrong, because it does not wake if the tx_ring of the tfile 
> >>>>> (that is currently torn down) is empty. We could change
> >>>>> tun_ring_consume() to call __tun_wake_queue()
> >>>>> with consumed=0 if !ptr but I think this would slow down the consumer
> >>>>> path.
> >>>>>
> >>>>
> >>>> My statement is wrong:
> >>>> There is no way that the tx_ring is empty and the queue is stopped
> >>>> at the same time. So we do not need to touch tun_ring_consume() and
> >>>> this works just fine.
> >>>>
> >>>>>>
> >>>>>>> I will swap tun_ring_consume() with ptr_ring_consume() again and
> >>>>>>> submit a v12 :)
> >>>>>>
> >>>>>> If so then maybe
> >>>>>> netif_tx_wake_queue(netdev_get_tx_queue(tun->dev, index));
> >>>>>>
> >>>>>
> >>>>> But we should only do this if there is space in the ntfile.
> >>>>> My approach:
> >>>>>
> >>>>> @@ -586,12 +588,18 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
> >>>>>  		BUG_ON(index >= tun->numqueues);
> >>>>>  
> >>>>>  		rcu_assign_pointer(tun->tfiles[index],
> >>>>>  				   tun->tfiles[tun->numqueues - 1]);
> >>>>>  		ntfile = rtnl_dereference(tun->tfiles[index]);
> >>>>> +		spin_lock(&ntfile->tx_ring.consumer_lock);
> >>>>>  		ntfile->queue_index = index;
> >>>>>  		ntfile->xdp_rxq.queue_index = index;
> >>>>> +		ntfile->cons_cnt = 0;
> >>>>> +		if (__ptr_ring_empty(&ntfile->tx_ring)) {
> >>>>> +			netif_wake_subqueue(tun->dev, index);
> >>>>> +		}
> >>>>> +		spin_unlock(&ntfile->tx_ring.consumer_lock);
> >>>>>  		rcu_assign_pointer(tun->tfiles[tun->numqueues - 1],
> >>>>>  				   NULL);
> >>>>>
> >>>>> ntfile->cons_cnt is unvalid, because the new queue might not be stopped.
> >>>>> That is the reason why I reset it to 0.
> >>>>
> >>>> However, I still prefer this approach because the code is easier to
> >>>> understand.
> >>>
> >>>
> >>> So do you want me to finish review of this one and ack, or want to
> >>> post v12?
> >>>
> >>
> >> I will post a v12 with the proposed changes for patch 1.
> >> No other changes.
> >>
> >> Thanks!
> > 
> > actually can you clarify? why only when ntfile ring is empty?
> > 
> 
> This avoids waking when ntfile->tx_ring is full. We can not use 
> __ptr_ring_can_produce() with consumer locks, therefore I chose
> __ptr_ring_empty() instead.
> 
> If there are any elements in ntfile->tx_ring we do not have to wake.
> This will be done by the consumer in tun_ring_consume() &
> __tun_wake_queue() after consuming those elements.

worth a code comment if you need to do v13.