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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20250819062451.1089842-1-jackzxcui1989@163.com>
Date: Tue, 19 Aug 2025 14:24:51 +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 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.
> 
> >   */
> > -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

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

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