From: Waiman Long <longman@redhat.com> To: Johannes Weiner <hannes@cmpxchg.org>, Michal Hocko <mhocko@kernel.org>, Vladimir Davydov <vdavydov.dev@gmail.com>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Sebastian Andrzej Siewior <bigeasy@linutronix.de>, Thomas Gleixner <tglx@linutronix.de>, Waiman Long <longman@redhat.com> Subject: [PATCH-next v2] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT Date: Thu, 9 Dec 2021 21:52:28 -0500 [thread overview] Message-ID: <20211210025228.158196-1-longman@redhat.com> (raw) Direct calls to local_irq_{save/restore}() and preempt_{enable/disable}() are not appropriate for PREEMPT_RT. To provide better PREEMPT_RT support, change local_irq_{save/restore}() to local_lock_irq{save/restore}() and add a local_lock_t to struct memcg_stock_pcp. Also disable the task and interrupt context optimization for obj_stock as there will be no performance gain in the case of PREEMPT_RT. In this case, task obj_stock will be there but remain unused. Note that preempt_enable() and preempt_disable() in get_obj_stock() and put_obj_stock() are not replaced by local_lock() and local_unlock() as it is possible that a task accessing task_obj may get interrupted and then access irq_obj concurrently. So using local_lock for task_obj access may cause lockdep splat. Signed-off-by: Waiman Long <longman@redhat.com> --- mm/memcontrol.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a09a7d2e0b1b..8bed8e2993e4 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2097,6 +2097,7 @@ struct obj_stock { }; struct memcg_stock_pcp { + local_lock_t lock; struct mem_cgroup *cached; /* this never be root cgroup */ unsigned int nr_pages; struct obj_stock task_obj; @@ -2145,7 +2146,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.lock, flags); stock = this_cpu_ptr(&memcg_stock); if (memcg == stock->cached && stock->nr_pages >= nr_pages) { @@ -2153,7 +2154,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages) ret = true; } - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); return ret; } @@ -2189,7 +2190,7 @@ 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.lock, flags); stock = this_cpu_ptr(&memcg_stock); drain_obj_stock(&stock->irq_obj); @@ -2198,7 +2199,7 @@ static void drain_local_stock(struct work_struct *dummy) drain_stock(stock); clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -2210,7 +2211,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) struct memcg_stock_pcp *stock; unsigned long flags; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.lock, flags); stock = this_cpu_ptr(&memcg_stock); if (stock->cached != memcg) { /* reset if necessary */ @@ -2223,7 +2224,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) if (stock->nr_pages > MEMCG_CHARGE_BATCH) drain_stock(stock); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -2779,29 +2780,28 @@ static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg) * which is cheap in non-preempt kernel. The interrupt context object stock * can only be accessed after disabling interrupt. User context code can * access interrupt object stock, but not vice versa. + * + * This task and interrupt context optimization is disabled for PREEMPT_RT + * as there is no performance gain in this case. */ static inline struct obj_stock *get_obj_stock(unsigned long *pflags) { - struct memcg_stock_pcp *stock; - - if (likely(in_task())) { + if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) { *pflags = 0UL; preempt_disable(); - stock = this_cpu_ptr(&memcg_stock); - return &stock->task_obj; + return this_cpu_ptr(&memcg_stock.task_obj); } - local_irq_save(*pflags); - stock = this_cpu_ptr(&memcg_stock); - return &stock->irq_obj; + local_lock_irqsave(&memcg_stock.lock, *pflags); + return this_cpu_ptr(&memcg_stock.irq_obj); } static inline void put_obj_stock(unsigned long flags) { - if (likely(in_task())) + if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) preempt_enable(); else - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -7088,9 +7088,12 @@ static int __init mem_cgroup_init(void) cpuhp_setup_state_nocalls(CPUHP_MM_MEMCQ_DEAD, "mm/memctrl:dead", NULL, memcg_hotplug_cpu_dead); - for_each_possible_cpu(cpu) - INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work, - drain_local_stock); + for_each_possible_cpu(cpu) { + struct memcg_stock_pcp *stock = per_cpu_ptr(&memcg_stock, cpu); + + INIT_WORK(&stock->work, drain_local_stock); + local_lock_init(&stock->lock); + } for_each_node(node) { struct mem_cgroup_tree_per_node *rtpn; -- 2.27.0
WARNING: multiple messages have this Message-ID (diff)
From: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Vladimir Davydov <vdavydov.dev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org> Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Subject: [PATCH-next v2] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT Date: Thu, 9 Dec 2021 21:52:28 -0500 [thread overview] Message-ID: <20211210025228.158196-1-longman@redhat.com> (raw) Direct calls to local_irq_{save/restore}() and preempt_{enable/disable}() are not appropriate for PREEMPT_RT. To provide better PREEMPT_RT support, change local_irq_{save/restore}() to local_lock_irq{save/restore}() and add a local_lock_t to struct memcg_stock_pcp. Also disable the task and interrupt context optimization for obj_stock as there will be no performance gain in the case of PREEMPT_RT. In this case, task obj_stock will be there but remain unused. Note that preempt_enable() and preempt_disable() in get_obj_stock() and put_obj_stock() are not replaced by local_lock() and local_unlock() as it is possible that a task accessing task_obj may get interrupted and then access irq_obj concurrently. So using local_lock for task_obj access may cause lockdep splat. Signed-off-by: Waiman Long <longman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> --- mm/memcontrol.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index a09a7d2e0b1b..8bed8e2993e4 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2097,6 +2097,7 @@ struct obj_stock { }; struct memcg_stock_pcp { + local_lock_t lock; struct mem_cgroup *cached; /* this never be root cgroup */ unsigned int nr_pages; struct obj_stock task_obj; @@ -2145,7 +2146,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.lock, flags); stock = this_cpu_ptr(&memcg_stock); if (memcg == stock->cached && stock->nr_pages >= nr_pages) { @@ -2153,7 +2154,7 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages) ret = true; } - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); return ret; } @@ -2189,7 +2190,7 @@ 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.lock, flags); stock = this_cpu_ptr(&memcg_stock); drain_obj_stock(&stock->irq_obj); @@ -2198,7 +2199,7 @@ static void drain_local_stock(struct work_struct *dummy) drain_stock(stock); clear_bit(FLUSHING_CACHED_CHARGE, &stock->flags); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -2210,7 +2211,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) struct memcg_stock_pcp *stock; unsigned long flags; - local_irq_save(flags); + local_lock_irqsave(&memcg_stock.lock, flags); stock = this_cpu_ptr(&memcg_stock); if (stock->cached != memcg) { /* reset if necessary */ @@ -2223,7 +2224,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages) if (stock->nr_pages > MEMCG_CHARGE_BATCH) drain_stock(stock); - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -2779,29 +2780,28 @@ static struct mem_cgroup *get_mem_cgroup_from_objcg(struct obj_cgroup *objcg) * which is cheap in non-preempt kernel. The interrupt context object stock * can only be accessed after disabling interrupt. User context code can * access interrupt object stock, but not vice versa. + * + * This task and interrupt context optimization is disabled for PREEMPT_RT + * as there is no performance gain in this case. */ static inline struct obj_stock *get_obj_stock(unsigned long *pflags) { - struct memcg_stock_pcp *stock; - - if (likely(in_task())) { + if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) { *pflags = 0UL; preempt_disable(); - stock = this_cpu_ptr(&memcg_stock); - return &stock->task_obj; + return this_cpu_ptr(&memcg_stock.task_obj); } - local_irq_save(*pflags); - stock = this_cpu_ptr(&memcg_stock); - return &stock->irq_obj; + local_lock_irqsave(&memcg_stock.lock, *pflags); + return this_cpu_ptr(&memcg_stock.irq_obj); } static inline void put_obj_stock(unsigned long flags) { - if (likely(in_task())) + if (likely(in_task()) && !IS_ENABLED(CONFIG_PREEMPT_RT)) preempt_enable(); else - local_irq_restore(flags); + local_unlock_irqrestore(&memcg_stock.lock, flags); } /* @@ -7088,9 +7088,12 @@ static int __init mem_cgroup_init(void) cpuhp_setup_state_nocalls(CPUHP_MM_MEMCQ_DEAD, "mm/memctrl:dead", NULL, memcg_hotplug_cpu_dead); - for_each_possible_cpu(cpu) - INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work, - drain_local_stock); + for_each_possible_cpu(cpu) { + struct memcg_stock_pcp *stock = per_cpu_ptr(&memcg_stock, cpu); + + INIT_WORK(&stock->work, drain_local_stock); + local_lock_init(&stock->lock); + } for_each_node(node) { struct mem_cgroup_tree_per_node *rtpn; -- 2.27.0
next reply other threads:[~2021-12-10 2:52 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-10 2:52 Waiman Long [this message] 2021-12-10 2:52 ` [PATCH-next v2] mm/memcg: Properly handle memcg_stock access for PREEMPT_RT Waiman Long 2021-12-10 13:01 ` Sebastian Andrzej Siewior 2021-12-10 13:01 ` Sebastian Andrzej Siewior 2021-12-10 16:29 ` Waiman Long 2021-12-10 16:29 ` Waiman Long 2021-12-10 16:34 ` Sebastian Andrzej Siewior 2021-12-10 16:34 ` Sebastian Andrzej Siewior 2021-12-10 16:37 ` Waiman Long
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211210025228.158196-1-longman@redhat.com \ --to=longman@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=bigeasy@linutronix.de \ --cc=cgroups@vger.kernel.org \ --cc=hannes@cmpxchg.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@kernel.org \ --cc=tglx@linutronix.de \ --cc=vdavydov.dev@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.