| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Apr 2023 00:35:08 +0000
From: Joel Fernandes <joel@...lfernandes.org>
To: "Paul E. McKenney" <paulmck@...nel.org>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel-team@...a.com, rostedt@...dmis.org, hch@....de,
"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Michał Mirosław <mirq-linux@...e.qmqm.pl>,
Dmitry Osipenko <dmitry.osipenko@...labora.com>,
Sachin Sant <sachinp@...ux.ibm.com>,
"Zhang, Qiang1" <qiang1.zhang@...el.com>
Subject: Re: [PATCH rcu 04/20] srcu: Begin offloading srcu_struct fields to
srcu_update
On Thu, Mar 30, 2023 at 03:47:10PM -0700, Paul E. McKenney wrote:
> The current srcu_struct structure is on the order of 200 bytes in size
> (depending on architecture and .config), which is much better than the
> old-style 26K bytes, but still all too inconvenient when one is trying
> to achieve good cache locality on a fastpath involving SRCU readers.
>
> However, only a few fields in srcu_struct are used by SRCU readers.
> The remaining fields could be offloaded to a new srcu_update
> structure, thus shrinking the srcu_struct structure down to a few
> tens of bytes. This commit begins this noble quest, a quest that is
> complicated by open-coded initialization of the srcu_struct within the
> srcu_notifier_head structure. This complication is addressed by updating
> the srcu_notifier_head structure's open coding, given that there does
> not appear to be a straightforward way of abstracting that initialization.
>
> This commit moves only the ->node pointer to srcu_update. Later commits
> will move additional fields.
>
> [ paulmck: Fold in qiang1.zhang@...el.com's memory-leak fix. ]
>
> Link: https://lore.kernel.org/all/20230320055751.4120251-1-qiang1.zhang@intel.com/
> Suggested-by: Christoph Hellwig <hch@....de>
[..]
> @@ -236,8 +236,12 @@ static bool init_srcu_struct_nodes(struct srcu_struct *ssp, gfp_t gfp_flags)
> */
> static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
> {
> + if (!is_static)
> + ssp->srcu_sup = kzalloc(sizeof(*ssp->srcu_sup), GFP_KERNEL);
> + if (!ssp->srcu_sup)
> + return -ENOMEM;
> ssp->srcu_size_state = SRCU_SIZE_SMALL;
> - ssp->node = NULL;
> + ssp->srcu_sup->node = NULL;
> mutex_init(&ssp->srcu_cb_mutex);
> mutex_init(&ssp->srcu_gp_mutex);
> ssp->srcu_idx = 0;
> @@ -249,8 +253,11 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
> ssp->sda_is_static = is_static;
> if (!is_static)
> ssp->sda = alloc_percpu(struct srcu_data);
> - if (!ssp->sda)
> + if (!ssp->sda) {
> + if (!is_static)
> + kfree(ssp->srcu_sup);
> return -ENOMEM;
> + }
> init_srcu_struct_data(ssp);
> ssp->srcu_gp_seq_needed_exp = 0;
> ssp->srcu_last_gp_end = ktime_get_mono_fast_ns();
> @@ -259,6 +266,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
[1] Here there is an if (!init_srcu_struct_nodes(...)) that the diff does not show.
> if (!ssp->sda_is_static) {
> free_percpu(ssp->sda);
> ssp->sda = NULL;
> + kfree(ssp->srcu_sup);
> return -ENOMEM;
> }
> } else {
Just a comment about the original code with reference to [1].
Here if allocations in init_srcu_struct_nodes() fail, it will return false
and execute the "if (!ssp->sda_is_is_static)" bit.
So if the allocation in [1] fails, then if sda_is_static is true, we return
-ENOMEM, however if sda_is_static is false, we do the following:
ssp->srcu_sup->srcu_ssp = ssp;
smp_store_release(&ssp->srcu_sup->srcu_gp_seq_needed, 0); /* Init done. */
return 0;
Is that really correct?
In other words, if init_srcu_struct_nodes() returns false, then passing along
the return value of init_srcu_struct_nodes() back to the caller of
init_srcu_struct_fields() depends on whether is_static = true or false. That
seems a bit wrong to me, init_srcu_struct_fields() should always return
-ENOMEM when init_srcu_struct_nodes() fails to allocate memory IMHO, whether
is_static is true or not.
Sorry if I missed something subtle, and if the code is correct to begin with.
Also I feel the return paths could be made better to also fix the above issue
I mentioned. How about the following diff on top of the series, would it
work?
Thanks!
---8<-----------------------
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index a887cfc89894..1975d06986fa 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -255,29 +255,30 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp, bool is_static)
ssp->srcu_sup->sda_is_static = is_static;
if (!is_static)
ssp->sda = alloc_percpu(struct srcu_data);
- if (!ssp->sda) {
- if (!is_static)
- kfree(ssp->srcu_sup);
- return -ENOMEM;
- }
+ if (!ssp->sda)
+ goto err_free_sup;
init_srcu_struct_data(ssp);
ssp->srcu_sup->srcu_gp_seq_needed_exp = 0;
ssp->srcu_sup->srcu_last_gp_end = ktime_get_mono_fast_ns();
if (READ_ONCE(ssp->srcu_sup->srcu_size_state) == SRCU_SIZE_SMALL && SRCU_SIZING_IS_INIT()) {
- if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC)) {
- if (!ssp->srcu_sup->sda_is_static) {
- free_percpu(ssp->sda);
- ssp->sda = NULL;
- kfree(ssp->srcu_sup);
- return -ENOMEM;
- }
- } else {
+ if (!init_srcu_struct_nodes(ssp, GFP_ATOMIC))
+ goto err_free_sda;
+ else
WRITE_ONCE(ssp->srcu_sup->srcu_size_state, SRCU_SIZE_BIG);
- }
}
ssp->srcu_sup->srcu_ssp = ssp;
smp_store_release(&ssp->srcu_sup->srcu_gp_seq_needed, 0); /* Init done. */
return 0;
+
+err_free_sda:
+ if (!is_static) {
+ free_percpu(ssp->sda);
+ ssp->sda = NULL;
+ }
+err_free_sup:
+ if (!is_static)
+ kfree(ssp->srcu_sup);
+ return -ENOMEM;
}
#ifdef CONFIG_DEBUG_LOCK_ALLOC
Powered by blists - more mailing lists