2021-07-13 11:48:04

by Vijayanand Jitta

[permalink] [raw]
Subject: [PATCH] mm: slub: Fix slub_debug disablement for list of slabs

From: Vijayanand Jitta <[email protected]>

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 <[email protected]>
---
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))
+ 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;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
2.7.4


2021-07-19 06:23:15

by David Rientjes

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

On Tue, 13 Jul 2021, [email protected] wrote:

> From: Vijayanand Jitta <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
> }

2021-07-20 06:38:23

by Vijayanand Jitta

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



On 7/19/2021 11:51 AM, David Rientjes wrote:
> On Tue, 13 Jul 2021, [email protected] wrote:
>
>> From: Vijayanand Jitta <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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;
>> }

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

2021-07-26 22:33:52

by Vlastimil Babka

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

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

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
--
2.32.0

2021-07-27 04:47:35

by Vijayanand Jitta

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



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

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