mm: slub: Fix slub_debug disablement for list of slabs

Commit Message

Vijayanand Jitta July 13, 2021, 11:45 a.m. UTC

From: Vijayanand Jitta <[e-mail]>

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 <[e-mail]>
---
 mm/slub.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

David Rientjes July 19, 2021, 6:21 a.m. UTC | #1

On Tue, 13 Jul 2021, [e-mail] wrote:

> From: Vijayanand Jitta <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  mm/slub.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index dc863c1..5a88418 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1429,6 +1429,7 @@ static int __init setup_slub_debug(char *str)
>  	char *slab_list;
>  	bool global_slub_debug_changed = false;
>  	bool slab_list_specified = false;
> +	bool slab_list_debug_disable = true;
>  
>  	slub_debug = DEBUG_DEFAULT_FLAGS;
>  	if (*str++ != '=' || !*str)
> @@ -1436,7 +1437,6 @@ static int __init setup_slub_debug(char *str)
>  		 * No options specified. Switch on full debugging.
>  		 */
>  		goto out;
> -
>  	saved_str = str;
>  	while (str) {
>  		str = parse_slub_debug_flags(str, &flags, &slab_list, true);
> @@ -1445,6 +1445,8 @@ static int __init setup_slub_debug(char *str)
>  			slub_debug = flags;
>  			global_slub_debug_changed = true;
>  		} else {
> +			if (flags || !IS_ENABLED(CONFIG_SLUB_DEBUG_ON))

Could you explain the check for CONFIG_SLUB_DEBUG_ON here?

I don't believe that using `-' for slub_debug is *only* useful when 
CONFIG_SLUB_DEBUG_ON is enabled.

> +				slab_list_debug_disable = false;
>  			slab_list_specified = true;
>  		}
>  	}
> @@ -1456,7 +1458,7 @@ static int __init setup_slub_debug(char *str)
>  	 * long as there is no option specifying flags without a slab list.
>  	 */
>  	if (slab_list_specified) {
> -		if (!global_slub_debug_changed)
> +		if (!global_slub_debug_changed && !slab_list_debug_disable)
>  			slub_debug = 0;
>  		slub_debug_string = saved_str;
>  	}

Vijayanand Jitta July 20, 2021, 6:35 a.m. UTC | #2

On 7/19/2021 11:51 AM, David Rientjes wrote:
> On Tue, 13 Jul 2021, [e-mail] wrote:
> 
>> From: Vijayanand Jitta <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  mm/slub.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index dc863c1..5a88418 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1429,6 +1429,7 @@ static int __init setup_slub_debug(char *str)
>>  	char *slab_list;
>>  	bool global_slub_debug_changed = false;
>>  	bool slab_list_specified = false;
>> +	bool slab_list_debug_disable = true;
>>  
>>  	slub_debug = DEBUG_DEFAULT_FLAGS;
>>  	if (*str++ != '=' || !*str)
>> @@ -1436,7 +1437,6 @@ static int __init setup_slub_debug(char *str)
>>  		 * No options specified. Switch on full debugging.
>>  		 */
>>  		goto out;
>> -
>>  	saved_str = str;
>>  	while (str) {
>>  		str = parse_slub_debug_flags(str, &flags, &slab_list, true);
>> @@ -1445,6 +1445,8 @@ static int __init setup_slub_debug(char *str)
>>  			slub_debug = flags;
>>  			global_slub_debug_changed = true;
>>  		} else {
>> +			if (flags || !IS_ENABLED(CONFIG_SLUB_DEBUG_ON))
> 
> Could you explain the check for CONFIG_SLUB_DEBUG_ON here?
>
Thanks for the review.

The check for CONFIG_SLUB_DEBUG_ON is for below scenario:

Consider CONFIG_SLUB_DEBUG_ON is not set and the below boot parameter is
set incorrectly "slub_debug=-,slab_name", Now without the additional
CONFIG_SLUB_DEBUG_ON check slab_list_debug_disable will still be true
and slub_debug will be set to DEBUG_DEFAULT_FLAGS instead of 0 as the
below check fails.

if (!global_slub_debug_changed && !slab_list_debug_disable)
	slub_debug = 0;

> I don't believe that using `-' for slub_debug is *only* useful when 
> CONFIG_SLUB_DEBUG_ON is enabled.
> 

'-' is still useful in CONFIG_SLUB_DEBUG_ON disabled case even with the
above check, all the possible cases using '-' work as expected.

i.e;
slub_debug=-.slab_name
slub_debug=flag,slab_name;-,slab_name

Thanks,
Vijay

>> +				slab_list_debug_disable = false;
>>  			slab_list_specified = true;
>>  		}
>>  	}
>> @@ -1456,7 +1458,7 @@ static int __init setup_slub_debug(char *str)
>>  	 * long as there is no option specifying flags without a slab list.
>>  	 */
>>  	if (slab_list_specified) {
>> -		if (!global_slub_debug_changed)
>> +		if (!global_slub_debug_changed && !slab_list_debug_disable)
>>  			slub_debug = 0;
>>  		slub_debug_string = saved_str;
>>  	}

