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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Xin Zhao wrote:
> In a system with high real-time requirements, the timeout mechanism of
> ordinary timers with jiffies granularity is insufficient to meet the
> demands for real-time performance. Meanwhile, the optimization of CPU
> usage with af_packet is quite significant. Use hrtimer instead of timer
> to help compensate for the shortcomings in real-time performance.
> In HZ=100 or HZ=250 system, the update of TP_STATUS_USER is not real-time
> enough, with fluctuations reaching over 8ms (on a system with HZ=250).
> This is unacceptable in some high real-time systems that require timely
> processing of network packets. By replacing it with hrtimer, if a timeout
> of 2ms is set, the update of TP_STATUS_USER can be stabilized to within
> 3 ms.
> 
> Signed-off-by: Xin Zhao <jackzxcui1989@163.com>

> -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, bool callback)
>  {
> -	mod_timer(&pkc->retire_blk_timer,
> -			jiffies + pkc->tov_in_jiffies);
> +	unsigned long flags;
> +
> +	local_irq_save(flags);

The two environments that can race are the timer callback running in
softirq context or the open_block from tpacket_rcv in process context.

So worst case the process context path needs to disable bh?

As you pointed out, the accesses to the hrtimer fields are already
protected, by the caller holding sk.sk_receive_queue.lock.

So it should be sufficient to just test hrtimer_is_queued inside that
critical section before calling hrtimer_start?

Side-note: tpacket_rcv calls spin_lock, not spin_lock_bh. But if the
same lock can also be taken in softirq context, the process context
caller should use the _bh variant. This is not new with your patch.
Classical timers also run in softirq context. I may be overlooking
something, will need to take a closer look at that.

In any case, I don't think local_irq_save is needed. 

> +	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;
>  }