Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [git pull] vfs.git pile 3 - dcache | Date | Tue, 09 Aug 2022 18:00:17 +0200 |
| |
Linus,
On Mon, Aug 08 2022 at 15:43, Linus Torvalds wrote: > On Mon, Aug 8, 2022 at 3:06 PM Thomas Gleixner <tglx@linutronix.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
| |