Re: [PATCH RFC v2] rcu/segcblist: Add counters to segcblist datastructure

[Boqun Feng <boqun.feng@gmail.com>]

On Fri, Jul 24, 2020 at 03:34:46PM -0400, Joel Fernandes wrote:
> On Mon, Jul 20, 2020 at 4:22 AM <boqun.feng@gmail.com> wrote:
> >
> > Hi Joel,
> 
> Sorry for the late reply as I was on vacation last several days.
> 
> >
> > On Sun, Jul 19, 2020 at 12:18:41AM -0400, Joel Fernandes wrote:
> > > On Sun, Jul 19, 2020 at 12:06:28AM -0400, Joel Fernandes wrote:
> > > > On Sat, Jul 18, 2020 at 11:55 PM Joel Fernandes (Google)
> > > > <joel@joelfernandes.org> wrote:
> > > > [...]
> > > > >         /* If no callbacks moved, nothing more need be done. */
> > > > > @@ -419,10 +494,9 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
> > > > >          * callbacks.  The overall effect is to copy down the later pointers
> > > > >          * into the gap that was created by the now-ready segments.
> > > > >          */
> > > > > -       for (j = RCU_WAIT_TAIL; i < RCU_NEXT_TAIL; i++, j++) {
> > > > > -               if (rsclp->tails[j] == rsclp->tails[RCU_NEXT_TAIL])
> > > > > -                       break;  /* No more callbacks. */
> > > > > +       for (j = RCU_WAIT_TAIL; i < RCU_NEXT_TAIL && j < RCU_NEXT_TAIL; i++, j++) {
> > > > >                 WRITE_ONCE(rsclp->tails[j], rsclp->tails[i]);
> > > > > +               rcu_segcblist_move_seglen(rsclp, i, j);
> > > > >                 rsclp->gp_seq[j] = rsclp->gp_seq[i];
> > > > >         }
> > > >
> > > > Unfortunately I broke this code, _sigh_.  I need to reinstate the
> > > > if (rsclp->tails[j] == rsclp->tails[RCU_NEXT_TAIL]) , I completely
> > > > misunderstood that.
> > >
> > > And hopefully, third time's a charm with various extra newlines removed:
> > >
> > > ---8<-----------------------
> > >
> > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > > Subject: [PATCH v3] rcu/segcblist: Add counters to segcblist datastructure
> > >
> > > Add counting of segment lengths of segmented callback list.
> > >
> > > This will be useful for a number of things such as knowing how big the
> > > ready-to-execute segment have gotten. The immediate benefit is ability
> > > to trace how the callbacks in the segmented callback list change.
> > >
> > > Tested by profusely reading traces when segcblist counts updated.
> > >
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > >
> > > v1->v3: minor nits.
> > > ---
> > >  include/linux/rcu_segcblist.h |  2 +
> > >  kernel/rcu/rcu_segcblist.c    | 77 +++++++++++++++++++++++++++++++++--
> > >  2 files changed, 76 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> > > index b36afe7b22c9..d462ae5e340a 100644
> > > --- a/include/linux/rcu_segcblist.h
> > > +++ b/include/linux/rcu_segcblist.h
> > > @@ -69,8 +69,10 @@ struct rcu_segcblist {
> > >       unsigned long gp_seq[RCU_CBLIST_NSEGS];
> > >  #ifdef CONFIG_RCU_NOCB_CPU
> > >       atomic_long_t len;
> > > +     atomic_long_t seglen[RCU_CBLIST_NSEGS];
> > >  #else
> > >       long len;
> > > +     long seglen[RCU_CBLIST_NSEGS];
> > >  #endif
> > >       u8 enabled;
> > >       u8 offloaded;
> > > diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> > > index 9a0f66133b4b..c5841efcd38e 100644
> > > --- a/kernel/rcu/rcu_segcblist.c
> > > +++ b/kernel/rcu/rcu_segcblist.c
> > > @@ -88,6 +88,57 @@ static void rcu_segcblist_set_len(struct rcu_segcblist *rsclp, long v)
> > >  #endif
> > >  }
> > >
> > > +/* Get the length of a segment of the rcu_segcblist structure. */
> > > +static long rcu_segcblist_get_seglen(struct rcu_segcblist *rsclp, int seg)
> > > +{
> > > +#ifdef CONFIG_RCU_NOCB_CPU
> > > +     return atomic_long_read(&rsclp->seglen[seg]);
> > > +#else
> > > +     return READ_ONCE(rsclp->seglen[seg]);
> > > +#endif
> > > +}
> > > +
> > > +/* Set the length of a segment of the rcu_segcblist structure. */
> > > +static void rcu_segcblist_set_seglen(struct rcu_segcblist *rsclp, int seg, long v)
> > > +{
> > > +#ifdef CONFIG_RCU_NOCB_CPU
> > > +     atomic_long_set(&rsclp->seglen[seg], v);
> > > +#else
> > > +     WRITE_ONCE(rsclp->seglen[seg], v);
> > > +#endif
> >
> > These #ifdef really make me uncomfortable ;-) Since we are allowed to
> > use the '_Generic' key from C11 (see include/linux/compiler_types.h), I
> > wonder whether it's better if we have a few "generic" primitives like:
> >
> >         #define gen_long_read(x) _Generic((x),                                          \
> >                                           atomic_long_t: atomic_long_read(&x, v),       \
> >                                           long: READ_ONCE(x)),                          \
> >                                           ...
> >
> >         #define gen_long_set(x, v) _Generic((x),                                        \
> >                                             atomic_long_t: atomic_long_set(&x, v),      \
> >                                             long: WRITE_ONCE(x, v)),                    \
> >                                             ...
> >
> > , and similar for _xchg and _add.
> 
> This sounds like a good idea. But isn't the kernel compiled with -std=gnu89
> 
> Here are the KBUILD_CFLAGS
> 
> KBUILD_CFLAGS   := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
>                    -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
>                    -Werror=implicit-function-declaration -Werror=implicit-int \
>                    -Wno-format-security \
>                    -std=gnu89
> 
> With "make kernel/rcu/tree.o V=1" confirming it as well:
> 
>   gcc -Wp,-MD,kernel/rcu/.tree.o.d  -nostdinc -isystem
> /usr/lib/gcc/x86_64-linux-gnu/9/include -I./arch/x86/include
> -I./arch/x86/include/generated  -I./include -I./arch/x86/include/uapi
> -I./arch/x86/include/generated/uapi -I./include/uapi
> -I./include/generated/uapi -include ./include/linux/kconfig.h -include
> ./include/linux/compiler_types.h -D__KERNEL__ -Wall -Wundef
> -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing
> -fno-common -fshort-wchar -fno-PIE
> -Werror=implicit-function-declaration -Werror=implicit-int
> -Wno-format-security -std=gnu89
> 

