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) <[email protected]> > --- > 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 >
