linux-kernel - Re: [patch 2/3] timers: do not raise softirq unconditionally (spinlockless version)

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.21.1905291653120.1395@somnus>
Date:   Wed, 29 May 2019 16:53:26 +0200 (CEST)
From:   Anna-Maria Gleixner <anna-maria@...utronix.de>
To:     Marcelo Tosatti <mtosatti@...hat.com>
cc:     linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Luiz Capitulino <lcapitulino@...hat.com>,
        Haris Okanovic <haris.okanovic@...com>
Subject: Re: [patch 2/3] timers: do not raise softirq unconditionally
 (spinlockless version)

On Mon, 15 Apr 2019, Marcelo Tosatti wrote:

> Check base->pending_map locklessly and skip raising timer softirq 
> if empty.
> 
> What allows the lockless (and potentially racy against mod_timer) 
> check is that mod_timer will raise another timer softirq after
> modifying base->pending_map.

The raise of the timer softirq after adding the timer is done
unconditionally - so there are timer softirqs raised which are not required
at all, as mentioned before.

This check is for !CONFIG_PREEMPT_RT_FULL only implemented. The commit
message totally igonres that you are implementing something
CONFIG_PREEMPT_RT_FULL dependent as well.

> Signed-off-by: Marcelo Tosatti <mtosatti@...hat.com>
> 
> ---
>  kernel/time/timer.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> Index: linux-rt-devel/kernel/time/timer.c
> ===================================================================
> --- linux-rt-devel.orig/kernel/time/timer.c	2019-04-15 14:21:02.788704354 -0300
> +++ linux-rt-devel/kernel/time/timer.c	2019-04-15 14:22:56.755047354 -0300
> @@ -1776,6 +1776,24 @@
>  		if (time_before(jiffies, base->clk))
>  			return;
>  	}
> +
> +#ifdef CONFIG_PREEMPT_RT_FULL
> +/* On RT, irq work runs from softirq */
> +	if (irq_work_needs_cpu())
> +		goto raise;

So with this patch and the change you made in the patch before, timers on
RT are expired only when there is pending irq work or after modifying a
timer on a non housekeeping cpu?

With your patches I could create the following problematic situation on RT
(if I understood everything properly): I add a timer which should expire in
50 jiffies to the wheel of a non housekeeping cpu. So it ends up 50 buckets
away form now in the first wheel. This timer is the only timer in the wheel
and the next timer softirq raise is required in 50 jiffies. After adding
the timer, the timer interrupt is raised, and no timer has to be expired,
because there is no timer pending. If there is no irq work required during
the next 51 jiffies and also no timer changed, the timer I added, will not
expire in time. The timer_base will come out of idle but will not forward
the base clk. This makes it even worse: When then adding a timer, the timer
base is forwarded - but without checking for the next pending timer, so the
first added timer will be delayed even more.

So your implementation lacks forwarding the timer_base->clk when timer_base
comes out of idle with respect to the next pending timer.

> +#endif
> +	base = this_cpu_ptr(&timer_bases[BASE_STD]);
> +	if (!housekeeping_cpu(base->cpu, HK_FLAG_TIMER)) {
> +		if (!bitmap_empty(base->pending_map, WHEEL_SIZE))
> +			goto raise;
> +		base++;
> +		if (!bitmap_empty(base->pending_map, WHEEL_SIZE))
> +			goto raise;
> +
> +		return;
> +	}
> +
> +raise:
>  	raise_softirq(TIMER_SOFTIRQ);
>  }
>  
>

Thanks,

	Anna-Maria