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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHk-=wizUgMbZKnOjvyeZT5E+WZM0sV+zS5Qxt84wp=BsRk3eQ@mail.gmail.com>
Date:   Wed, 3 Aug 2022 11:57:27 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] vfs.git pile 3 - dcache

On Wed, Aug 3, 2022 at 11:39 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
>         Main part here is making parallel lookups safe for RT - making
> sure preemption is disabled in start_dir_add()/ end_dir_add() sections (on
> non-RT it's automatic, on RT it needs to to be done explicitly) and moving
> wakeups from __d_lookup_done() inside of such to the end of those sections.

Ugh.

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();
        ...

which would make the code really explain what is going on. I would
still encourage that *comment* about it, but I think we really should
strive for code that makes sense even without a comment.

The fact that then without PREEMPT_RT, the whole
"preempt_disable_under_spinlock()" becomes a no-op is then an
implementation detail - and not so different from how a regular
preempt_disable() becomes a no-op when on UP (or with PREEMPT_NONE).

And that "preempt_disable_under_spinlock()" really documents what is
going on, and I feel would make that code easier to understand? The
fact that PREEMPT_RT has different rules about preemption is not
something that the dentry code should care about.

The dentry code could just say "I want to disable preemption, and I
already hold a spinlock, so do what is best".

So then "preempt_disable_under_spinlock()" precisely documents what
the dentry code really wants.

No?

Anyway, I have pulled this, but I really would like fewer of these
random PREEMPT_RT turds around, and more "this code makes sense" code.

                Linus