Message-ID: <willemdebruijn.kernel.ade9d72d060e@gmail.com>
Date: Tue, 26 Aug 2025 08:54:46 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Xin Zhao <jackzxcui1989@....com>, 
 willemdebruijn.kernel@...il.com, 
 edumazet@...gle.com, 
 ferenc@...es.dev
Cc: 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 v7] net: af_packet: Use hrtimer to do the retire
 operation

Xin Zhao wrote:
> On Tue, 2025-08-25 at 0:20 +0800, Willem wrote:
> 
> > > We cannot use hrtimer_set_expires/hrtimer_forward_now when a hrtimer is
> > > already enqueued.  
> > > The WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED) in hrtimer_forward
> > > already clearly indicates this point. The reason for not adding this
> > > WARN_ON in hrtimer_set_expires is that hrtimer_set_expires is an inline
> > > function, wory about increase code size.
> > > The implementation of perf_mux_hrtimer_restart actually checks whether
> > > the hrtimer is active when restarting the hrtimer.
> > 
> > Perhaps we need to simplify and stop trying to adjust the timer from
> > tpacket_rcv once scheduled. Let the callback handle that.
> > 
> 
> Okay, I would also like to modify the timeout only within the callback,
> so I think PATCH v7 might be a better solution. Additionally, in terms of
> performance, it should be more efficient than frequently calling
> hrtimer_cancel/hrtimer_start functions to change the timeout outside the
> callback.
> 
> Why do I add the pkc->expire_ktime in PATCH v7?
> 
> For example 8ms retire timeout.
> T means the time callback/tpacket_rcv call _prb_refresh_rx_retire_blk_timer.
> T1 means time T plus 1ms, T2 means time T plus 2ms...
> 
> timeline: past -----------> -----------> -----------> future
> callback:      T	           T8
> tpacket_rcv:                 T7
> 
> Considering the situation in the above diagram, at time T7, the tpacket_rcv
> function processes the network and finds that a new block needs to be opened,
> which requires setting a timeout of T7 + 8ms which is T15ms. However, we
> cannot directly set the timeout within tpacket_rcv, so we use a variable
> expire_ktime to record this value. At time T8, in the hrtimer callback, we
> check that expire_ktime which is T15 is greater than the current timeout of
> the hrtimer, which is T8. Therefore, we simply return from the hrtimer
> callback at T8, the next execution time of the hrtimer callback will be T15.
> This achieves the same effect as executing hrtimer_start in tpacket_rcv
> using a "one shot" approach.
> 
> 
> > > Do you agree with adding a callback variable to distinguish between
> > > scheduled from tpacket_rcv and scheduled from the callback? I really
> > > couldn't think of a better solution.
> > 
> > Yes, no objections to that if necessary.
> 
> So it seems that the logic of 'adding a callback variable to distinguish' in 
> PATCH v7 is OK?
> 
> 
> > > So, a possible solution may be?
> > > 1. Continue to keep the callback parameter to strictly ensure whether it
> > > is within the callback.
> > > 2. Use hrtimer_set_expires within the callback to update the timeout (the
> > > hrtimer module will enqueue the hrtimer when callback return)
> > > 3. If it is not in callback, call hrtimer_cancel + hrtimer_start to restart
> > > the timer.
> >
> > Instead, I would use an in_scheduled param, as in my previous reply and
> > simply skip trying to schedule if already scheduled.
> 
> I understand that the additional in_scheduled variable is meant to prevent
> multiple calls to hrtimer_start. However, based on the current logic
> implementation, the only scenario that would cancel the hrtimer is after calling
> prb_shutdown_retire_blk_timer. Therefore, once we have called hrtimer_start in
> prb_setup_retire_blk_timer, we don't need to worry about the hrtimer stopping,
> and we don't need to execute hrtimer_start again or check if the hrtimer is in
> an active state. We can simply update the timeout in the callback.

The hrtimer is also canceled when the callback returns
HRTIMER_NORESTART.

> Additionally, we don't need to worry about the situation where packet_set_ring
> is entered twice, leading to multiple calls to hrtimer_start, because there is
> a check for pg_vec before executing init_prb_bdqc in packet_set_ring. If pg_vec
> is non-zero, it will go to the out label.
> 
> So is PATCH v7 good to go? Besides I think that ktime_after should be used
> instead of ktime_compare, I haven't noticed any other areas in PATCH v7 that
> need modification. What do you think?
> 
> 
> Thanks
> Xin Zhao
> 

Powered by blists - more mailing lists