linux-kernel - Re: [PATCH] sched: use rq_lock/unlock in online_fair_sched

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: <d1997635-0e89-c901-00d4-819d6c2cc33c@arm.com>
Date:   Fri, 9 Aug 2019 18:43:09 +0100
From:   Valentin Schneider <valentin.schneider@....com>
To:     Phil Auld <pauld@...hat.com>, Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH] sched: use rq_lock/unlock in online_fair_sched_group

On 09/08/2019 14:33, Phil Auld wrote:
> On Tue, Aug 06, 2019 at 03:03:34PM +0200 Peter Zijlstra wrote:
>> On Thu, Aug 01, 2019 at 09:37:49AM -0400, Phil Auld wrote:
>>> Enabling WARN_DOUBLE_CLOCK in /sys/kernel/debug/sched_features causes
>>
>> ISTR there were more issues; but it sure is good to start picking them
>> off.
>>
> 
> Following up on this I hit another in rt.c which looks like:
> 
> [  156.348854] Call Trace:
> [  156.351301]  <IRQ>
> [  156.353322]  sched_rt_period_timer+0x124/0x350
> [  156.357766]  ? sched_rt_rq_enqueue+0x90/0x90
> [  156.362037]  __hrtimer_run_queues+0xfb/0x270
> [  156.366303]  hrtimer_interrupt+0x122/0x270
> [  156.370403]  smp_apic_timer_interrupt+0x6a/0x140
> [  156.375022]  apic_timer_interrupt+0xf/0x20
> [  156.379119]  </IRQ>
> 
> It looks like the same issue of not using the rq_lock* wrappers and
> hence not using the pinning. From looking at the code there is at 
> least one potential hit in deadline.c in the push_dl_task path with 
> find_lock_later_rq but I have not hit that in practice.
> 
> This commit, which introduced the warning, seems to imply that the use
> of the rq_lock* wrappers is required, at least for any sections that will
> call update_rq_clock:
> 
> commit 26ae58d23b94a075ae724fd18783a3773131cfbc
> Author: Peter Zijlstra <peterz@...radead.org>
> Date:   Mon Oct 3 16:53:49 2016 +0200
> 
>     sched/core: Add WARNING for multiple update_rq_clock() calls
>     
>     Now that we have no missing calls, add a warning to find multiple
>     calls.
>     
>     By having only a single update_rq_clock() call per rq-lock section,
>     the section appears 'atomic' wrt time.
> 
> 
> Is that the case? Otherwise we have these false positives.
> 

Looks like it - only rq_pin_lock() clears RQCF_UPDATED, so any
update_rq_clock() that isn't preceded by that function will still have
RQCF_UPDATED set the second time it's executed and will trigger the warn.

Seeing as the wrappers boil down to raw_spin_*() when the debug bits are
disabled, I don't see why we wouldn't want to convert these callsites.

> I can spin up patches if so. 
> 
> 
> Thanks,
> Phil
> 
>