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;
Sender: vjitta=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 6CA9DC4323A
Subject: Re: [PATCH] mm: slub: Fix slub_debug disablement for list of slabs
To:     Vlastimil Babka <vbabka@suse.cz>, cl@linux.com, penberg@kernel.org,
        rientjes@google.com, iamjoonsoo.kim@lge.com,
        akpm@linux-foundation.org
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        vinmenon@codeaurora.org
References: <1626176750-13099-1-git-send-email-vjitta@codeaurora.org>
 <bf2a8571-325c-6d94-0d5a-f6df71ae0c4f@suse.cz>
From:   Vijayanand Jitta <vjitta@codeaurora.org>
Message-ID: <e0442add-dcf0-57d7-2298-3459136673af@codeaurora.org>
Date:   Tue, 27 Jul 2021 10:13:16 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <bf2a8571-325c-6d94-0d5a-f6df71ae0c4f@suse.cz>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/27/2021 4:02 AM, Vlastimil Babka wrote:
> On 7/13/21 1:45 PM, vjitta@codeaurora.org wrote:
>> From: Vijayanand Jitta <vjitta@codeaurora.org>
>>
>> Consider the scenario where CONFIG_SLUB_DEBUG_ON is set
>> and we would want to disable slub_debug for few slabs.
>> Using boot parameter with slub_debug=-,slab_name syntax
>> doesn't work as expected i.e; only disabling debugging for
>> the specified list of slabs, instead it disables debugging
>> for all slabs. Fix this.
>>
>> Signed-off-by: Vijayanand Jitta <vjitta@codeaurora.org>
> 
> Would the following work too, and perhaps be easier to follow?

Right, the below change would also work and its easier to follow as you
said. We can go with this.

Reviewed-by: Vijayanand Jitta <vjitta@codeaurora.org>

Thanks,
Vijay

> ----8<----
> diff --git a/mm/slub.c b/mm/slub.c
> index 090fa14628f9..024f49706386 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1400,12 +1400,13 @@ parse_slub_debug_flags(char *str, slab_flags_t *flags, char **slabs, bool init)
>  static int __init setup_slub_debug(char *str)
>  {
>  	slab_flags_t flags;
> +	slab_flags_t global_flags;
>  	char *saved_str;
>  	char *slab_list;
>  	bool global_slub_debug_changed = false;
>  	bool slab_list_specified = false;
>  
> -	slub_debug = DEBUG_DEFAULT_FLAGS;
> +	global_flags = DEBUG_DEFAULT_FLAGS;
>  	if (*str++ != '=' || !*str)
>  		/*
>  		 * No options specified. Switch on full debugging.
> @@ -1417,7 +1418,7 @@ static int __init setup_slub_debug(char *str)
>  		str = parse_slub_debug_flags(str, &flags, &slab_list, true);
>  
>  		if (!slab_list) {
> -			slub_debug = flags;
> +			global_flags = flags;
>  			global_slub_debug_changed = true;
>  		} else {
>  			slab_list_specified = true;
> @@ -1426,16 +1427,18 @@ static int __init setup_slub_debug(char *str)
>  
>  	/*
>  	 * For backwards compatibility, a single list of flags with list of
> -	 * slabs means debugging is only enabled for those slabs, so the global
> -	 * slub_debug should be 0. We can extended that to multiple lists as
> +	 * slabs means debugging is only changed for those slabs, so the global
> +	 * slub_debug should be unchanged (0 or DEBUG_DEFAULT_FLAGS, depending
> +	 * on CONFIG_SLUB_DEBUG_ON). We can extended that to multiple lists as
>  	 * long as there is no option specifying flags without a slab list.
>  	 */
>  	if (slab_list_specified) {
>  		if (!global_slub_debug_changed)
> -			slub_debug = 0;
> +			global_flags = slub_debug;
>  		slub_debug_string = saved_str;
>  	}
>  out:
> +	slub_debug = global_flags;
>  	if (slub_debug != 0 || slub_debug_string)
>  		static_branch_enable(&slub_debug_enabled);
>  	else
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation