| Message ID | 20220221182540.380526-7-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcg: Address PREEMPT_RT problems instead of disabling it. | expand |
On Mon 21-02-22 19:25:40, Sebastian Andrzej Siewior wrote: > Before the for-each-CPU loop, preemption is disabled so that so that > drain_local_stock() can be invoked directly instead of scheduling a > worker. Ensuring that drain_local_stock() completed on the local CPU is > not correctness problem. It _could_ be that the charging path will be > forced to reclaim memory because cached charges are still waiting for > their draining. > > Disabling preemption before invoking drain_local_stock() is problematic > on PREEMPT_RT due to the sleeping locks involved. To ensure that no CPU > migrations happens across the for_each_online_cpu() it is enouhg to use > migrate_disable() which disables migration and keeps context preemptible > to a sleeping lock can be acquired. I would just add that a race with cpu hotplug is not a problem because pcp data is not going away. In the worst case we just schedule draining of an empty stock. > > Use migrate_disable() instead of get_cpu() around the > for_each_online_cpu() loop. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/memcontrol.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 3d7ccb104374c..9c29b1a0e6bec 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2293,7 +2293,8 @@ 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(); > + migrate_disable(); > + curcpu = smp_processor_id(); > for_each_online_cpu(cpu) { > struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); > struct mem_cgroup *memcg; > @@ -2316,7 +2317,7 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) > schedule_work_on(cpu, &stock->work); > } > } > - put_cpu(); > + migrate_enable(); > mutex_unlock(&percpu_charge_mutex); > } > > -- > 2.35.1
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 3d7ccb104374c..9c29b1a0e6bec 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2293,7 +2293,8 @@ 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(); + migrate_disable(); + curcpu = smp_processor_id(); for_each_online_cpu(cpu) { struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu); struct mem_cgroup *memcg; @@ -2316,7 +2317,7 @@ static void drain_all_stock(struct mem_cgroup *root_memcg) schedule_work_on(cpu, &stock->work); } } - put_cpu(); + migrate_enable(); mutex_unlock(&percpu_charge_mutex); }
Before the for-each-CPU loop, preemption is disabled so that so that drain_local_stock() can be invoked directly instead of scheduling a worker. Ensuring that drain_local_stock() completed on the local CPU is not correctness problem. It _could_ be that the charging path will be forced to reclaim memory because cached charges are still waiting for their draining. Disabling preemption before invoking drain_local_stock() is problematic on PREEMPT_RT due to the sleeping locks involved. To ensure that no CPU migrations happens across the for_each_online_cpu() it is enouhg to use migrate_disable() which disables migration and keeps context preemptible to a sleeping lock can be acquired. Use migrate_disable() instead of get_cpu() around the for_each_online_cpu() loop. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- mm/memcontrol.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)