Linus,
On Mon, Aug 08 2022 at 15:43, Linus Torvalds wrote:
> On Mon, Aug 8, 2022 at 3:06 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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
