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]

Date:   Tue, 09 Aug 2022 18:00:17 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Matthew Wilcox <willy@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] vfs.git pile 3 - dcache

Linus,

On Mon, Aug 08 2022 at 15:43, Linus Torvalds wrote:
> On Mon, Aug 8, 2022 at 3:06 PM Thomas Gleixner <tglx@...utronix.de> wrote:
>> The use cases in mm/vmstat are not really all under spinlocks. That code
>> gets called also just from plain local_irq or even just preempt disabled
>> regions (depending on the stats item), which makes the proposed name
>> less accurate than you describe.
>
> Augh.
>
> How about "preempt_disable_nested()" with a big comment about how some
> operations normally disable preemption (interrupts off, spinlocks,
> anything else?) but not on PREEMPT_RT?

Let me do that.

>> A worse case is the u64_stat code which is an ifdef maze (only partially
>> due to RT). Those stats updates can also be called from various contexts
>> where no spinlock is involved. That code is extra convoluted due to
>> irqsave variants and "optimizations" for 32bit UP. Removing the latter
>> would make a cleanup with write_seqcount_...() wrappers pretty simple.
>
> I think we most definitely can start removing optimisations for 32-bit
> UP by now.
>
> Let's not do them without any reason, but any time you hit a code that
> makes you go "this makes it harder to do better", feel free to go all
> Alexander the Great on the 32-bit UP code and just cut through the
> problem by removing it.

With that mopped up:

 1 file changed, 42 insertions(+), 84 deletions(-)

plus a followup cleanup of the then not longer required
_irqsave/restore() variants:

 8 files changed, 33 insertions(+), 62 deletions(-)

is not a Great conquest, but makes the code definitely readable.

The fetch/retry_irq() variants are then obsolete as well, but that's just
a rename in 70 files and the removal of the two helpers.

>> Aside of that we have RT conditional preempt related code in
>> page_alloc() and kmap_atomic(). Both care only about the task staying
>> pinned on a CPU. In page_alloc() using preempt_disable() on !RT is more
>> lightweight than migrate_disable(). So something like
>> task_[un]pin_temporary() might work and be descriptive enough.
>
> Yeah, that was the other odd pattern. I'm not sure "temporary" is all
> that relevant, but yes, if we end up having more of those, some kind
> of "thread_{un]pin_cpu()" would probably be worth it.
>
> But the kmap code may be so special that nothing else has _that_
> particular issue.

We just want to get rid of kmap_atomic() completely. I'll go and find
minions.

Thanks,

        tglx