2020-08-25 02:51:44

by Joel Fernandes

[permalink] [raw]
Subject: [PATCH v4 -rcu 3/4] 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) <[email protected]>
---
include/linux/rcu_segcblist.h | 2 +
kernel/rcu/rcu_segcblist.c | 82 +++++++++++++++++++++++++++++++++--
2 files changed, 81 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 076337ae2e50..73a103464ea4 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -88,6 +88,62 @@ 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
+}
+
+/* Return number of callbacks in a segment of the segmented callback list. */
+static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
+{
+#ifdef CONFIG_RCU_NOCB_CPU
+ smp_mb__before_atomic(); /* Up to the caller! */
+ atomic_long_add(v, &rsclp->seglen[seg]);
+ smp_mb__after_atomic(); /* Up to the caller! */
+#else
+ smp_mb(); /* Up to the caller! */
+ WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
+ smp_mb(); /* Up to the caller! */
+#endif
+}
+
+/* Move from's segment length to to's segment. */
+static void rcu_segcblist_move_seglen(struct rcu_segcblist *rsclp, int from, int to)
+{
+ long len;
+
+ if (from == to)
+ return;
+
+ len = rcu_segcblist_get_seglen(rsclp, from);
+ if (!len)
+ return;
+
+ rcu_segcblist_add_seglen(rsclp, to, len);
+ rcu_segcblist_set_seglen(rsclp, from, 0);
+}
+
+/* Increment segment's length. */
+static void rcu_segcblist_inc_seglen(struct rcu_segcblist *rsclp, int seg)
+{
+ rcu_segcblist_add_seglen(rsclp, seg, 1);
+}
+
/*
* Increase the numeric length of an rcu_segcblist structure by the
* specified amount, which can be negative. This can cause the ->len
@@ -149,8 +205,10 @@ void rcu_segcblist_init(struct rcu_segcblist *rsclp)
BUILD_BUG_ON(RCU_NEXT_TAIL + 1 != ARRAY_SIZE(rsclp->gp_seq));
BUILD_BUG_ON(ARRAY_SIZE(rsclp->tails) != ARRAY_SIZE(rsclp->gp_seq));
rsclp->head = NULL;
- for (i = 0; i < RCU_CBLIST_NSEGS; i++)
+ for (i = 0; i < RCU_CBLIST_NSEGS; i++) {
rsclp->tails[i] = &rsclp->head;
+ rcu_segcblist_set_seglen(rsclp, i, 0);
+ }
rcu_segcblist_set_len(rsclp, 0);
rsclp->enabled = 1;
}
@@ -245,6 +303,7 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
struct rcu_head *rhp)
{
rcu_segcblist_inc_len(rsclp);
+ rcu_segcblist_inc_seglen(rsclp, RCU_NEXT_TAIL);
smp_mb(); /* Ensure counts are updated before callback is enqueued. */
rhp->next = NULL;
WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
@@ -274,6 +333,7 @@ bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
for (i = RCU_NEXT_TAIL; i > RCU_DONE_TAIL; i--)
if (rsclp->tails[i] != rsclp->tails[i - 1])
break;
+ rcu_segcblist_inc_seglen(rsclp, i);
WRITE_ONCE(*rsclp->tails[i], rhp);
for (; i <= RCU_NEXT_TAIL; i++)
WRITE_ONCE(rsclp->tails[i], &rhp->next);
@@ -307,6 +367,7 @@ void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,

if (!rcu_segcblist_ready_cbs(rsclp))
return; /* Nothing to do. */
+ rclp->len = rcu_segcblist_get_seglen(rsclp, RCU_DONE_TAIL);
*rclp->tail = rsclp->head;
WRITE_ONCE(rsclp->head, *rsclp->tails[RCU_DONE_TAIL]);
WRITE_ONCE(*rsclp->tails[RCU_DONE_TAIL], NULL);
@@ -314,6 +375,7 @@ void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
for (i = RCU_CBLIST_NSEGS - 1; i >= RCU_DONE_TAIL; i--)
if (rsclp->tails[i] == rsclp->tails[RCU_DONE_TAIL])
WRITE_ONCE(rsclp->tails[i], &rsclp->head);
+ rcu_segcblist_set_seglen(rsclp, RCU_DONE_TAIL, 0);
}

