Message-Id: <20250825050628.124977-1-jackzxcui1989@163.com>
Date: Mon, 25 Aug 2025 13:06: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 v6] net: af_packet: Use hrtimer to do the retire operation

On Mon, 2025-08-25 at 2:08 +0800, Willem wrote:

> This is getting more complex than needed.
> 
> Essentially the lifecycle is that packet_set_ring calls hrtimer_setup
> and hrtimer_del_sync.
> 
> Inbetween, while the ring is configured, the timer is either
> 
> - scheduled from tpacket_rcv and !is_scheduled
>     -> call hrtimer_start
> - scheduled from tpacket_rcv and is_scheduled
>     -> call hrtimer_set_expires

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.

static int perf_mux_hrtimer_restart(struct perf_cpu_pmu_context *cpc)
{
	struct hrtimer *timer = &cpc->hrtimer;
	unsigned long flags;

	raw_spin_lock_irqsave(&cpc->hrtimer_lock, flags);
	if (!cpc->hrtimer_active) {
		cpc->hrtimer_active = 1;
		hrtimer_forward_now(timer, cpc->hrtimer_interval);
		hrtimer_start_expires(timer, HRTIMER_MODE_ABS_PINNED_HARD);
	}
	raw_spin_unlock_irqrestore(&cpc->hrtimer_lock, flags);

	return 0;
}

Therefore, according to the overall design of the hrtimer, once the
hrtimer is active, it is not allowed to set the timeout outside of the
hrtimer callback nor is it allowed to restart the hrtimer.

So two ways to update the hrtimer timeout:
1. update expire time in the callback
2. Call the hrtimer_cancel and then call hrtimer_start
According to your suggestion, we don't call hrtimer_start inside the
callback, would you accept calling hrtimer_cancel first and then calling
hrtimer_start in the callback? However, this approach also requires
attention, as hrtimer_cancel will block until the callback is running,
so it is essential to ensure that it is not called within the hrtimer
callback; otherwise, it could lead to a deadlock.


> - rescheduled from the timer callback
>     -> call hrtimer_set_expires and return HRTIMER_RESTART
> 
> The only complication is that the is_scheduled check can race with the
> HRTIMER_RESTART restart, as that happens outside the sk_receive_queue
> critical section.
> 
> One option that I suggested before is to convert pkc->delete_blk_timer
> to pkc->blk_timer_scheduled to record whether the timer is scheduled
> without relying on hrtimer_is_queued. Set it on first open_block and
> clear it from the callback when returning HR_NORESTART.

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.


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.
4. To avoid the potential issue of the enqueue in step 2 and the
hrtimer_start in step 3 happening simultaneously, which could lead to
hrtimer_start being triggered twice in a very short period, the logic should
be:
if (hrtimer_cancel(...))
    hrtimer_start(...);
Additionally, the hrtimer_cancel check will also avoid hrtimer callback
triggered once more when just called prb_del_retire_blk_timer by packet_set_ring.
The hrtimer should be in an active state beginning from when
prb_setup_retire_blk_timer is called to the time when prb_del_retire_blk_timer
is called.


Thanks
Xin Zhao

Powered by blists - more mailing lists