Note that gnu89 is not equal to c89, according to gcc manual:

	https://gcc.gnu.org/onlinedocs/gcc/Standards.html#C-Language

"""
By default, GCC provides some extensions to the C language that, on
rare occasions conflict with the C standard. See Extensions to the C
Language Family. Some features that are part of the C99 standard are
accepted as extensions in C90 mode, and some features that are part of
the C11 standard are accepted as extensions in C90 and C99 modes. Use of
the -std options listed above disables these extensions where they
conflict with the C standard version selected. You may also select an
extended version of the C language explicitly with -std=gnu90 (for C90
with GNU extensions), -std=gnu99 (for C99 with GNU extensions) or
-std=gnu11 (for C11 with GNU extensions).
"""

So C11 features are available to gnu89 as extensions, also I tried to
compile the following code with -std=gnu89:

	#include <stdio.h>

	typedef struct {
		int a;
	} atomic_t;

	void g(void) {
		printf("this is g\n");
	}

	void h(void) {
		printf("this is f\n");
	}

	#define gen(x) _Generic((x), atomic_t : h(), int : g())

	int main(void) {
		int a;
		atomic_t b;
		gen(a);
		gen(b);
		gen(b);
	}

, and it worked.

Besides, please note that in include/linux/compiler_types.h, _Generic is
already used.

> >
> > With these primitives introduced, you can avoid () to add those
> > rcu_segcblist_*_seglen() which have #ifdefs in them. Of course, an
> > alternative would be that we implement rcu_segcblist_*_seglen() using
> > _Generic, but I think someone else may have the similar problems or
> > requirement (already or in the future), so it might be worthwhile to
> > introduce the gen_ primitives for broader usage.
> 
> One issue is code using memory barriers around the operation, such as
> in rcu_segcblist_add_len() where you use smp_mb__before_atomic() for
> the atomic version, and regular smp_mb() for the non-atomic version.
> So ifdef will still exist to some degree.
> 

Right, I think we can have two functions: long_add_mb() and
atomic_long_add_mb(), this part is similar to ifdef approach, but we can
make a gen_long_add_mb() based on these two functions, and
gen_long_add_mb() simply switches between those functions according to
the actual type of the field, which I think is better than ifdef
approach at readability and maintenance.

Regards,
Boqun

> thanks,
> 
>  - Joel