/*
@@ -330,11 +392,16 @@ void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,

if (!rcu_segcblist_pend_cbs(rsclp))
return; /* Nothing to do. */
+ rclp->len = rcu_segcblist_get_seglen(rsclp, RCU_WAIT_TAIL) +
+ rcu_segcblist_get_seglen(rsclp, RCU_NEXT_READY_TAIL) +
+ rcu_segcblist_get_seglen(rsclp, RCU_NEXT_TAIL);
*rclp->tail = *rsclp->tails[RCU_DONE_TAIL];
rclp->tail = rsclp->tails[RCU_NEXT_TAIL];
WRITE_ONCE(*rsclp->tails[RCU_DONE_TAIL], NULL);
- for (i = RCU_DONE_TAIL + 1; i < RCU_CBLIST_NSEGS; i++)
+ for (i = RCU_DONE_TAIL + 1; i < RCU_CBLIST_NSEGS; i++) {
WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_DONE_TAIL]);
+ rcu_segcblist_set_seglen(rsclp, i, 0);
+ }
}

/*
@@ -359,6 +426,7 @@ void rcu_segcblist_insert_done_cbs(struct rcu_segcblist *rsclp,

if (!rclp->head)
return; /* No callbacks to move. */
+ rcu_segcblist_add_seglen(rsclp, RCU_DONE_TAIL, rclp->len);
*rclp->tail = rsclp->head;
WRITE_ONCE(rsclp->head, rclp->head);
for (i = RCU_DONE_TAIL; i < RCU_CBLIST_NSEGS; i++)
@@ -379,6 +447,8 @@ void rcu_segcblist_insert_pend_cbs(struct rcu_segcblist *rsclp,
{
if (!rclp->head)
return; /* Nothing to do. */
+
+ rcu_segcblist_add_seglen(rsclp, RCU_NEXT_TAIL, rclp->len);
WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rclp->head);
WRITE_ONCE(rsclp->tails[RCU_NEXT_TAIL], rclp->tail);
}
@@ -403,6 +473,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
if (ULONG_CMP_LT(seq, rsclp->gp_seq[i]))
break;
WRITE_ONCE(rsclp->tails[RCU_DONE_TAIL], rsclp->tails[i]);
+ rcu_segcblist_move_seglen(rsclp, i, RCU_DONE_TAIL);
}

