Re: [PATCH 1/4] mm/memcg: Disable threshold event handlers on PREEMPT_RT

From: Michal Hocko <mhocko@suse.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: cgroups@vger.kernel.org, linux-mm@kvack.org,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Johannes Weiner" <hannes@cmpxchg.org>,
	"Michal Koutný" <mkoutny@suse.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Vladimir Davydov" <vdavydov.dev@gmail.com>,
	"Waiman Long" <longman@redhat.com>
Subject: Re: [PATCH 1/4] mm/memcg: Disable threshold event handlers on PREEMPT_RT
Date: Wed, 26 Jan 2022 15:40:54 +0100	[thread overview]
Message-ID: <YfFddqkAhd1YKqX9@dhcp22.suse.cz> (raw)
In-Reply-To: <20220125164337.2071854-2-bigeasy@linutronix.de>

On Tue 25-01-22 17:43:34, Sebastian Andrzej Siewior wrote:
> During the integration of PREEMPT_RT support, the code flow around
> memcg_check_events() resulted in `twisted code'. Moving the code around
> and avoiding then would then lead to an additional local-irq-save
> section within memcg_check_events(). While looking better, it adds a
> local-irq-save section to code flow which is usually within an
> local-irq-off block on non-PREEMPT_RT configurations.
> 
> The threshold event handler is a deprecated memcg v1 feature. Instead of
> trying to get it to work under PREEMPT_RT just disable it. There should
> be no users on PREEMPT_RT. From that perspective it makes even less
> sense to get it to work under PREEMPT_RT while having zero users.
> 
> Make memory.soft_limit_in_bytes and cgroup.event_control return
> -EOPNOTSUPP on PREEMPT_RT. Make an empty memcg_check_events() and
> memcg_write_event_control() which return only -EOPNOTSUPP on PREEMPT_RT.
> Document that the two knobs are disabled on PREEMPT_RT. Shuffle the code around
> so that all unused function are in on #ifdef block.
> 
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Suggested-by: Michal Koutný <mkoutny@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I still support this approach but the patch is much larger than
necessary. The code moving shouldn't be really necessary and a simple
"do not allow" to set any thresholds or soft limit should be good
enough. 

While in general it is better to disable the unreachable code I do not
think this is worth the code churn here.

> ---
>  .../admin-guide/cgroup-v1/memory.rst          |   2 +
>  mm/memcontrol.c                               | 788 +++++++++---------
>  2 files changed, 404 insertions(+), 386 deletions(-)
-- 
Michal Hocko
SUSE Labs