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

On Mon, 2025-08-11 at 22:48 +0800, Willem wrote:

quoted

@@ -603,9 +603,10 @@ static void prb_setup_retire_blk_timer(struct packet_sock *po)
 	struct tpacket_kbdq_core *pkc;

 	pkc = GET_PBDQC_FROM_RB(&po->rx_ring);
-	timer_setup(&pkc->retire_blk_timer, prb_retire_rx_blk_timer_expired,
-		    0);
-	pkc->retire_blk_timer.expires = jiffies;
+	hrtimer_setup(&pkc->retire_blk_timer, prb_retire_rx_blk_timer_expired,
+		      CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+	if (pkc->tov_in_msecs == 0)
+		pkc->tov_in_msecs = jiffies_to_msecs(1);

why is this bounds check needed now, while it was not needed when
converting to jiffies?

init_prb_bdqc will compute a retire_blk_tov if it is passed as zero,
by calling prb_calc_retire_blk_tmo.

Dear Willem,

I am very grateful for your suggestion. I will delete this bounds check in the v1
PATCH.

quoted

 static void _prb_refresh_rx_retire_blk_timer(struct tpacket_kbdq_core *pkc)
 {
-	mod_timer(&pkc->retire_blk_timer,
-			jiffies + pkc->tov_in_jiffies);
+	hrtimer_start_range_ns(&pkc->retire_blk_timer,
+			       ms_to_ktime(pkc->tov_in_msecs), 0, HRTIMER_MODE_REL_SOFT);

Just hrtimer_start if leaving the slack (delta_ns) as 0.

More importantly, this scheduled the timer, while the caller also
returns HRTIMER_RESTART. Should this just call hrtimer_set_expires or
hrtimer_forward.

I will use hrtimer_set_expires here while using hrtimer_start in 
prb_setup_retire_blk_timer to start the timer.
Previously I used hrtimer_forward here, then encountered 
WARN_ON(timer->state & HRTIMER_STATE_ENQUEUED warning in the hrtimer_forward
function. This warning occurred because the tpacket_rcv function eventually calls
prb_open_block, which might call _prb_refresh_rx_retire_blk_timer at the same time
prb_retire_rx_blk_timer_expired might also call _prb_refresh_rx_retire_blk_timer,
leading to the warning.

quoted

 refresh_timer:
 	_prb_refresh_rx_retire_blk_timer(pkc);
+	ret = HRTIMER_RESTART;

reinitializing a variable that was already set to the same value?

quoted

 out:
 	spin_unlock(&po->sk.sk_receive_queue.lock);
+	return ret;

just return HRTIMER_RESTART directly.

quoted

 }

I will modify the patch based on these suggestions and upload it later.


Thanks
Xin Zhao