Re: [PATCH net-next v6] net: af_packet: Use hrtimer to do the retire operation

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Xin Zhao wrote:
> On Thu, 2025-08-21 at 22:32 +0800, Willem wrote:
> 
> > Thanks for the analysis.
> > 
> > Using hrtimer_start from within the callback that returns
> > HRTIMER_RESTART does not sound in line with the intention of the API
> > to me.
> > 
> > I think we should just adjust and restart from within the callback and
> > hrtimer_start from tpacket_rcv iff the timer is not yet queued.
> > 
> > Since all these modifications are made while the receive queue lock is
> > held I don't immediately see why we would need additional mutual
> > exclusion beyond that.
> 
> 
> The hrtimer callback is called by __run_hrtimer, if we only use hrtimer_forward_now in the callback,
> it will not restart the time within the callback. The timer will be enqueued after the callback
> return. So when the timer is being enqueued, it is not protected by sk_receive_queue.lock.

I see.
 
> Consider the following timing sequence:
> timer   cpu0 (softirq context, hrtimer timeout)                cpu
> 0       hrtimer_run_softirq
> 1         __hrtimer_run_queues
> 2           __run_hrtimer
> 3             prb_retire_rx_blk_timer_expired
> 4               spin_lock(&po->sk.sk_receive_queue.lock);
> 5               _prb_refresh_rx_retire_blk_timer
> 6                 hrtimer_forward_now
> 7               spin_unlock(&po->sk.sk_receive_queue.lock)
> 8             raw_spin_lock_irq(&cpu_base->lock);              tpacket_rcv
> 9             enqueue_hrtimer                                    spin_lock(&sk->sk_receive_queue.lock);
> 10                                                               packet_current_rx_frame
> 11                                                                 __packet_lookup_frame_in_block
> 12            finish enqueue_hrtimer                                 prb_open_block
> 13                                                                     _prb_refresh_rx_retire_blk_timer
> 14                                                                       hrtimer_is_queued(&pkc->retire_blk_timer) == true
> 15                                                                       hrtimer_forward_now
> 16                                                                         WARN_ON
> 
> On cpu0 in the timing sequence above, enqueue_hrtimer is not protected by sk_receive_queue.lock,
> while the hrtimer_forward_now is not protected by raw_spin_lock_irq(&cpu_base->lock).
> 
> It will cause WARN_ON if we only use 'hrtimer_is_queued(&pkc->retire_blk_timer) == true' to check
> whether to call hrtimer_forward_now.

One way around this may be to keep the is_timer_queued state inside
tpacket_kbdq_core protected by a relevant lock, like the receive queue
lock. Similar to pkc->delete_blk_timer.

Admittedly I have not given this much thought yet. Am traveling for a
few days, have limited time.
 
> 
> Thanks
> Xin Zhao
>