Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 Aug 2022 20:49:32 +0100
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [git pull] vfs.git pile 3 - dcache
Message-ID: <YurRTBO40kil4Bw5@ZenIV>
References: <YurA3aSb4GRr4wlW@ZenIV>
 <CAHk-=wizUgMbZKnOjvyeZT5E+WZM0sV+zS5Qxt84wp=BsRk3eQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHk-=wizUgMbZKnOjvyeZT5E+WZM0sV+zS5Qxt84wp=BsRk3eQ@mail.gmail.com>
Sender: Al Viro <viro@ftp.linux.org.uk>
Precedence: bulk

On Wed, Aug 03, 2022 at 11:57:27AM -0700, Linus Torvalds wrote:
> On Wed, Aug 3, 2022 at 11:39 AM Al Viro <viro@zeniv.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?

Fine by me, but I think that this is better dealt with by the rt folks;
I've no objections to replacing that open-coded stuff in dcache.c with
better documented primitives, so when such patches materialize...