Re: [PATCH] mm: slub: Fix slub_debug disablement for list of slabs

[Vijayanand Jitta <vjitta@codeaurora.org>]

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