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.
