Messages in this thread | | | From | Linus Torvalds <> | Date | Wed, 3 Aug 2022 11:57:27 -0700 | Subject | Re: [git pull] vfs.git pile 3 - dcache |
| |
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?
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
| |