Message-Id: <20250826030328.878001-1-jackzxcui1989@163.com>
Date: Tue, 26 Aug 2025 11:03:28 +0800
From: Xin Zhao <jackzxcui1989@....com>
To: 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

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.
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