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, 25 Aug 2020 13:08:09 -0700 From: "Paul E. McKenney" <paulmck@...nel.org> To: "Joel Fernandes (Google)" <joel@...lfernandes.org> Cc: linux-kernel@...r.kernel.org, boqun.feng@...il.com, dave@...olabs.net, Ingo Molnar <mingo@...hat.com>, Josh Triplett <josh@...htriplett.org>, Lai Jiangshan <jiangshanlai@...il.com>, Madhuparna Bhowmik <madhuparnabhowmik10@...il.com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, neeraj.iitr10@...il.com, rcu@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>, "Uladzislau Rezki (Sony)" <urezki@...il.com>, vineethrp@...il.com Subject: Re: [PATCH v4 -rcu 1/4] rcu/segcblist: Do not depend on rcl->len to store the segcb len during merge On Mon, Aug 24, 2020 at 10:48:39PM -0400, Joel Fernandes (Google) wrote: > The donecbs's ->len field is used to store the total count of the segmented > callback list's length. This ->len field is then added to the destination segcb > list. > > However, this presents a problem for per-segment length counting which is added > in a future patch. This future patch sets the rcl->len field as we move > segments of callbacks between source and destination lists, thus becoming > incompatible with the donecb's ->len field. OK, I will bite. What is "rcl"? A placeholder for donecbs and pendcbs? If so, please just name them both. If not, please explain. > This commit therefore avoids depending on the ->len field in this way. IMHO, > this is also less error-prone and is more accurate - the donecb's ->len field > should be the length of the done segment and not just used as a temporarily > variable. Please also mention why ->len is handled specially at all, namely interactions between rcu_barrier() and callback invocation. This is the answer to "why not just make all this work like normal lists?" This might go well in the first paragraph. > Signed-off-by: Joel Fernandes (Google) <joel@...lfernandes.org> > --- > kernel/rcu/rcu_segcblist.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c > index 2d2a6b6b9dfb..b70d4154433c 100644 > --- a/kernel/rcu/rcu_segcblist.c > +++ b/kernel/rcu/rcu_segcblist.c > @@ -513,14 +513,18 @@ void rcu_segcblist_merge(struct rcu_segcblist *dst_rsclp, > { > struct rcu_cblist donecbs; > struct rcu_cblist pendcbs; > + long src_len; > > rcu_cblist_init(&donecbs); > rcu_cblist_init(&pendcbs); > - rcu_segcblist_extract_count(src_rsclp, &donecbs); > + > + src_len = rcu_segcblist_xchg_len(src_rsclp, 0); Given that both rcu_segcblist_xchg_len() and rcu_segcblist_extract_count() have only one callsite each, why not get rid of one of them? Or better yet, please see below, which should allow getting rid of both of them. > rcu_segcblist_extract_done_cbs(src_rsclp, &donecbs); > rcu_segcblist_extract_pend_cbs(src_rsclp, &pendcbs); > - rcu_segcblist_insert_count(dst_rsclp, &donecbs); > + > + rcu_segcblist_add_len(dst_rsclp, src_len); > rcu_segcblist_insert_done_cbs(dst_rsclp, &donecbs); > rcu_segcblist_insert_pend_cbs(dst_rsclp, &pendcbs); Rather than adding the blank lines, why not have the rcu_cblist structures carry the lengths? You are already adjusting one of the two call sites that care (rcu_do_batch()), and the other is srcu_invoke_callbacks(). That should shorten this function a bit more. And make callback handling much more approachable, I suspect. There would still be the callback-invocation need to be careful with ->cblist.len due to rcu_barrier() and srcu_barrier(). But both of those should be excluded by this code. (But don't take my word for it, ask KCSAN.) Thanx, Paul > + > rcu_segcblist_init(src_rsclp); > } > -- > 2.28.0.297.g1956fa8f8d-goog >
Powered by blists - more mailing lists