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?
