| Message ID | 20210902215152.ibWfL_bvd%akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [001/212] ia64: fix typo in a comment | expand |
On Thu, Sep 2, 2021 at 2:51 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > From: Vlastimil Babka <vbabka@suse.cz> > Subject: mm, slab: make flush_slab() possible to call with irqs enabled > > Currently flush_slab() is always called with disabled IRQs if it's needed, > but the following patches will change that, so add a parameter to control > IRQ disabling within the function, which only protects the kmem_cache_cpu > manipulation and not the call to deactivate_slab() which doesn't need it. I absolutely DETEST this patch. Code like this is just garbage: > -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c) > +static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, > + bool lock) .... > + if (lock) > + local_irq_save(flags); with actively misleading names ("lock"? WTF? There's no lock there!) and just disgusting semantics with conditionals etc. And the thing is, I don't even see that this would be in the least required. If that function had just been split into two: the first part that needed to absolutely be called with interrupts disabled, and the latter part that didn't, then none of this stupid ugly code would be needed at all. So flush_slab() could have been split into "that first part that returned the freelist and page, and then the second part that did the deactivate_slab() and the stats update. Then somebody who had interrupts disabled anyway (ie existing cases) would just do both. And then the new code that you want to have with interrupts disabled just for the first part would do the first part with interrupts disabled, and the second part without. See? No need for a "flags" field and odd conditional interrupt disables. And yes, I realize that the "inline" means that *most* of the time, the compiler will inline things, the "conditional" will become static, and it will not be a run-time conditional. But it's the human-readable conditional that is the ugly part here. Now any sane human that reads that flush_slab() code will see "oh, sometimes interrupts are disabled, and sometimes they aren't". Because that is what the code does, at that level. But no, what's actually going on is that interrupts are *always* disabled - it's just that sometimes they will have already been disabled in the caller. So all that misleading and misnamed "lock" argument does is really "were interrupts already disabled so that I don't need to disable and re-enable them". That's what the code actually *does*, but it's not how the code reads, and it's not how the code is written, or how the argument in question is named. So it's all very misleading, and ugly. I'm going to sleep on this, and maybe tomorrow morning my reaction will be "whatever, the code probably works". But more likely, tomorrow morning I will take another look, and still say "no, this is actually making the code less maintainable and actively misleading", and just throw the whole slab series out the window. Linus
On Thu, Sep 2, 2021 at 4:15 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > .... > > + if (lock) > > + local_irq_save(flags); > > with actively misleading names ("lock"? WTF? There's no lock there!) > and just disgusting semantics with conditionals etc. .. and I see that "mm, slub: convert kmem_cpu_slab protection to local_lock" makes that local_irq_save() make it use a local_lock, which may have been the cause of the naming. But that doesn't make my other objections about this go away. Instead of having it lock/unlock halfway through the function (and have magic "Oh, the caller already holds the lock, so don't lock" semantics except with misleading names) I really think that function should just have been split in two, and then the locked region can be minimized in the caller only taking it for the first part. Linus
[ Talking to myself while mulling this series over ... ] On Thu, Sep 2, 2021 at 4:34 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Instead of having it lock/unlock halfway through the function (and > have magic "Oh, the caller already holds the lock, so don't lock" > semantics except with misleading names) I really think that function > should just have been split in two, and then the locked region can be > minimized in the caller only taking it for the first part. If there's some reason why it can't sanely be split into two (too many common variables or some odd control flow or whatever), at least the locking logic should be changed from if (lock) local_irq_save(flags); to something along the lines of if (!caller_already_locked) local_irq_save(flags); so that when you read that function on its own, it's clear that the lock is always held over that critical section - and the issue is that perhaps the lock was already taken by the caller. This ignores the whole misleading "lock" name when it isn't even yet a lock. Linus
On 9/3/21 1:51 AM, Linus Torvalds wrote: > [ Talking to myself while mulling this series over ... ] > > On Thu, Sep 2, 2021 at 4:34 PM Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Instead of having it lock/unlock halfway through the function (and >> have magic "Oh, the caller already holds the lock, so don't lock" >> semantics except with misleading names) I really think that function >> should just have been split in two, and then the locked region can be >> minimized in the caller only taking it for the first part. Normally I would have done that, similarly to " [patch 033/212] mm, slub: separate detaching of partial list in unfreeze_partials() from unfreezing" > If there's some reason why it can't sanely be split into two (too many > common variables or some odd control flow or whatever), at least the > locking logic should be changed from I think what discouraged me was that the second part is to call deactivate_slab() and to that we need to return two values from the first part (page and freelist), so one of them has to be a return parameter. On the other hand the second part does so little it can be opencoded. See below. > > if (lock) > local_irq_save(flags); > > to something along the lines of > > if (!caller_already_locked) > local_irq_save(flags); > > so that when you read that function on its own, it's clear that the > lock is always held over that critical section - and the issue is that > perhaps the lock was already taken by the caller. Actually that "already taken" becomes "caller does not need it/can't even take the local lock as it's not local" (it's a cpu hot remove handler on behalf of another, dead cpu). So would it work with something like the following cleanup on top later after proper testing? (now just compile tested). ---8<--- diff --git a/mm/slub.c b/mm/slub.c index df1ac8aff86f..0d9e63e918f1 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2566,38 +2566,33 @@ static inline void unfreeze_partials_cpu(struct kmem_cache *s, #endif /* CONFIG_SLUB_CPU_PARTIAL */ -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, - bool lock) +static inline struct page * +__detach_cpu_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, + void **freelist) { - unsigned long flags; - void *freelist; struct page *page; - if (lock) - local_lock_irqsave(&s->cpu_slab->lock, flags); - - freelist = c->freelist; page = c->page; + *freelist = c->freelist; c->page = NULL; c->freelist = NULL; c->tid = next_tid(c->tid); - if (lock) - local_unlock_irqrestore(&s->cpu_slab->lock, flags); - - if (page) - deactivate_slab(s, page, freelist); - - stat(s, CPUSLAB_FLUSH); + return page; } static inline void __flush_cpu_slab(struct kmem_cache *s, int cpu) { struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu); + struct page *page; + void *freelist; - if (c->page) - flush_slab(s, c, false); + if (c->page) { + page = __detach_cpu_slab(s, c, &freelist); + deactivate_slab(s, page, freelist); + stat(s, CPUSLAB_FLUSH); + } unfreeze_partials_cpu(s, c); } @@ -2618,14 +2613,24 @@ static void flush_cpu_slab(struct work_struct *w) struct kmem_cache *s; struct kmem_cache_cpu *c; struct slub_flush_work *sfw; + struct page *page; + void *freelist; + unsigned long flags; sfw = container_of(w, struct slub_flush_work, work); s = sfw->s; c = this_cpu_ptr(s->cpu_slab); - if (c->page) - flush_slab(s, c, true); + if (c->page) { + local_lock_irqsave(&s->cpu_slab->lock, flags); + page = __detach_cpu_slab(s, c, &freelist); + local_unlock_irqrestore(&s->cpu_slab->lock, flags); + + if (page) + deactivate_slab(s, page, freelist); + stat(s, CPUSLAB_FLUSH); + } unfreeze_partials(s); }
> > so that when you read that function on its own, it's clear that the > > lock is always held over that critical section - and the issue is that > > perhaps the lock was already taken by the caller. > > Actually that "already taken" becomes "caller does not need it/can't > even take the local lock as it's not local" (it's a cpu hot remove > handler on behalf of another, dead cpu). > > So would it work with something like the following cleanup on top later > after proper testing? (now just compile tested). Scroll downward... > ---8<--- > diff --git a/mm/slub.c b/mm/slub.c > index df1ac8aff86f..0d9e63e918f1 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2566,38 +2566,33 @@ static inline void unfreeze_partials_cpu(struct kmem_cache *s, > > #endif /* CONFIG_SLUB_CPU_PARTIAL */ > > -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, > - bool lock) > +static inline struct page * > +__detach_cpu_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, > + void **freelist) > { > - unsigned long flags; > - void *freelist; > struct page *page; > > - if (lock) > - local_lock_irqsave(&s->cpu_slab->lock, flags); > - > - freelist = c->freelist; > page = c->page; > + *freelist = c->freelist; > > c->page = NULL; > c->freelist = NULL; > c->tid = next_tid(c->tid); > > - if (lock) > - local_unlock_irqrestore(&s->cpu_slab->lock, flags); > - > - if (page) > - deactivate_slab(s, page, freelist); > - > - stat(s, CPUSLAB_FLUSH); > + return page; > } > > static inline void __flush_cpu_slab(struct kmem_cache *s, int cpu) > { > struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu); > + struct page *page; > + void *freelist; > > - if (c->page) > - flush_slab(s, c, false); > + if (c->page) { > + page = __detach_cpu_slab(s, c, &freelist); > + deactivate_slab(s, page, freelist); > + stat(s, CPUSLAB_FLUSH); > + } > > unfreeze_partials_cpu(s, c); > } > @@ -2618,14 +2613,24 @@ static void flush_cpu_slab(struct work_struct *w) > struct kmem_cache *s; > struct kmem_cache_cpu *c; > struct slub_flush_work *sfw; > + struct page *page; > + void *freelist; > + unsigned long flags; > > sfw = container_of(w, struct slub_flush_work, work); > > s = sfw->s; > c = this_cpu_ptr(s->cpu_slab); > > - if (c->page) > - flush_slab(s, c, true); > + if (c->page) { > + local_lock_irqsave(&s->cpu_slab->lock, flags); > + page = __detach_cpu_slab(s, c, &freelist); > + local_unlock_irqrestore(&s->cpu_slab->lock, flags); > + > + if (page) > + deactivate_slab(s, page, freelist); > + stat(s, CPUSLAB_FLUSH); > + } > > unfreeze_partials(s); > } To my eyeballs, below duplication of a couple lines of initialization needed by the lockless function is less icky than the double return. --- mm/slub.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) --- a/mm/slub.c +++ b/mm/slub.c @@ -2566,15 +2566,13 @@ static inline void unfreeze_partials_cpu #endif /* CONFIG_SLUB_CPU_PARTIAL */ -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, - bool lock) +static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c) { unsigned long flags; void *freelist; struct page *page; - if (lock) - local_lock_irqsave(&s->cpu_slab->lock, flags); + local_lock_irqsave(&s->cpu_slab->lock, flags); freelist = c->freelist; page = c->page; @@ -2583,8 +2581,7 @@ static inline void flush_slab(struct kme c->freelist = NULL; c->tid = next_tid(c->tid); - if (lock) - local_unlock_irqrestore(&s->cpu_slab->lock, flags); + local_unlock_irqrestore(&s->cpu_slab->lock, flags); if (page) deactivate_slab(s, page, freelist); @@ -2595,11 +2592,19 @@ static inline void flush_slab(struct kme static inline void __flush_cpu_slab(struct kmem_cache *s, int cpu) { struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu); + struct page *page = c->page; + void *freelist = c->freelist; - if (c->page) - flush_slab(s, c, false); + c->page = NULL; + c->freelist = NULL; + c->tid = next_tid(c->tid); + + if (page) + deactivate_slab(s, page, freelist); unfreeze_partials_cpu(s, c); + + stat(s, CPUSLAB_FLUSH); } struct slub_flush_work { @@ -2625,7 +2630,7 @@ static void flush_cpu_slab(struct work_s c = this_cpu_ptr(s->cpu_slab); if (c->page) - flush_slab(s, c, true); + flush_slab(s, c); unfreeze_partials(s); }
On 9/3/21 08:22, Mike Galbraith wrote: >> > so that when you read that function on its own, it's clear that the >> > lock is always held over that critical section - and the issue is that >> > perhaps the lock was already taken by the caller. >> >> Actually that "already taken" becomes "caller does not need it/can't Meant to say "... later in the series becomes ...". >> even take the local lock as it's not local" (it's a cpu hot remove >> handler on behalf of another, dead cpu). >> >> So would it work with something like the following cleanup on top later >> after proper testing? (now just compile tested). > > To my eyeballs, below duplication of a couple lines of initialization > needed by the lockless function is less icky than the double return. Yeah, that's better, thanks Mike.
On 9/3/21 09:03, Vlastimil Babka wrote: > On 9/3/21 08:22, Mike Galbraith wrote: >>> > so that when you read that function on its own, it's clear that the >>> > lock is always held over that critical section - and the issue is that >>> > perhaps the lock was already taken by the caller. >>> >>> Actually that "already taken" becomes "caller does not need it/can't > > Meant to say "... later in the series becomes ...". > >>> even take the local lock as it's not local" (it's a cpu hot remove >>> handler on behalf of another, dead cpu). >>> >>> So would it work with something like the following cleanup on top later >>> after proper testing? (now just compile tested). >> >> To my eyeballs, below duplication of a couple lines of initialization >> needed by the lockless function is less icky than the double return. > > Yeah, that's better, thanks Mike. Formal patch below, also added to my git branch: https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=slub-local-lock-v5r1 ----8<---- From b67952ce67528f3ebeaae58e0eae22a6dbae64b5 Mon Sep 17 00:00:00 2001 From: Vlastimil Babka <vbabka@suse.cz> Date: Fri, 3 Sep 2021 12:59:25 +0200 Subject: [PATCH] mm, slub: remove conditional locking parameter from flush_slab() flush_slab() is called either as part of work scheduled on given live cpu, or called as a cleanup for another cpu that went offline. In the first case it needs to hold the cpu_slab->lock local lock when updating the protected kmem_cache_cpu fields. This is now achieved by a "bool lock" parameter. To avoid the conditional locking, we can instead lock unconditionally in flush_slab() for live cpus, and opencode the variant without locking in __flush_cpu_slab() for the dead cpus. Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Suggested-by: Mike Galbraith <efault@gmx.de> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slub.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/mm/slub.c b/mm/slub.c index df1ac8aff86f..77fe3d6d2065 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2566,15 +2566,13 @@ static inline void unfreeze_partials_cpu(struct kmem_cache *s, #endif /* CONFIG_SLUB_CPU_PARTIAL */ -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, - bool lock) +static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c) { unsigned long flags; void *freelist; struct page *page; - if (lock) - local_lock_irqsave(&s->cpu_slab->lock, flags); + local_lock_irqsave(&s->cpu_slab->lock, flags); freelist = c->freelist; page = c->page; @@ -2583,8 +2581,7 @@ static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, c->freelist = NULL; c->tid = next_tid(c->tid); - if (lock) - local_unlock_irqrestore(&s->cpu_slab->lock, flags); + local_unlock_irqrestore(&s->cpu_slab->lock, flags); if (page) deactivate_slab(s, page, freelist); @@ -2595,9 +2592,17 @@ static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, static inline void __flush_cpu_slab(struct kmem_cache *s, int cpu) { struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu); + struct page *page = c->page; + void *freelist = c->freelist; - if (c->page) - flush_slab(s, c, false); + c->page = NULL; + c->freelist = NULL; + c->tid = next_tid(c->tid); + + if (page) { + deactivate_slab(s, page, freelist); + stat(s, CPUSLAB_FLUSH); + } unfreeze_partials_cpu(s, c); } @@ -2625,7 +2630,7 @@ static void flush_cpu_slab(struct work_struct *w) c = this_cpu_ptr(s->cpu_slab); if (c->page) - flush_slab(s, c, true); + flush_slab(s, c); unfreeze_partials(s); }
On Thu, Sep 2, 2021 at 11:22 PM Mike Galbraith <efault@gmx.de> wrote: > > To my eyeballs, below duplication of a couple lines of initialization > needed by the lockless function is less icky than the double return. Ack. Something like this seems to avoid the whole issue. Vlastimil - the alternative I was actually going to suggest for that "double return value" thing is something we use elsewhere in the VM code - just use a structure for "state". It's not hugely common, and I think Mike's solution in this case is the much simpler one, but I'll mention it anyway because it's actually been quite successful in the few cases we use it, and it's both readable to humans and generates fairly good code. The "generates fairly good code" is generally more true when inlining, but it's not horrible even outside of that. So the idea with the "state structure" is that you just pass in both the arguments and the return values in a struct. Some examples of this are - 'struct follow_page_context' in mm/gup.c is an example of a small local one. - 'struct vm_fault' is an example of a *big* one, that has a lot of state, and that is actually exported as an interface. That 'struct vm_fault' one is interesting as an example because it does that "both inputs and outputs", and has that unnamed "const struct" member to actually catch mis-uses where some callee would change those fields (which is a no-no). It's also an example of something that generates good code despite not inlining, because it actively makes argument passing simpler and avoids copying arguments as you call other functions. Those issues are non-issues in this case, which is why I point to that 'follow_page_context' in the gup code as a very different example of this, which is much more along the lines of the slub case. It's purely local to that call chain, and it's basically used to return more state. In both cases, the top-level caller just initializes things on the stack. For that 'struct vm_fault' case you actually *have* to use an initializer, since the constant fields cannot be changed later. It makes for pretty legible code, and when things are inlined, all those fields actually act exactly like just regular local variables shared across functions. When things aren't inlined, it can generate extra memory accesses, but that 'struct vm_fault' is an example of when that isn't even necessarily worse. Anyway, I wanted to just point that out as a pattern that has been quite successful. Side note: one very special case of that pattern goes back decades: the VM use of structures that get returned and passed *as* structures. It's in fact so common that people don't even think about it: 'pte_t' and friends. They are designed to be opaque data types that are often multi-word structures. Quite often those structures have only one or two words in them, which helps code generation (returning two words can be done in registers), but they _can_ have more. Eg x86: typedef union { struct { unsigned long pte_low, pte_high; }; pteval_t pte; } pte_t; or some powerpc cases: typedef struct { pte_basic_t pte, pte1, pte2, pte3; } pte_t; just to show that you can actually pass structures not by reference, but by value, and it's not even unusual in the kernel. That can also be used to return multiple values if you want to. Note: code generation really matters, and when doing those multi-value structures, for code generation it's generally important to limit it to at most two words. Anything more, and the compiler will generally be forced to pass it by copying it to the stack and using a pointer to it, and then you get the worst of both worlds. You're better off just passing the structure by reference, and avoiding extra copies on the stack. ANYWAY. I'll drop this part from Andrew's patch-bomb, and I can take it later in its cleaned-up form. You mentioned a git tree elsewhere, maybe even that way. Linus
--- a/mm/slub.c~mm-slab-make-flush_slab-possible-to-call-with-irqs-enabled +++ a/mm/slub.c @@ -2480,16 +2480,28 @@ static void put_cpu_partial(struct kmem_ #endif /* CONFIG_SLUB_CPU_PARTIAL */ } -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c) +static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c, + bool lock) { - void *freelist = c->freelist; - struct page *page = c->page; + unsigned long flags; + void *freelist; + struct page *page; + + if (lock) + local_irq_save(flags); + + freelist = c->freelist; + page = c->page; c->page = NULL; c->freelist = NULL; c->tid = next_tid(c->tid); - deactivate_slab(s, page, freelist); + if (lock) + local_irq_restore(flags); + + if (page) + deactivate_slab(s, page, freelist); stat(s, CPUSLAB_FLUSH); } @@ -2499,7 +2511,7 @@ static inline void __flush_cpu_slab(stru struct kmem_cache_cpu *c = per_cpu_ptr(s->cpu_slab, cpu); if (c->page) - flush_slab(s, c); + flush_slab(s, c, false); unfreeze_partials_cpu(s, c); } @@ -2515,7 +2527,7 @@ static void flush_cpu_slab(void *d) struct kmem_cache_cpu *c = this_cpu_ptr(s->cpu_slab); if (c->page) - flush_slab(s, c); + flush_slab(s, c, false); unfreeze_partials(s); }