linux-kernel - Re: [git pull] vfs.git pile 3

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: <YuruqoGHJONpdZcK@home.goodmis.org>
Date:   Wed, 3 Aug 2022 17:54:50 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Al Viro <viro@...iv.linux.org.uk>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] vfs.git pile 3 - dcache

On Wed, Aug 03, 2022 at 11:57:27AM -0700, Linus Torvalds wrote:
> 
> I really dislike this pattern:
> 
>         if (IS_ENABLED(CONFIG_PREEMPT_RT))
>                 preempt_disable();
>        ...
>         if (IS_ENABLED(CONFIG_PREEMPT_RT))
>                 preempt_enable();
> 
> and while the new comment explains *why* it exists, it's still very ugly indeed.
> 
> We have it in a couple of other places, and we also end up having
> another variation on the theme that is about "migrate_{dis,en}able()",
> except it is written as
> 
>         if (IS_ENABLED(CONFIG_PREEMPT_RT))
>                 migrate_disable();
>         else
>                 preempt_disable();
> 
> because on non-PREEMPT_RT obviously preempt_disable() is the better
> and simpler thing.
> 
> Can we please just introduce helper functions?
> 
> At least that
> 
>         if (IS_ENABLED(CONFIG_PREEMPT_RT))
>                 preempt_disable();
>         ...
> 
> pattern could be much more naturally expressed as
> 
>         preempt_disable_under_spinlock();
>         ...
>

The original patch years ago use to have:

 preempt_disable_rt()

 preempt_enable_rt()


That did exactly that, but an effort was made to get rid of it. But your more
descriptive "preempt_enable/disable_under_spinlock()" may make more sense.

-- Steve