RCU Archive on lore.kernel.org
 help / color / Atom feed
From: Joel Fernandes <joel@joelfernandes.org>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	rcu <rcu@vger.kernel.org>, Will Deacon <will@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH RFC v2] rcu/segcblist: Add counters to segcblist datastructure
Date: Fri, 24 Jul 2020 15:34:46 -0400
Message-ID: <CAEXW_YR4dxuPV+Yu9HcYCSAdiV1H=9Rk9HJgCST-YMMc7J2Mgg@mail.gmail.com> (raw)
In-Reply-To: <20200720082211.GA35358@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>

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

>
> 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.

thanks,

 - Joel

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-19  3:55 Joel Fernandes (Google)
2020-07-19  4:06 ` Joel Fernandes
2020-07-19  4:18   ` Joel Fernandes
2020-07-20  8:22     ` boqun.feng
2020-07-24 19:34       ` Joel Fernandes [this message]
2020-07-27 13:49         ` Boqun Feng
2020-07-27 23:03           ` Joel Fernandes
2020-07-29  1:28             ` Boqun Feng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAEXW_YR4dxuPV+Yu9HcYCSAdiV1H=9Rk9HJgCST-YMMc7J2Mgg@mail.gmail.com' \
    --to=joel@joelfernandes.org \
    --cc=boqun.feng@gmail.com \
    --cc=jiangshanlai@gmail.com \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rcu@vger.kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

RCU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/rcu/0 rcu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 rcu rcu/ https://lore.kernel.org/rcu \
		rcu@vger.kernel.org
	public-inbox-index rcu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.rcu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git