Message-ID: <willemdebruijn.kernel.26d6abeee5c4c@gmail.com>
Date: Mon, 25 Aug 2025 12:20:17 -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 v6] net: af_packet: Use hrtimer to do the retire
 operation

Xin Zhao wrote:
> 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.  

Perhaps we need to simplify and stop trying to adjust the timer from
tpacket_rcv once scheduled. Let the callback handle that.

> 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

1 seems preferable. The intent of the API.

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

Yes, no objections to that if necessary.
> 
> 
> 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.

> 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