linux-kernel - Re: [PREEMPT_RT PATCH 2/3] i915: convert all irq

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]

Date:   Mon, 16 Sep 2019 14:21:22 -0700
From:   Sean V Kelley <sean.v.kelley@...ux.intel.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Clark Williams <clark.williams@...il.com>, bigeasy@...utronix.com,
        tglx@...utronix.com, linux-rt-users@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PREEMPT_RT PATCH 2/3] i915: convert all irq_locks spinlocks to
 raw spinlocks



> On Sep 3, 2019, at 1:03 AM, Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> 
> On 2019-08-19 19:33:18 [-0500], Clark Williams wrote:
>> From: Clark Williams <williams@...hat.com>
>> 
>> The following structures contain a member named 'irq_lock'.
>> These three locks are of type spinlock_t and are used in
>> multiple contexts including atomic:
>> 
>>    struct drm_i915_private
>>    struct intel_breadcrumbs
>>    strict intel_guc
>> 
>> Convert them all to be raw_spinlock_t so that lockdep and the lock
>> debugging code will be happy.
> 
> What is your motivation to make the lock raw?
> I did the following:
> 
> void intel_engine_signal_breadcrumbs(struct intel_engine_cs *engine)
> {
> -       local_irq_disable();
> -       intel_engine_breadcrumbs_irq(engine);
> -       local_irq_enable();
> +       if (IS_ENABLED(CONFIG_PREEMPT_RT_FULL)) {
> +               intel_engine_breadcrumbs_irq(engine);
> +       } else {
> +               local_irq_disable();
> +               intel_engine_breadcrumbs_irq(engine);
> +               local_irq_enable();
> +       }
> }
> 
> and lockdep was quiet (+ ignoring/patching the lockdep-irq-off-asserts).
> The local_irq_disable() is here (my interpretation of the situation)
> because that function is called from process context while the remaining
> callers invoke intel_engine_breadcrumbs_irq() from the interrupt
> handler and it acquires irq_lock via a plain spin_lock().  That
> local_irq_disable() would be required if everyone did a _irqsave().

I’ve tested this also on the v5.2.14-rt7 and can confirm that it avoids the need for making the locks raw.

Tested-by: Sean V Kelley <sean.v.kelley@...ux.intel.com>

Thanks,

Sean

> 
> I tried to check how much worse the latency gets here but I didn't see
> anything in a brief test. What I saw however is that switching to
> fullscreen while playing a video gives me ~0.5 to ~2ms latency. This is
> has nothing to do with this change, I have to dig deeper… It might be
> one of the preempt_disable() section I just noticed.
> I would prefer to keep the lock non-raw unless there is actual need for
> it.
> 
> Sebastian