From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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
On 12/31/23 02:28, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
>> 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
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
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/[email protected]/
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 <[email protected]>
| Closes: https://lore.kernel.org/oe-lkp/[email protected]
[ 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/[email protected]
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 12/31/23 02:28, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
>