Message-ID: <CAL+tcoB0dbRnQUFqV9WEzZx+UxjYTt_yP21951HPvu9Dg9jxeg@mail.gmail.com>
Date: Thu, 4 Sep 2025 11:26:26 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Xin Zhao <jackzxcui1989@....com>
Cc: willemdebruijn.kernel@...il.com, edumazet@...gle.com, ferenc@...es.dev, 
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, horms@...nel.org, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v10 2/2] net: af_packet: Use hrtimer to do the
 retire operation

On Thu, Sep 4, 2025 at 1:07 AM Xin Zhao <jackzxcui1989@....com> wrote:
>
> On Wed, Sep 3, 2025 at 00:42 +0800 Jason Xing <kerneljasonxing@...il.com> wrote:
>
> > One more review from my side is that as to the removal of
> > delete_blk_timer, I'm afraid it deserves a clarification in the commit
> > message.
> >
> > > > -       spin_unlock_bh(&rb_queue->lock);
> > > > -
> > > > -       prb_del_retire_blk_timer(pkc);
> > > > -}
> > > > -
>
> In the description of [PATCH net-next v10 0/2] net: af_packet: optimize retire operation:
>
> Changes in v8:
> - Delete delete_blk_timer field, as suggested by Willem de Bruijn,
>   hrtimer_cancel will check and wait until the timer callback return and ensure
>   enter enter callback again;

I see the reason now :)

Please know that the history changes through versions will finally be
removed, only the official message that will be kept in the git. So
this kind of change, I think, should be clarified officially since
you're removing a structure member. Adding more descriptions will be
helpful to readers in the future. Thank you.

>
> I will also emphasize the removal of delete_blk_timer in the commit message for this 2/2
> commit. The updated commit message for the 2/2 patch is as follows：
>
> Changes in v8:
> - Simplify the logic related to setting timeout.
> - Delete delete_blk_timer field, hrtimer_cancel will check and wait until
>   the timer callback return.
>
>
> > I gradually understand your thought behind this modification. You're
> > trying to move the timer operation out of prb_open_block() and then
> > spread the timer operation into each caller.
> >
> > You probably miss the following call trace:
> > packet_current_rx_frame() -> __packet_lookup_frame_in_block() ->
> > prb_open_block() -> _prb_refresh_rx_retire_blk_timer()
> > ?
> >
> > May I ask why bother introducing so many changes like this instead of
> > leaving it as-is?
>
>
>
>
> Consider the following timing sequence:
> timer   cpu0 (softirq context, hrtimer timeout)                cpu1 (process context)
> 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).
>
> In my previous email, I provided an explanation. As a supplement, I would
> like to reiterate a paragraph from my earlier response to Willem.
> The point is that when the hrtimer is in the enqueued state, you cannot

How about tring hrtimer_is_queued() beforehand?

IIUC, with this patch applied, we will lose the opportunity to refresh
the timer when the lookup function (in the above path I mentioned)
gets called compared to before. If the packet socket tries to look up
a new block and it doesn't update its expiry time, the timer will soon
wake up. Does it sound unreasonable?

Thanks,
Jason

> call interfaces like hrtimer_forward_now. The kernel has a WARN_ON check
> in hrtimer_forward_now for this reason. Similarly, you also cannot call
> interfaces like hrtimer_set_expires. The kernel does not include a WARN_ON
> check in hrtimer_set_expires to avoid increasing the code size, as
> hrtimer_set_expires is an inline function.
>
>
> Thanks
> Xin Zhao
>

Powered by blists - more mailing lists