Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 10 Sep 2021 10:35:17 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Feng Tang <feng.tang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        David Rientjes <rientjes@google.com>,
        Mel Gorman <mgorman@techsingularity.net>,
        Vlastimil Babka <vbabka@suse.cz>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/page_alloc: detect allocation forbidden by cpuset and
 bail out early
Message-ID: <YTsYxbMhGIunUPZr@dhcp22.suse.cz>
References: <1631003150-96935-1-git-send-email-feng.tang@intel.com>
 <YTcmcEUmtO++WeBk@dhcp22.suse.cz>
 <20210908015014.GA28091@shbuild999.sh.intel.com>
 <YThg8Mp42b194k0/@dhcp22.suse.cz>
 <20210910074400.GA18707@shbuild999.sh.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210910074400.GA18707@shbuild999.sh.intel.com>
Precedence: bulk

On Fri 10-09-21 15:44:00, Feng Tang wrote:
> On Wed, Sep 08, 2021 at 09:06:24AM +0200, Michal Hocko wrote:
> > On Wed 08-09-21 09:50:14, Feng Tang wrote:
> > > On Tue, Sep 07, 2021 at 10:44:32AM +0200, Michal Hocko wrote:
> > [...]
> > > > While this is a good fix from the functionality POV I believe you can go
> > > > a step further. Please add a detection to the cpuset code and complain
> > > > to the kernel log if somebody tries to configure movable only cpuset.
> > > > Once you have that in place you can easily create a static branch for
> > > > cpuset_insane_setup() and have zero overhead for all reasonable
> > > > configuration. There shouldn't be any reason to pay a single cpu cycle
> > > > to check for something that almost nobody does.
> > > > 
> > > > What do you think?
> > > 
> > > I thought about the implementation, IIUC, the static_branch_enable() is
> > > easy, it could be done when cpuset.mems is set with movable only nodes,
> > > but disable() is much complexer,
> > 
> > Do we care about disable at all? The point is to not have 99,999999%
> > users pay overhead of the check which is irrelevant to them. Once
> > somebody wants to use this "creative" setup then paying an extra check
> > sounds perfectly sensible to me. If somebody cares enough then the
> > disable logic could be implemented. But for now I believe we should be
> > OK with only enable case.
> 
> Here is tested draft patch to add the check in cpuset code (the looping
> zone code could be improved by adding a for_each_populated_zone_nodemask
> macro.
> 
> Thanks,
> Feng
> 
> ---
>  include/linux/cpuset.h |  7 +++++++
>  include/linux/mmzone.h | 14 ++++++++++++++
>  kernel/cgroup/cpuset.c | 10 ++++++++++
>  mm/page_alloc.c        |  4 +++-
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
> index d2b9c41..a434985 100644
> --- a/include/linux/cpuset.h
> +++ b/include/linux/cpuset.h
> @@ -34,6 +34,8 @@
>   */
>  extern struct static_key_false cpusets_pre_enable_key;
>  extern struct static_key_false cpusets_enabled_key;
> +extern struct static_key_false cpusets_abnormal_setup_key;
> +
>  static inline bool cpusets_enabled(void)
>  {
>  	return static_branch_unlikely(&cpusets_enabled_key);
> @@ -51,6 +53,11 @@ static inline void cpuset_dec(void)
>  	static_branch_dec_cpuslocked(&cpusets_pre_enable_key);
>  }
>  
> +static inline bool cpusets_abnormal_check_needed(void)

I would go with cpusets_insane_config with a comment explaining what
that means. I would also do a pr_info() when the static branch is
enabled.

[...]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4e455fa..5728675 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4919,7 +4919,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	 * any suitable zone to satisfy the request - e.g. non-movable
>  	 * GFP_HIGHUSER allocations from MOVABLE nodes only.
>  	 */
> -	if (cpusets_enabled() && (gfp_mask & __GFP_HARDWALL)) {
> +	if (cpusets_enabled() &&
> +		cpusets_abnormal_check_needed() &&

You do not need cpusets_enabled check here. Remember the primary point
is to not introduce any branch unless a dubious configuration is in
place.

> +		(gfp_mask & __GFP_HARDWALL)) {
>  		struct zoneref *z = first_zones_zonelist(ac->zonelist,
>  					ac->highest_zoneidx,
>  					&cpuset_current_mems_allowed);
> -- 
> 2.7.4
> 

-- 
Michal Hocko
SUSE Labs