Vlastimil Babka July 26, 2021, 10:32 p.m. UTC | #3

On 7/13/21 1:45 PM, [e-mail] wrote:
> From: Vijayanand Jitta <[e-mail]>
> 
> 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 <[e-mail]>

Would the following work too, and perhaps be easier to follow?
----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

Vijayanand Jitta July 27, 2021, 4:43 a.m. UTC | #4

On 7/27/2021 4:02 AM, Vlastimil Babka wrote:
> On 7/13/21 1:45 PM, [e-mail] wrote:
>> From: Vijayanand Jitta <[e-mail]>
>>
>> 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 <[e-mail]>
> 
> 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 <[e-mail]>

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
>

Vlastimil Babka July 27, 2021, 12:22 p.m. UTC | #5

On 7/27/21 6:43 AM, Vijayanand Jitta wrote:
> 
> 
> On 7/27/2021 4:02 AM, Vlastimil Babka wrote:
>> On 7/13/21 1:45 PM, [e-mail] wrote:
>>> From: Vijayanand Jitta <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>
>> 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 <[e-mail]>

Thanks! Here's the full patch
----8<----
From 81a225fe31e53701902bb4caa9ab1524eb044cbc Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <[e-mail]>
Date: Tue, 13 Jul 2021 17:15:50 +0530
Subject: [PATCH] mm: slub: fix slub_debug disabling for list of slabs

Vijayanand Jitta reports:

  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, which is wrong.

This patch fixes it by delaying the moment when the global slub_debug flags
variable is updated. In case a "slub_debug=-,slab_name" has been passed, the
global flags remain as initialized (depending on CONFIG_SLUB_DEBUG_ON enabled
or disabled) and are not simply reset to 0.

Reported-by: Vijayanand Jitta <[e-mail]>
Signed-off-by: Vlastimil Babka <[e-mail]>
Reviewed-by: Vijayanand Jitta <[e-mail]>
---
 mm/slub.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

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

David Rientjes Aug. 2, 2021, 3:26 a.m. UTC | #6

On Tue, 27 Jul 2021, Vlastimil Babka wrote:

> From 81a225fe31e53701902bb4caa9ab1524eb044cbc Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <[e-mail]>
> Date: Tue, 13 Jul 2021 17:15:50 +0530
> Subject: [PATCH] mm: slub: fix slub_debug disabling for list of slabs
> 
> Vijayanand Jitta reports:
> 
>   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, which is wrong.
> 
> This patch fixes it by delaying the moment when the global slub_debug flags
> variable is updated. In case a "slub_debug=-,slab_name" has been passed, the
> global flags remain as initialized (depending on CONFIG_SLUB_DEBUG_ON enabled
> or disabled) and are not simply reset to 0.
> 
> Reported-by: Vijayanand Jitta <[e-mail]>
> Signed-off-by: Vlastimil Babka <[e-mail]>
> Reviewed-by: Vijayanand Jitta <[e-mail]>

Acked-by: David Rientjes <[e-mail]>

Looks better, thanks!

Patch

diff --git a/mm/slub.c b/mm/slub.c
index dc863c1..5a88418 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1429,6 +1429,7 @@  static int __init setup_slub_debug(char *str)
 	char *slab_list;
 	bool global_slub_debug_changed = false;
 	bool slab_list_specified = false;
+	bool slab_list_debug_disable = true;
 
 	slub_debug = DEBUG_DEFAULT_FLAGS;
 	if (*str++ != '=' || !*str)
@@ -1436,7 +1437,6 @@  static int __init setup_slub_debug(char *str)
 		 * No options specified. Switch on full debugging.
 		 */
 		goto out;
-
 	saved_str = str;
 	while (str) {
 		str = parse_slub_debug_flags(str, &flags, &slab_list, true);
@@ -1445,6 +1445,8 @@  static int __init setup_slub_debug(char *str)
 			slub_debug = flags;
 			global_slub_debug_changed = true;
 		} else {
+			if (flags || !IS_ENABLED(CONFIG_SLUB_DEBUG_ON))
+				slab_list_debug_disable = false;
 			slab_list_specified = true;
 		}
 	}
@@ -1456,7 +1458,7 @@  static int __init setup_slub_debug(char *str)
 	 * long as there is no option specifying flags without a slab list.
 	 */
 	if (slab_list_specified) {
-		if (!global_slub_debug_changed)
+		if (!global_slub_debug_changed && !slab_list_debug_disable)
 			slub_debug = 0;
 		slub_debug_string = saved_str;
 	}