| Message ID | 20220217094802.3644569-6-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcg: Address PREEMPT_RT problems instead of disabling it. | expand |
On Thu 17-02-22 10:48:02, Sebastian Andrzej Siewior wrote: [...] > @@ -2266,7 +2273,6 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > * as well as workers from this path always operate on the local > * per-cpu data. CPU up doesn't touch memcg_stock at all. > */ > - curcpu = get_cpu(); Could you make this a separate patch? > for_each_online_cpu(cpu) { > struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); > struct mem_cgroup *memcg; > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > rcu_read_unlock(); > > if (flush && > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > - if (cpu == curcpu) > - drain_local_stock(&stock->work); > - else > - schedule_work_on(cpu, &stock->work); > - } > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > + schedule_work_on(cpu, &stock->work); Maybe I am missing but on !PREEMPT kernels there is nothing really guaranteeing that the worker runs so there should be cond_resched after the mutex is unlocked. I do not think we want to rely on callers to be aware of this subtlety. An alternative would be to split out __drain_local_stock which doesn't do local_lock.
On 2022-02-21 15:46:30 [+0100], Michal Hocko wrote: > On Thu 17-02-22 10:48:02, Sebastian Andrzej Siewior wrote: > [...] > > @@ -2266,7 +2273,6 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > * as well as workers from this path always operate on the local > > * per-cpu data. CPU up doesn't touch memcg_stock at all. > > */ > > - curcpu = get_cpu(); > > Could you make this a separate patch? Sure. > > for_each_online_cpu(cpu) { > > struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); > > struct mem_cgroup *memcg; > > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > rcu_read_unlock(); > > > > if (flush && > > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > > - if (cpu == curcpu) > > - drain_local_stock(&stock->work); > > - else > > - schedule_work_on(cpu, &stock->work); > > - } > > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > > + schedule_work_on(cpu, &stock->work); > > Maybe I am missing but on !PREEMPT kernels there is nothing really > guaranteeing that the worker runs so there should be cond_resched after > the mutex is unlocked. I do not think we want to rely on callers to be > aware of this subtlety. There is no guarantee on PREEMPT kernels, too. The worker will be made running and will be put on the CPU when the scheduler sees it fit and there could be other worker which take precedence (queued earlier). But I was not aware that the worker _needs_ to run before we return. We might get migrated after put_cpu() so I wasn't aware that this is important. Should we attempt best effort and wait for the worker on the current CPU? > An alternative would be to split out __drain_local_stock which doesn't > do local_lock. but isn't the section in drain_local_stock() unprotected then? Sebastian
On Mon 21-02-22 16:19:52, Sebastian Andrzej Siewior wrote: > On 2022-02-21 15:46:30 [+0100], Michal Hocko wrote: > > On Thu 17-02-22 10:48:02, Sebastian Andrzej Siewior wrote: > > [...] > > > @@ -2266,7 +2273,6 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > > * as well as workers from this path always operate on the local > > > * per-cpu data. CPU up doesn't touch memcg_stock at all. > > > */ > > > - curcpu = get_cpu(); > > > > Could you make this a separate patch? > > Sure. > > > > for_each_online_cpu(cpu) { > > > struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); > > > struct mem_cgroup *memcg; > > > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > > rcu_read_unlock(); > > > > > > if (flush && > > > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > > > - if (cpu == curcpu) > > > - drain_local_stock(&stock->work); > > > - else > > > - schedule_work_on(cpu, &stock->work); > > > - } > > > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > > > + schedule_work_on(cpu, &stock->work); > > > > Maybe I am missing but on !PREEMPT kernels there is nothing really > > guaranteeing that the worker runs so there should be cond_resched after > > the mutex is unlocked. I do not think we want to rely on callers to be > > aware of this subtlety. > > There is no guarantee on PREEMPT kernels, too. The worker will be made > running and will be put on the CPU when the scheduler sees it fit and > there could be other worker which take precedence (queued earlier). > But I was not aware that the worker _needs_ to run before we return. A lack of draining will not be a correctness problem (sorry I should have made that clear). It is more about subtlety than anything. E.g. the charging path could be forced to memory reclaim because of the cached charges which are still waiting for their draining. Not really something to lose sleep over from the runtime perspective. I was just wondering that this makes things more complex than necessary. > We > might get migrated after put_cpu() so I wasn't aware that this is > important. Should we attempt best effort and wait for the worker on the > current CPU? > > An alternative would be to split out __drain_local_stock which doesn't > > do local_lock. > > but isn't the section in drain_local_stock() unprotected then? local_lock instead of {get,put}_cpu would handle that right?
On 2022-02-21 17:24:41 [+0100], Michal Hocko wrote: > > > > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > > > rcu_read_unlock(); > > > > > > > > if (flush && > > > > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > > > > - if (cpu == curcpu) > > > > - drain_local_stock(&stock->work); > > > > - else > > > > - schedule_work_on(cpu, &stock->work); > > > > - } > > > > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > > > > + schedule_work_on(cpu, &stock->work); > > > > > > Maybe I am missing but on !PREEMPT kernels there is nothing really > > > guaranteeing that the worker runs so there should be cond_resched after > > > the mutex is unlocked. I do not think we want to rely on callers to be > > > aware of this subtlety. > > > > There is no guarantee on PREEMPT kernels, too. The worker will be made > > running and will be put on the CPU when the scheduler sees it fit and > > there could be other worker which take precedence (queued earlier). > > But I was not aware that the worker _needs_ to run before we return. > > A lack of draining will not be a correctness problem (sorry I should > have made that clear). It is more about subtlety than anything. E.g. the > charging path could be forced to memory reclaim because of the cached > charges which are still waiting for their draining. Not really something > to lose sleep over from the runtime perspective. I was just wondering > that this makes things more complex than necessary. So it is no strictly wrong but it would be better if we could do drain_local_stock() on the local CPU. > > We > > might get migrated after put_cpu() so I wasn't aware that this is > > important. Should we attempt best effort and wait for the worker on the > > current CPU? > > > > > An alternative would be to split out __drain_local_stock which doesn't > > > do local_lock. > > > > but isn't the section in drain_local_stock() unprotected then? > > local_lock instead of {get,put}_cpu would handle that right? It took a while, but it clicked :) If we acquire the lock_lock_t, that we would otherwise acquire in drain_local_stock(), before the for_each_cpu loop (as you say get,pu_cpu) then we would indeed need __drain_local_stock() and things would work. But it looks like an abuse of the lock to avoid CPU migration since there is no need to have it acquired at this point. Also the whole section would run with disabled interrupts and there is no need for it. What about if replace get_cpu() with migrate_disable()? Sebastian
On Mon 21-02-22 17:44:13, Sebastian Andrzej Siewior wrote: > On 2022-02-21 17:24:41 [+0100], Michal Hocko wrote: > > > > > @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > > > > > rcu_read_unlock(); > > > > > > > > > > if (flush && > > > > > - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { > > > > > - if (cpu == curcpu) > > > > > - drain_local_stock(&stock->work); > > > > > - else > > > > > - schedule_work_on(cpu, &stock->work); > > > > > - } > > > > > + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) > > > > > + schedule_work_on(cpu, &stock->work); > > > > > > > > Maybe I am missing but on !PREEMPT kernels there is nothing really > > > > guaranteeing that the worker runs so there should be cond_resched after > > > > the mutex is unlocked. I do not think we want to rely on callers to be > > > > aware of this subtlety. > > > > > > There is no guarantee on PREEMPT kernels, too. The worker will be made > > > running and will be put on the CPU when the scheduler sees it fit and > > > there could be other worker which take precedence (queued earlier). > > > But I was not aware that the worker _needs_ to run before we return. > > > > A lack of draining will not be a correctness problem (sorry I should > > have made that clear). It is more about subtlety than anything. E.g. the > > charging path could be forced to memory reclaim because of the cached > > charges which are still waiting for their draining. Not really something > > to lose sleep over from the runtime perspective. I was just wondering > > that this makes things more complex than necessary. > > So it is no strictly wrong but it would be better if we could do > drain_local_stock() on the local CPU. > > > > We > > > might get migrated after put_cpu() so I wasn't aware that this is > > > important. Should we attempt best effort and wait for the worker on the > > > current CPU? > > > > > > > > An alternative would be to split out __drain_local_stock which doesn't > > > > do local_lock. > > > > > > but isn't the section in drain_local_stock() unprotected then? > > > > local_lock instead of {get,put}_cpu would handle that right? > > It took a while, but it clicked :) > If we acquire the lock_lock_t, that we would otherwise acquire in > drain_local_stock(), before the for_each_cpu loop (as you say > get,pu_cpu) then we would indeed need __drain_local_stock() and things > would work. But it looks like an abuse of the lock to avoid CPU > migration since there is no need to have it acquired at this point. Also > the whole section would run with disabled interrupts and there is no > need for it. > > What about if replace get_cpu() with migrate_disable()? Yeah, that would be a better option. I am just not used to think in RT so migrate_disable didn't really come to my mind.
On 2022-02-21 18:17:39 [+0100], Michal Hocko wrote: > > What about if replace get_cpu() with migrate_disable()? > > Yeah, that would be a better option. I am just not used to think in RT > so migrate_disable didn't really come to my mind. Good. Let me rebase accordingly, new version is coming soon. Side note: migrate_disable() is by now implemented in !RT and used in highmem for instance (kmap_atomic()). Sebastian
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a3225501cce36..97a88b63ee983 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2108,6 +2108,7 @@ void unlock_page_memcg(struct page *page) } struct memcg_stock_pcp { + local_lock_t stock_lock; struct mem_cgroup *cached; /* this never be root cgroup */ unsigned int nr_pages; @@ -2123,18 +2124,21 @@ struct memcg_stock_pcp { unsigned long flags; #define FLUSHING_CACHED_CHARGE 0 }; -static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock); +static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock) = { + .stock_lock = INIT_LOCAL_LOCK(stock_lock), +}; static DEFINE_MUTEX(percpu_charge_mutex); #ifdef CONFIG_MEMCG_KMEM -static void drain_obj_stock(struct memcg_stock_pcp *stock); +static struct obj_cgroup *drain_obj_stock(struct memcg_stock_pcp *stock); static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, struct mem_cgroup *root_memcg); static void memcg_account_kmem(struct mem_cgroup *memcg, int nr_pages); #else -static inline void drain_obj_stock(struct memcg_stock_pcp *stock) +static inline struct obj_cgroup *drain_obj_stock(struct memcg_stock_pcp *stock) { + return NULL; } static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, struct mem_cgroup *root_memcg) @@ -2166,7 +2170,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages) if (nr_pages > MEMCG_CHARGE_BATCH) return ret; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); stock = this_cpu_ptr(&memcg_stock); if (memcg == stock->cached && stock->nr_pages >= nr_pages) { @@ -2174,7 +2178,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages) ret = true; } - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); return ret; } @@ -2203,6 +2207,7 @@ static void drain_stock(struct memcg_stock_pcp *stock) static void drain_local_stock(struct work_struct *dummy) { struct memcg_stock_pcp *stock; + struct obj_cgroup *old = NULL; unsigned long flags; /* @@ -2210,14 +2215,16 @@ static void drain_local_stock(struct work_struct *dummy) * drain_stock races is that we always operate on local CPU stock * here with IRQ disabled */ - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); stock = this_cpu_ptr(&memcg_stock); - drain_obj_stock(stock); + old = drain_obj_stock(stock); drain_stock(stock); clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); + if (old) + obj_cgroup_put(old); } /* @@ -2244,9 +2251,9 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) { unsigned long flags; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); __refill_stock(memcg, nr_pages); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); } /* @@ -2255,7 +2262,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) */ static void drain_all_stock(struct mem_cgroup *root_memcg) { - int cpu, curcpu; + int cpu; /* If someone's already draining, avoid adding running more workers. */ if (!mutex_trylock(&percpu_charge_mutex)) @@ -2266,7 +2273,6 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) * as well as workers from this path always operate on the local * per-cpu data. CPU up doesn't touch memcg_stock at all. */ - curcpu = get_cpu(); for_each_online_cpu(cpu) { struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); struct mem_cgroup *memcg; @@ -2282,14 +2288,9 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) rcu_read_unlock(); if (flush && - !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) { - if (cpu == curcpu) - drain_local_stock(&stock->work); - else - schedule_work_on(cpu, &stock->work); - } + !test_and_set_bit(FLUSHING_CACHED_CHARGE, &stock->flags)) + schedule_work_on(cpu, &stock->work); } - put_cpu(); mutex_unlock(&percpu_charge_mutex); } @@ -3073,10 +3074,11 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, enum node_stat_item idx, int nr) { struct memcg_stock_pcp *stock; + struct obj_cgroup *old = NULL; unsigned long flags; int *bytes; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); stock = this_cpu_ptr(&memcg_stock); /* @@ -3085,7 +3087,7 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, * changes. */ if (stock->cached_objcg != objcg) { - drain_obj_stock(stock); + old = drain_obj_stock(stock); obj_cgroup_get(objcg); stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes) ? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0; @@ -3129,7 +3131,9 @@ void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat, if (nr) mod_objcg_mlstate(objcg, pgdat, idx, nr); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); + if (old) + obj_cgroup_put(old); } static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) @@ -3138,7 +3142,7 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) unsigned long flags; bool ret = false; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); stock = this_cpu_ptr(&memcg_stock); if (objcg == stock->cached_objcg && stock->nr_bytes >= nr_bytes) { @@ -3146,17 +3150,17 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes) ret = true; } - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); return ret; } -static void drain_obj_stock(struct memcg_stock_pcp *stock) +static struct obj_cgroup *drain_obj_stock(struct memcg_stock_pcp *stock) { struct obj_cgroup *old = stock->cached_objcg; if (!old) - return; + return NULL; if (stock->nr_bytes) { unsigned int nr_pages = stock->nr_bytes >> PAGE_SHIFT; @@ -3206,8 +3210,8 @@ static void drain_obj_stock(struct memcg_stock_pcp *stock) stock->cached_pgdat = NULL; } - obj_cgroup_put(old); stock->cached_objcg = NULL; + return old; } static bool obj_stock_flush_required(struct memcg_stock_pcp *stock, @@ -3228,14 +3232,15 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, bool allow_uncharge) { struct memcg_stock_pcp *stock; + struct obj_cgroup *old = NULL; unsigned long flags; unsigned int nr_pages = 0; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.stock_lock, flags); stock = this_cpu_ptr(&memcg_stock); if (stock->cached_objcg != objcg) { /* reset if necessary */ - drain_obj_stock(stock); + old = drain_obj_stock(stock); obj_cgroup_get(objcg); stock->cached_objcg = objcg; stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes) @@ -3249,7 +3254,9 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes, stock->nr_bytes &= (PAGE_SIZE - 1); } - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.stock_lock, flags); + if (old) + obj_cgroup_put(old); if (nr_pages) obj_cgroup_uncharge_pages(objcg, nr_pages);
The members of the per-CPU structure memcg_stock_pcp are protected by disabling interrupts. This is not working on PREEMPT_RT because it creates atomic context in which actions are performed which require preemptible context. One example is obj_cgroup_release(). The IRQ-disable sections can be replaced with local_lock_t which preserves the explicit disabling of interrupts while keeps the code preemptible on PREEMPT_RT. drain_all_stock() disables preemption via get_cpu() and then invokes drain_local_stock() if it is the local CPU to avoid scheduling a worker (which invokes the same function). Disabling preemption here is problematic due to the sleeping locks in drain_local_stock(). This can be avoided by always scheduling a worker, even for the local CPU. Using cpus_read_lock() to stabilize the cpu_online_mask is not needed since the worker operates always on the CPU-local data structure. Should a CPU go offline then a two worker would perform the work and no harm is done. Using cpus_read_lock() leads to a possible deadlock. drain_obj_stock() drops a reference on obj_cgroup which leads to an invocation of obj_cgroup_release() if it is the last object. This in turn leads to recursive locking of the local_lock_t. To avoid this, obj_cgroup_release() is invoked outside of the locked section. obj_cgroup_uncharge_pages() can be invoked with the local_lock_t acquired and without it. This will lead later to a recursion in refill_stock(). To avoid the locking recursion provide obj_cgroup_uncharge_pages_locked() which uses the locked version of refill_stock(). - Replace disabling interrupts for memcg_stock with a local_lock_t. - Schedule a worker even for the local CPU instead of invoking it directly (in drain_all_stock()). - Let drain_obj_stock() return the old struct obj_cgroup which is passed to obj_cgroup_put() outside of the locked section. - Provide obj_cgroup_uncharge_pages_locked() which uses the locked version of refill_stock() to avoid recursive locking in drain_obj_stock(). Link: https://lkml.kernel.org/r/20220209014709.GA26885@xsang-OptiPlex-9020 Reported-by: kernel test robot <oliver.sang@intel.com> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- mm/memcontrol.c | 67 +++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 30 deletions(-)