Re: [PATCH net-next v4] net: af_packet: Use hrtimer to do the retire operation

On Mon, 2025-08-18 at 17:29 +0800, Willem wrote:

"We" don't do anything in the middle of a computation. Anyway, branch is
self explanatory enough, can drop comment.

quoted

  */
-static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc)
+static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc,
+		bool start)

Indentation, align with first argument on previous line

quoted

+	else
+		/* We cannot use hrtimer_forward_now here because the function
+		 * _prb_refresh_rx_retire_blk_timer can be called not only when
+		 * the retire timer expires, but also when the kernel logic for
+		 * receiving network packets detects that a network packet has
+		 * filled up a block and calls prb_open_block to use the next
+		 * block. This can lead to a WARN_ON being triggered in
+		 * hrtimer_forward_now when it checks if the timer has already
+		 * been enqueued.
+		 */

As discussed, this will be changed in v5.

I will change them in v5. And I will ensure that there is a 24-hour send gap between
each patch.

quoted

 {
-	mod_timer(&pkc->retire_blk_timer,
-			jiffies + pkc->tov_in_jiffies);
+	if (start)
+		hrtimer_start(&pkc->retire_blk_timer, pkc->interval_ktime,
+			      HRTIMER_MODE_REL_SOFT);

It's okay to call this from inside a timer callback itself and return
HRTIMER_RESTART? I don't know off the top of my head.

Although I have been using hrtimer_start to restart the timer within the callback in
our project and seem to work weill, I found that it seems no one does this in the
current mainline kernel code. Therefore, I will add a boolean parameter to the
callback in version 5 to indicate whether it is within the callback function. If it is
in the callback function, I will use hrtimer_forward_now instead of hrtimer_start.
Additionally, while looking at the historical Git logs of hrtimer, I noticed that it is
possible to call hrtimer_start to start the hrtimer outside of the hrtimer callback, but
it requires the protection of raw_spin_lock_irqsave. When entering the
_prb_refresh_rx_retire_blk_timer function, as noted in the comments, there is already
protection with the sk_buff_head lock, so I only need to add a set of irq save and restore
operations. The reason for this is based on the reference from link
https://lore.kernel.org/all/20150415113105.GT5029@twins.programming.kicks-ass.net/T/#u (local) and
the implementation of the perf_mux_hrtimer_restart function.

The implementation of the _prb_refresh_rx_retire_blk_timer function in PATCH v5:

/*  Do NOT update the last_blk_num first.
 *  Assumes sk_buff_head lock is held.
 */
static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc,
					     bool start, bool callback)
{
	unsigned long flags;

	local_irq_save(flags);
	if (start && !callback)
		hrtimer_start(&pkc->retire_blk_timer, pkc->interval_ktime,
			      HRTIMER_MODE_REL_SOFT);
	else
		hrtimer_forward_now(&pkc->retire_blk_timer, pkc->interval_ktime);
	local_irq_restore(flags);
	pkc->last_kactive_blk_num = pkc->kactive_blk_num;
}


Thanks
Xin Zhao