/* If no callbacks moved, nothing more need be done. */
@@ -423,6 +494,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
if (rsclp->tails[j] == rsclp->tails[RCU_NEXT_TAIL])
break; /* No more callbacks. */
WRITE_ONCE(rsclp->tails[j], rsclp->tails[i]);
+ rcu_segcblist_move_seglen(rsclp, i, j);
rsclp->gp_seq[j] = rsclp->gp_seq[i];
}
}
@@ -444,7 +516,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
*/
bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
{
- int i;
+ int i, j;

WARN_ON_ONCE(!rcu_segcblist_is_enabled(rsclp));
if (rcu_segcblist_restempty(rsclp, RCU_DONE_TAIL))
@@ -487,6 +559,10 @@ bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
if (rcu_segcblist_restempty(rsclp, i) || ++i >= RCU_NEXT_TAIL)
return false;

+ /* Accounting: everything below i is about to get merged into i. */
+ for (j = i + 1; j <= RCU_NEXT_TAIL; j++)
+ rcu_segcblist_move_seglen(rsclp, j, i);
+
/*
* Merge all later callbacks, including newly arrived callbacks,
* into the segment located by the for-loop above. Assign "seq"
--
2.28.0.297.g1956fa8f8d-goog


2020-08-25 21:55:07

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH v4 -rcu 3/4] rcu/segcblist: Add counters to segcblist datastructure

On Mon, Aug 24, 2020 at 10:48:41PM -0400, Joel Fernandes (Google) wrote:
> 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.

Might I recommend a more repeatable and sustainable test methodology?
(Sorry, couldn't resist, given that you left yourself wide open for
that one...)

> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> include/linux/rcu_segcblist.h | 2 +
> kernel/rcu/rcu_segcblist.c | 82 +++++++++++++++++++++++++++++++++--
> 2 files changed, 81 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];

What are the guarantees for access to seglen?

I get that the possibility of RCU callback offloading means that you
need atomic_long_t, but presumably you are only guaranteeing coherent
lockless access to the individual elements of the array, rather than
any coherent relationship among them or with ->len. Anything surprising
should go in the header comment blocks of the access functions.

And in case someone thinks that atomic_t would suffice, I have in the past
had list with in excess of 30M callbacks without RCU CPU stall warnings,
so 2B is not as large a number as you might think.

> #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 076337ae2e50..73a103464ea4 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -88,6 +88,62 @@ 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
> +}
> +
> +/* Return number of callbacks in a segment of the segmented callback list. */
> +static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
> +{
> +#ifdef CONFIG_RCU_NOCB_CPU
> + smp_mb__before_atomic(); /* Up to the caller! */
> + atomic_long_add(v, &rsclp->seglen[seg]);
> + smp_mb__after_atomic(); /* Up to the caller! */
> +#else
> + smp_mb(); /* Up to the caller! */
> + WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
> + smp_mb(); /* Up to the caller! */
> +#endif
> +}
> +
> +/* Move from's segment length to to's segment. */
> +static void rcu_segcblist_move_seglen(struct rcu_segcblist *rsclp, int from, int to)
> +{
> + long len;
> +
> + if (from == to)
> + return;
> +
> + len = rcu_segcblist_get_seglen(rsclp, from);
> + if (!len)
> + return;

You don't need the blank lines on this small a function.

> + rcu_segcblist_add_seglen(rsclp, to, len);
> + rcu_segcblist_set_seglen(rsclp, from, 0);
> +}
> +
> +/* Increment segment's length. */
> +static void rcu_segcblist_inc_seglen(struct rcu_segcblist *rsclp, int seg)
> +{
> + rcu_segcblist_add_seglen(rsclp, seg, 1);
> +}
> +
> /*
> * Increase the numeric length of an rcu_segcblist structure by the
> * specified amount, which can be negative. This can cause the ->len
> @@ -149,8 +205,10 @@ void rcu_segcblist_init(struct rcu_segcblist *rsclp)
> BUILD_BUG_ON(RCU_NEXT_TAIL + 1 != ARRAY_SIZE(rsclp->gp_seq));
> BUILD_BUG_ON(ARRAY_SIZE(rsclp->tails) != ARRAY_SIZE(rsclp->gp_seq));
> rsclp->head = NULL;
> - for (i = 0; i < RCU_CBLIST_NSEGS; i++)
> + for (i = 0; i < RCU_CBLIST_NSEGS; i++) {
> rsclp->tails[i] = &rsclp->head;
> + rcu_segcblist_set_seglen(rsclp, i, 0);
> + }
> rcu_segcblist_set_len(rsclp, 0);
> rsclp->enabled = 1;
> }
> @@ -245,6 +303,7 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> struct rcu_head *rhp)
> {
> rcu_segcblist_inc_len(rsclp);
> + rcu_segcblist_inc_seglen(rsclp, RCU_NEXT_TAIL);
> smp_mb(); /* Ensure counts are updated before callback is enqueued. */
> rhp->next = NULL;
> WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
> @@ -274,6 +333,7 @@ bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> for (i = RCU_NEXT_TAIL; i > RCU_DONE_TAIL; i--)
> if (rsclp->tails[i] != rsclp->tails[i - 1])
> break;
> + rcu_segcblist_inc_seglen(rsclp, i);
> WRITE_ONCE(*rsclp->tails[i], rhp);
> for (; i <= RCU_NEXT_TAIL; i++)
> WRITE_ONCE(rsclp->tails[i], &rhp->next);
> @@ -307,6 +367,7 @@ void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
>
> if (!rcu_segcblist_ready_cbs(rsclp))
> return; /* Nothing to do. */
> + rclp->len = rcu_segcblist_get_seglen(rsclp, RCU_DONE_TAIL);
> *rclp->tail = rsclp->head;
> WRITE_ONCE(rsclp->head, *rsclp->tails[RCU_DONE_TAIL]);
> WRITE_ONCE(*rsclp->tails[RCU_DONE_TAIL], NULL);
> @@ -314,6 +375,7 @@ void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
> for (i = RCU_CBLIST_NSEGS - 1; i >= RCU_DONE_TAIL; i--)
> if (rsclp->tails[i] == rsclp->tails[RCU_DONE_TAIL])
> WRITE_ONCE(rsclp->tails[i], &rsclp->head);
> + rcu_segcblist_set_seglen(rsclp, RCU_DONE_TAIL, 0);
> }
>
> /*
> @@ -330,11 +392,16 @@ void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
>
> if (!rcu_segcblist_pend_cbs(rsclp))
> return; /* Nothing to do. */
> + rclp->len = rcu_segcblist_get_seglen(rsclp, RCU_WAIT_TAIL) +
> + rcu_segcblist_get_seglen(rsclp, RCU_NEXT_READY_TAIL) +
> + rcu_segcblist_get_seglen(rsclp, RCU_NEXT_TAIL);

