diff mbox series

cgroup/cpuset: Adjust exception handling in generate_sched_domains()

Message ID 53be5f98-6359-48b5-955e-fd203d99d3cb@web.de
State New
Headers
Series cgroup/cpuset: Adjust exception handling in generate_sched_domains() |

Commit Message

Markus Elfring Dec. 31, 2023, 7:28 a.m. UTC 
  From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 31 Dec 2023 08:00:31 +0100

Two resource allocation failures triggered further actions
over the label “done” so far.

* Jump to the statement “ndoms = 1;” in three cases directly
  by using the label “set_ndoms” instead.

* Delete an assignment for the variable “ndoms” in one if branch.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 kernel/cgroup/cpuset.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--
2.43.0

Comments

Waiman Long Dec. 31, 2023, 6:29 p.m. UTC | #1 
On 12/31/23 02:28, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 08:00:31 +0100
>
> Two resource allocation failures triggered further actions
> over the label “done” so far.
>
> * Jump to the statement “ndoms = 1;” in three cases directly
>    by using the label “set_ndoms” instead.
>
> * Delete an assignment for the variable “ndoms” in one if branch.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   kernel/cgroup/cpuset.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index ba36c073304a..29ccd52eb45c 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -973,10 +973,9 @@ static int generate_sched_domains(cpumask_var_t **domains,
>
>   	/* Special case for the 99% of systems with one, full, sched domain */
>   	if (root_load_balance && !top_cpuset.nr_subparts) {
> -		ndoms = 1;
>   		doms = alloc_sched_domains(ndoms);
>   		if (!doms)
> -			goto done;
> +			goto set_ndoms;
>
>   		dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
>   		if (dattr) {
> @@ -986,12 +985,12 @@ static int generate_sched_domains(cpumask_var_t **domains,
>   		cpumask_and(doms[0], top_cpuset.effective_cpus,
>   			    housekeeping_cpumask(HK_TYPE_DOMAIN));
>
> -		goto done;
> +		goto set_ndoms;
>   	}
>
>   	csa = kmalloc_array(nr_cpusets(), sizeof(cp), GFP_KERNEL);
>   	if (!csa)
> -		goto done;
> +		goto set_ndoms;
>   	csn = 0;
>
>   	rcu_read_lock();
> @@ -1123,6 +1122,7 @@ static int generate_sched_domains(cpumask_var_t **domains,
>   	 * See comments in partition_sched_domains().
>   	 */
>   	if (doms == NULL)
> +set_ndoms:
>   		ndoms = 1;
>
>   	*domains    = doms;
> --
> 2.43.0
>
Please clarify what this patch is for. Is it just a cleanup with no 
functional changes or is there a bug that is being fixed?

Cheers,
Longman
Markus Elfring Jan. 1, 2024, 8:56 a.m. UTC | #2 
>> Two resource allocation failures triggered further actions
>> over the label “done” so far.
>>
>> * Jump to the statement “ndoms = 1;” in three cases directly
>>    by using the label “set_ndoms” instead.
>>
>> * Delete an assignment for the variable “ndoms” in one if branch.
…
>> ---
>>   kernel/cgroup/cpuset.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
…
>> @@ -973,10 +973,9 @@ static int generate_sched_domains(cpumask_var_t **domains,
>>
>>       /* Special case for the 99% of systems with one, full, sched domain */
>>       if (root_load_balance && !top_cpuset.nr_subparts) {
>> -        ndoms = 1;
>>           doms = alloc_sched_domains(ndoms);
>>           if (!doms)
>> -            goto done;
>> +            goto set_ndoms;
>>
>>           dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
>>           if (dattr) {
…
>> @@ -1123,6 +1122,7 @@ static int generate_sched_domains(cpumask_var_t **domains,
>>        * See comments in partition_sched_domains().
>>        */
>>       if (doms == NULL)
>> +set_ndoms:
>>           ndoms = 1;
>>
>>       *domains    = doms;
…
> Please clarify what this patch is for. Is it just a cleanup with no functional changes or is there a bug that is being fixed?

The development opinions might vary for the presented transformation.
I suggest to reconsider the number of relevant variable assignments here.
Would you categorise an extra statement still as a desirable implementation detail?

Regards,
Markus
Waiman Long Jan. 1, 2024, 4:26 p.m. UTC | #3 
On 1/1/24 03:56, Markus Elfring wrote:
>>> Two resource allocation failures triggered further actions
>>> over the label “done” so far.
>>>
>>> * Jump to the statement “ndoms = 1;” in three cases directly
>>>     by using the label “set_ndoms” instead.
>>>
>>> * Delete an assignment for the variable “ndoms” in one if branch.
> …
>>> ---
>>>    kernel/cgroup/cpuset.c | 8 ++++----
>>>    1 file changed, 4 insertions(+), 4 deletions(-)
> …
>>> @@ -973,10 +973,9 @@ static int generate_sched_domains(cpumask_var_t **domains,
>>>
>>>        /* Special case for the 99% of systems with one, full, sched domain */
>>>        if (root_load_balance && !top_cpuset.nr_subparts) {
>>> -        ndoms = 1;
>>>            doms = alloc_sched_domains(ndoms);
>>>            if (!doms)
>>> -            goto done;
>>> +            goto set_ndoms;
>>>
>>>            dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
>>>            if (dattr) {
> …
>>> @@ -1123,6 +1122,7 @@ static int generate_sched_domains(cpumask_var_t **domains,
>>>         * See comments in partition_sched_domains().
>>>         */
>>>        if (doms == NULL)
>>> +set_ndoms:
>>>            ndoms = 1;
>>>
>>>        *domains    = doms;
> …
>> Please clarify what this patch is for. Is it just a cleanup with no functional changes or is there a bug that is being fixed?
> The development opinions might vary for the presented transformation.
> I suggest to reconsider the number of relevant variable assignments here.
> Would you categorise an extra statement still as a desirable implementation detail?

My understanding of the patch is just to avoid an unnecessary call to 
kfree() as kfree(NULL) is basically a NOP. By jumping inside the "if" 
part of the conditional statement, however, it makes it a bit harder to 
read. As generate_sched_domains() is not in the fast path, I would 
rather have a patch that can simplify the logic than to make it harder 
to understand.

Cheers,
Longman
kernel test robot Jan. 10, 2024, 3:01 p.m. UTC | #4 
Hello,

kernel test robot noticed "canonical_address#:#[##]" on:

commit: 85f67f5f644dba9e15b9fa957f4af4745a330157 ("[PATCH] cgroup/cpuset: Adjust exception handling in generate_sched_domains()")
url: https://github.com/intel-lab-lkp/linux/commits/Markus-Elfring/cgroup-cpuset-Adjust-exception-handling-in-generate_sched_domains/20231231-152941
base: https://git.kernel.org/cgit/linux/kernel/git/tj/cgroup.git for-next
patch link: https://lore.kernel.org/all/53be5f98-6359-48b5-955e-fd203d99d3cb@web.de/
patch subject: [PATCH] cgroup/cpuset: Adjust exception handling in generate_sched_domains()

in testcase: cpu-hotplug
version: 
with following parameters:




compiler: gcc-12
test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz (Kaby Lake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202401102216.c93598af-oliver.sang@intel.com


[   32.391748][  T310] RESULT_ROOT=/result/cpu-hotplug/defaults/lkp-kbl-d01/debian-11.1-x86_64-20220510.cgz/x86_64-rhel-8.3-func/gcc-12/85f67f5f644dba9e15b9fa957f4af4745a330157/0
[   32.391787][  T310]
[   32.412091][  T310] job=/lkp/jobs/scheduled/lkp-kbl-d01/cpu-hotplug-defaults-debian-11.1-x86_64-20220510.cgz-85f67f5f644d-20240110-32485-1y7a2hd-0.yaml
[   32.412128][  T310]
[   32.449795][    T8] smpboot: CPU 1 is now offline
[   32.455650][  T230] general protection fault, probably for non-canonical address 0xdffffc0000000002: 0000 [#1] PREEMPT SMP KASAN PTI
[   32.467476][  T230] KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
[   32.475678][  T230] CPU: 0 PID: 230 Comm: kworker/1:2 Not tainted 6.7.0-rc1-00370-g85f67f5f644d #1
[   32.484571][  T230] Hardware name: Dell Inc. OptiPlex 7050/062KRH, BIOS 1.2.0 12/22/2016
[   32.492601][  T230] Workqueue: events cpuset_hotplug_workfn
[ 32.498139][ T230] RIP: 0010:generate_sched_domains (kbuild/src/consumer/kernel/cgroup/cpuset.c:985) 
[ 32.504106][ T230] Code: b8 ff ff bf 05 00 00 00 e8 98 60 e6 ff 48 8b 54 24 20 4c 8b 25 64 fb 71 03 48 89 c3 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 02 00 0f 85 a1 08 00 00 48 ba 00 00 00 00 00 fc ff df 48 8b
All code
========
   0:	b8 ff ff bf 05       	mov    $0x5bfffff,%eax
   5:	00 00                	add    %al,(%rax)
   7:	00 e8                	add    %ch,%al
   9:	98                   	cwtl
   a:	60                   	(bad)
   b:	e6 ff                	out    %al,$0xff
   d:	48 8b 54 24 20       	mov    0x20(%rsp),%rdx
  12:	4c 8b 25 64 fb 71 03 	mov    0x371fb64(%rip),%r12        # 0x371fb7d
  19:	48 89 c3             	mov    %rax,%rbx
  1c:	48 b8 00 00 00 00 00 	movabs $0xdffffc0000000000,%rax
  23:	fc ff df 
  26:	48 c1 ea 03          	shr    $0x3,%rdx
  2a:*	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)		<-- trapping instruction
  2e:	0f 85 a1 08 00 00    	jne    0x8d5
  34:	48 ba 00 00 00 00 00 	movabs $0xdffffc0000000000,%rdx
  3b:	fc ff df 
  3e:	48                   	rex.W
  3f:	8b                   	.byte 0x8b

Code starting with the faulting instruction
===========================================
   0:	80 3c 02 00          	cmpb   $0x0,(%rdx,%rax,1)
   4:	0f 85 a1 08 00 00    	jne    0x8ab
   a:	48 ba 00 00 00 00 00 	movabs $0xdffffc0000000000,%rdx
  11:	fc ff df 
  14:	48                   	rex.W
  15:	8b                   	.byte 0x8b
[   32.523422][  T230] RSP: 0000:ffffc900009afb58 EFLAGS: 00010202
[   32.529307][  T230] RAX: dffffc0000000000 RBX: ffffffff8546fa20 RCX: 0000000000000004
[   32.537078][  T230] RDX: 0000000000000002 RSI: ffffffff84bd2ac0 RDI: 0000000000000005
[   32.544847][  T230] RBP: ffff88810c9e4e10 R08: 0000000000000000 R09: fffffbfff097a571
[   32.552619][  T230] R10: ffffffff84bd2b8f R11: ffffffff819c9c87 R12: ffff88810c9e4c30
[   32.560388][  T230] R13: ffffffff85470010 R14: 1ffff92000135f84 R15: dffffc0000000000
[   32.568159][  T230] FS:  0000000000000000(0000) GS:ffff8887a5600000(0000) knlGS:0000000000000000
[   32.576877][  T230] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   32.583270][  T230] CR2: 000055eb6b0d5000 CR3: 000000081ac5a001 CR4: 00000000003706f0
[   32.591040][  T230] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   32.598811][  T230] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   32.606583][  T230] Call Trace:
[   32.609699][  T230]  <TASK>
[ 32.612477][ T230] ? die_addr (kbuild/src/consumer/arch/x86/kernel/dumpstack.c:421 kbuild/src/consumer/arch/x86/kernel/dumpstack.c:460) 
[ 32.616462][ T230] ? exc_general_protection (kbuild/src/consumer/arch/x86/kernel/traps.c:700 kbuild/src/consumer/arch/x86/kernel/traps.c:642) 
[ 32.621824][ T230] ? asm_exc_general_protection (kbuild/src/consumer/arch/x86/include/asm/idtentry.h:564) 
[ 32.627357][ T230] ? __kasan_kmalloc (kbuild/src/consumer/mm/kasan/common.c:374 kbuild/src/consumer/mm/kasan/common.c:383) 


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240110/202401102216.c93598af-oliver.sang@intel.com
Waiman Long Jan. 10, 2024, 3:21 p.m. UTC | #5 
On 12/31/23 02:28, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 31 Dec 2023 08:00:31 +0100
>
> Two resource allocation failures triggered further actions
> over the label “done” so far.
>
> * Jump to the statement “ndoms = 1;” in three cases directly
>    by using the label “set_ndoms” instead.
>
> * Delete an assignment for the variable “ndoms” in one if branch.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   kernel/cgroup/cpuset.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index ba36c073304a..29ccd52eb45c 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -973,10 +973,9 @@ static int generate_sched_domains(cpumask_var_t **domains,
>
>   	/* Special case for the 99% of systems with one, full, sched domain */
>   	if (root_load_balance && !top_cpuset.nr_subparts) {
> -		ndoms = 1;

This statement cannot be removed as it is used in the following 
statement to size the allocation. This change the behavior to make doms 
NULL rather than a 1-entry array. This is probably the cause of the 
kernel test robot failure report.

NAK

>   		doms = alloc_sched_domains(ndoms);
>   		if (!doms)
> -			goto done;
> +			goto set_ndoms;
>
>   		dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
>   		if (dattr) {
> @@ -986,12 +985,12 @@ static int generate_sched_domains(cpumask_var_t **domains,
>   		cpumask_and(doms[0], top_cpuset.effective_cpus,
>   			    housekeeping_cpumask(HK_TYPE_DOMAIN));
>
> -		goto done;
> +		goto set_ndoms;
>   	}
>
>   	csa = kmalloc_array(nr_cpusets(), sizeof(cp), GFP_KERNEL);
>   	if (!csa)
> -		goto done;
> +		goto set_ndoms;
>   	csn = 0;
>
>   	rcu_read_lock();
> @@ -1123,6 +1122,7 @@ static int generate_sched_domains(cpumask_var_t **domains,
>   	 * See comments in partition_sched_domains().
>   	 */
>   	if (doms == NULL)
> +set_ndoms:
>   		ndoms = 1;
>
>   	*domains    = doms;
> --
> 2.43.0
>
>
diff mbox series

Patch

diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
index ba36c073304a..29ccd52eb45c 100644
--- a/kernel/cgroup/cpuset.c
+++ b/kernel/cgroup/cpuset.c
@@ -973,10 +973,9 @@  static int generate_sched_domains(cpumask_var_t **domains,

 	/* Special case for the 99% of systems with one, full, sched domain */
 	if (root_load_balance && !top_cpuset.nr_subparts) {
-		ndoms = 1;
 		doms = alloc_sched_domains(ndoms);
 		if (!doms)
-			goto done;
+			goto set_ndoms;

 		dattr = kmalloc(sizeof(struct sched_domain_attr), GFP_KERNEL);
 		if (dattr) {
@@ -986,12 +985,12 @@  static int generate_sched_domains(cpumask_var_t **domains,
 		cpumask_and(doms[0], top_cpuset.effective_cpus,
 			    housekeeping_cpumask(HK_TYPE_DOMAIN));

-		goto done;
+		goto set_ndoms;
 	}

 	csa = kmalloc_array(nr_cpusets(), sizeof(cp), GFP_KERNEL);
 	if (!csa)
-		goto done;
+		goto set_ndoms;
 	csn = 0;

 	rcu_read_lock();
@@ -1123,6 +1122,7 @@  static int generate_sched_domains(cpumask_var_t **domains,
 	 * See comments in partition_sched_domains().
 	 */
 	if (doms == NULL)
+set_ndoms:
 		ndoms = 1;

 	*domains    = doms;