[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2bf1b0cb-86af-4e00-a0aa-23e3944617a2@linux.ibm.com>
Date: Tue, 2 Jan 2024 10:53:15 +0100
From: Alexandra Winter <wintera@...ux.ibm.com>
To: Suman Ghosh <sumang@...vell.com>, Markus Elfring <Markus.Elfring@....de>,
"linux-s390@...r.kernel.org" <linux-s390@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Wenjia Zhang <wenjia@...ux.ibm.com>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [EXT] Re: [PATCH 1/2] net/iucv: Improve unlocking in
iucv_enable()
On 02.01.24 09:27, Suman Ghosh wrote:
>>> [Suman] This looks confusing. What is the issue with retaining the
>> original change?
>>
>> I propose to reduce the number of cpus_read_unlock() calls (in the
>> source code).
>>
>> Regards,
>> Markus
> [Suman] Then I think we should do something like this. Changing the code flow back-and-forth using "goto" does not seem correct.
I share Suman's concern that jumping backwards goto is confusing.
But I think the Coccinelle finding of freeing a null-pointer should be addressed (see patch 2/2)
Thank you Markus for reporting it.
The allocation does require holding the cpus_read_lock.
For some reason Markus wants to reduce the number of cpus_read_unlock() calls (why?),
so what about something like this for both issues:
diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 0ed6e34d6edd..1030403b826b 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -542,24 +542,22 @@ static int iucv_enable(void)
size_t alloc_size;
int cpu, rc;
- cpus_read_lock();
- rc = -ENOMEM;
alloc_size = iucv_max_pathid * sizeof(struct iucv_path);
iucv_path_table = kzalloc(alloc_size, GFP_KERNEL);
if (!iucv_path_table)
- goto out;
+ return -ENOMEM;
/* Declare per cpu buffers. */
- rc = -EIO;
+ cpus_read_lock();
for_each_online_cpu(cpu)
smp_call_function_single(cpu, iucv_declare_cpu, NULL, 1);
- if (cpumask_empty(&iucv_buffer_cpumask))
+ if (cpumask_empty(&iucv_buffer_cpumask)) {
/* No cpu could declare an iucv buffer. */
- goto out;
- cpus_read_unlock();
- return 0;
-out:
- kfree(iucv_path_table);
- iucv_path_table = NULL;
+ kfree(iucv_path_table);
+ iucv_path_table = NULL;
+ rc = -EIO;
+ } else {
+ rc = 0;
+ }
cpus_read_unlock();
return rc;
}
Powered by blists - more mailing lists