A loop for future-proofness, please. The compiler will unroll it anyway.
Plus this is nowhere near a fastpath.

> *rclp->tail = *rsclp->tails[RCU_DONE_TAIL];
> rclp->tail = rsclp->tails[RCU_NEXT_TAIL];
> WRITE_ONCE(*rsclp->tails[RCU_DONE_TAIL], NULL);
> - for (i = RCU_DONE_TAIL + 1; i < RCU_CBLIST_NSEGS; i++)
> + for (i = RCU_DONE_TAIL + 1; i < RCU_CBLIST_NSEGS; i++) {
> WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_DONE_TAIL]);
> + rcu_segcblist_set_seglen(rsclp, i, 0);
> + }
> }
>
> /*
> @@ -359,6 +426,7 @@ void rcu_segcblist_insert_done_cbs(struct rcu_segcblist *rsclp,
>
> if (!rclp->head)
> return; /* No callbacks to move. */
> + rcu_segcblist_add_seglen(rsclp, RCU_DONE_TAIL, rclp->len);
> *rclp->tail = rsclp->head;
> WRITE_ONCE(rsclp->head, rclp->head);
> for (i = RCU_DONE_TAIL; i < RCU_CBLIST_NSEGS; i++)
> @@ -379,6 +447,8 @@ void rcu_segcblist_insert_pend_cbs(struct rcu_segcblist *rsclp,
> {
> if (!rclp->head)
> return; /* Nothing to do. */
> +

This only has five lines of code, so the blank line is superfluous.

> + rcu_segcblist_add_seglen(rsclp, RCU_NEXT_TAIL, rclp->len);
> WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rclp->head);
> WRITE_ONCE(rsclp->tails[RCU_NEXT_TAIL], rclp->tail);
> }
> @@ -403,6 +473,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
> if (ULONG_CMP_LT(seq, rsclp->gp_seq[i]))
> break;
> WRITE_ONCE(rsclp->tails[RCU_DONE_TAIL], rsclp->tails[i]);
> + rcu_segcblist_move_seglen(rsclp, i, RCU_DONE_TAIL);
> }
>
> /* If no callbacks moved, nothing more need be done. */
> @@ -423,6 +494,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
> if (rsclp->tails[j] == rsclp->tails[RCU_NEXT_TAIL])
> break; /* No more callbacks. */
> WRITE_ONCE(rsclp->tails[j], rsclp->tails[i]);
> + rcu_segcblist_move_seglen(rsclp, i, j);
> rsclp->gp_seq[j] = rsclp->gp_seq[i];
> }
> }
> @@ -444,7 +516,7 @@ void rcu_segcblist_advance(struct rcu_segcblist *rsclp, unsigned long seq)
> */
> bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
> {
> - int i;
> + int i, j;
>
> WARN_ON_ONCE(!rcu_segcblist_is_enabled(rsclp));
> if (rcu_segcblist_restempty(rsclp, RCU_DONE_TAIL))
> @@ -487,6 +559,10 @@ bool rcu_segcblist_accelerate(struct rcu_segcblist *rsclp, unsigned long seq)
> if (rcu_segcblist_restempty(rsclp, i) || ++i >= RCU_NEXT_TAIL)
> return false;
>
> + /* Accounting: everything below i is about to get merged into i. */

How about "newer than i"? Many people would think of "below i" as being
numerically less than i, which is not what we need them to think.

> + for (j = i + 1; j <= RCU_NEXT_TAIL; j++)
> + rcu_segcblist_move_seglen(rsclp, j, i);

I thought about merging the above loop with the next one, but the added
check and the snapshotting of "i" makes it not worthwhile, so please
leave this loop separate, just as you now have it.

Thanx, Paul

> /*
> * Merge all later callbacks, including newly arrived callbacks,
> * into the segment located by the for-loop above. Assign "seq"
> --
> 2.28.0.297.g1956fa8f8d-goog
>

2020-08-25 22:52:16

by Joel Fernandes

[permalink] [raw]
Subject: Re: [PATCH v4 -rcu 3/4] rcu/segcblist: Add counters to segcblist datastructure

On Tue, Aug 25, 2020 at 02:53:38PM -0700, Paul E. McKenney wrote:
> On Mon, Aug 24, 2020 at 10:48:41PM -0400, Joel Fernandes (Google) wrote:
> > 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.
>
> Might I recommend a more repeatable and sustainable test methodology?
> (Sorry, couldn't resist, given that you left yourself wide open for
> that one...)

Ah, this message was from an older series :-(. I did test it with rcutorture
for many hours. But I forgot to update this commit message. :-(

I will respond to other comments soon, I am still stuck on the comment about
rcl.len and rcu_barrier interaction that you brought up in 1/4 and 2/4.

Thanks!

- Joel

2020-08-26 14:38:33

by Paul E. McKenney

[permalink] [raw]
Subject: Re: [PATCH v4 -rcu 3/4] rcu/segcblist: Add counters to segcblist datastructure

On Tue, Aug 25, 2020 at 06:51:03PM -0400, Joel Fernandes wrote:
> On Tue, Aug 25, 2020 at 02:53:38PM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 24, 2020 at 10:48:41PM -0400, Joel Fernandes (Google) wrote:
> > > 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.
> >
> > Might I recommend a more repeatable and sustainable test methodology?
> > (Sorry, couldn't resist, given that you left yourself wide open for
> > that one...)
>
> Ah, this message was from an older series :-(. I did test it with rcutorture
> for many hours. But I forgot to update this commit message. :-(

That is better. Let me see if I can find a more targeted test from the
old days...

> I will respond to other comments soon, I am still stuck on the comment about
> rcl.len and rcu_barrier interaction that you brought up in 1/4 and 2/4.

Hoping my more recent replies help. If not, you know where to find me.

Thanx, Paul

2020-08-28 00:31:29

by kernel test robot

[permalink] [raw]
Subject: [rcu/segcblist] ab9a370277: WARNING:at_kernel/rcu/srcutree.c:#cleanup_srcu_struct

Greeting,

FYI, we noticed the following commit (built with gcc-9):

commit: ab9a3702779c3ced6523a717b41b2b36e9592e3b ("[PATCH v4 -rcu 3/4] rcu/segcblist: Add counters to segcblist datastructure")
url: https://github.com/0day-ci/linux/commits/Joel-Fernandes-Google/Maintain-the-length-of-each-segment-in-the-segcblist/20200825-105141
base: https://git.kernel.org/cgit/linux/kernel/git/paulmck/linux-rcu.git dev

in testcase: ltp
with following parameters:

test: filecaps
ucode: 0x7000019

test-description: The LTP testsuite contains a collection of tools for testing the Linux kernel and related features.
test-url: http://linux-test-project.github.io/


on test machine: 16 threads Intel(R) Xeon(R) CPU D-1541 @ 2.10GHz with 48G memory

caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):




If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>


[ 54.493216] WARNING: CPU: 1 PID: 175 at kernel/rcu/srcutree.c:384 cleanup_srcu_struct+0x9d/0x100
[ 54.502626] Modules linked in: btrfs blake2b_generic xor zstd_compress intel_rapl_msr raid6_pq intel_rapl_common libcrc32c sd_mod t10_pi sg sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel ipmi_ssif kvm ast drm_vram_helper drm_ttm_helper irqbypass crct10dif_pclmul ttm crc32_pclmul crc32c_intel ghash_clmulni_intel drm_kms_helper rapl syscopyarea sysfillrect sysimgblt fb_sys_fops intel_cstate ahci acpi_ipmi mxm_wmi libahci drm intel_uncore ipmi_si mei_me libata ioatdma ipmi_devintf mei dca intel_pch_thermal gpio_ich ipmi_msghandler joydev wmi acpi_pad ip_tables
[ 54.556977] CPU: 1 PID: 175 Comm: kworker/1:1 Not tainted 5.9.0-rc1-00141-gab9a3702779c3c #1
[ 54.566167] Hardware name: Supermicro SYS-5018D-FN4T/X10SDV-8C-TLN4F, BIOS 1.1 03/02/2016
[ 54.575132] Workqueue: events free_user_work [ipmi_msghandler]
[ 54.581760] RIP: 0010:cleanup_srcu_struct+0x9d/0x100
[ 54.587529] Code: 1c c5 60 b9 41 82 48 8d bb d8 00 00 00 e8 db 4d 01 00 48 8d bb 00 01 00 00 e8 ef af f9 ff 48 8b 83 90 00 00 00 48 85 c0 74 b1 <0f> 0b 5b 5d 41 5c 41 5d c3 49 8b 84 24 d0 c3 00 00 a8 03 0f 85 ae
[ 54.607973] RSP: 0018:ffffc900004cbe48 EFLAGS: 00010202
[ 54.614039] RAX: 0000000000000001 RBX: ffffe8ffffa4b1c0 RCX: 0000000000000000
[ 54.622026] RDX: 0000000000000040 RSI: 0000000000000002 RDI: ffff888c7fa6af00
[ 54.630012] RBP: 0000000000000001 R08: ffff888c7fa6af00 R09: ffff888107802910
[ 54.638002] R10: ffff888107802948 R11: 0000000000000000 R12: ffffc90001461018
[ 54.645993] R13: ffffc9000146d408 R14: ffff88810709f680 R15: ffffc9000146d4e8
[ 54.653994] FS: 0000000000000000(0000) GS:ffff888c7fa40000(0000) knlGS:0000000000000000
[ 54.662953] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 54.669572] CR2: 00007efdccfd6f58 CR3: 0000000c7e60a005 CR4: 00000000003706e0
[ 54.677624] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 54.685667] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 54.693711] Call Trace:
[ 54.697062] free_user_work+0x19/0x40 [ipmi_msghandler]
[ 54.703186] process_one_work+0x1b5/0x3a0
[ 54.708134] ? process_one_work+0x3a0/0x3a0
[ 54.713219] worker_thread+0x50/0x3c0
[ 54.717787] ? process_one_work+0x3a0/0x3a0
[ 54.722849] kthread+0x114/0x160
[ 54.726958] ? kthread_park+0xa0/0xa0
[ 54.731473] ret_from_fork+0x22/0x30
[ 54.735930] ---[ end trace 03716c0aea0df37c ]---


To reproduce:

git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp install job.yaml # job file is attached in this email
bin/lkp run job.yaml



Thanks,
lkp


Attachments:
(No filename) (3.64 kB)
config-5.9.0-rc1-00141-gab9a3702779c3c (172.93 kB)
job-script (5.67 kB)
dmesg (143.20 kB)
ltp (13.18 kB)
job.yaml (4.62 kB)
reproduce (84.00 B)
Download all attachments