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.
Also this patch remove hacks related to using donecbs's ->len field as a
temporary variable to save the segmented callback list's length. This cannot be
done anymore and is not needed.
Reviewed-by: Frederic Weisbecker <[email protected]>
Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
include/linux/rcu_segcblist.h | 1 +
kernel/rcu/rcu_segcblist.c | 120 ++++++++++++++++++++++------------
kernel/rcu/rcu_segcblist.h | 2 -
3 files changed, 79 insertions(+), 44 deletions(-)
diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
index b36afe7b22c9..6c01f09a6456 100644
--- a/include/linux/rcu_segcblist.h
+++ b/include/linux/rcu_segcblist.h
@@ -72,6 +72,7 @@ struct rcu_segcblist {
#else
long len;
#endif
+ long seglen[RCU_CBLIST_NSEGS];
u8 enabled;
u8 offloaded;
};
diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
index bb246d8c6ef1..357c19bbcb00 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -7,10 +7,11 @@
* Authors: Paul E. McKenney <[email protected]>
*/
-#include <linux/types.h>
-#include <linux/kernel.h>
+#include <linux/cpu.h>
#include <linux/interrupt.h>
+#include <linux/kernel.h>
#include <linux/rcupdate.h>
+#include <linux/types.h>
#include "rcu_segcblist.h"
@@ -88,6 +89,46 @@ 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)
+{
+ return READ_ONCE(rsclp->seglen[seg]);
+}
+
+/* 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)
+{
+ WRITE_ONCE(rsclp->seglen[seg], v);
+}
+
+/* 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)
+{
+ WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
+}
+
+/* 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
@@ -119,26 +160,6 @@ void rcu_segcblist_inc_len(struct rcu_segcblist *rsclp)
rcu_segcblist_add_len(rsclp, 1);
}
-/*
- * Exchange the numeric length of the specified rcu_segcblist structure
- * with the specified value. This can cause the ->len field to disagree
- * with the actual number of callbacks on the structure. This exchange is
- * fully ordered with respect to the callers accesses both before and after.
- */
-static long rcu_segcblist_xchg_len(struct rcu_segcblist *rsclp, long v)
-{
-#ifdef CONFIG_RCU_NOCB_CPU
- return atomic_long_xchg(&rsclp->len, v);
-#else
- long ret = rsclp->len;
-
- smp_mb(); /* Up to the caller! */
- WRITE_ONCE(rsclp->len, v);
- smp_mb(); /* Up to the caller! */
- return ret;
-#endif
-}
-
/*
* Initialize an rcu_segcblist structure.
*/
@@ -149,8 +170,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;
}
@@ -246,6 +269,7 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
{
rcu_segcblist_inc_len(rsclp);
smp_mb(); /* Ensure counts are updated before callback is enqueued. */
+ rcu_segcblist_inc_seglen(rsclp, RCU_NEXT_TAIL);
rhp->next = NULL;
WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
WRITE_ONCE(rsclp->tails[RCU_NEXT_TAIL], &rhp->next);
@@ -274,27 +298,13 @@ 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);
return true;
}
-/*
- * Extract only the counts from the specified rcu_segcblist structure,
- * and place them in the specified rcu_cblist structure. This function
- * supports both callback orphaning and invocation, hence the separation
- * of counts and callbacks. (Callbacks ready for invocation must be
- * orphaned and adopted separately from pending callbacks, but counts
- * apply to all callbacks. Locking must be used to make sure that
- * both orphaned-callbacks lists are consistent.)
- */
-void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
- struct rcu_cblist *rclp)
-{
- rclp->len = rcu_segcblist_xchg_len(rsclp, 0);
-}
-
/*
* Extract only those callbacks ready to be invoked from the specified
* rcu_segcblist structure and place them in the specified rcu_cblist
@@ -307,6 +317,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 +325,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 +342,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);
+ }
}
/*
@@ -345,7 +362,6 @@ void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
struct rcu_cblist *rclp)
{
rcu_segcblist_add_len(rsclp, rclp->len);
- rclp->len = 0;
}
/*
@@ -359,6 +375,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 +396,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 +422,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 +443,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 +465,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 +508,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"
@@ -514,13 +539,24 @@ void rcu_segcblist_merge(struct rcu_segcblist *dst_rsclp,
struct rcu_cblist donecbs;
struct rcu_cblist pendcbs;
+ lockdep_assert_cpus_held();
+
rcu_cblist_init(&donecbs);
rcu_cblist_init(&pendcbs);
- rcu_segcblist_extract_count(src_rsclp, &donecbs);
+
rcu_segcblist_extract_done_cbs(src_rsclp, &donecbs);
rcu_segcblist_extract_pend_cbs(src_rsclp, &pendcbs);
+
+ /*
+ * No need smp_mb() before setting length to 0, because CPU hotplug
+ * lock excludes rcu_barrier.
+ */
+ rcu_segcblist_set_len(src_rsclp, 0);
+
rcu_segcblist_insert_count(dst_rsclp, &donecbs);
+ rcu_segcblist_insert_count(dst_rsclp, &pendcbs);
rcu_segcblist_insert_done_cbs(dst_rsclp, &donecbs);
rcu_segcblist_insert_pend_cbs(dst_rsclp, &pendcbs);
+
rcu_segcblist_init(src_rsclp);
}
diff --git a/kernel/rcu/rcu_segcblist.h b/kernel/rcu/rcu_segcblist.h
index 1d2d61406463..cd35c9faaf51 100644
--- a/kernel/rcu/rcu_segcblist.h
+++ b/kernel/rcu/rcu_segcblist.h
@@ -89,8 +89,6 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
struct rcu_head *rhp);
bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
struct rcu_head *rhp);
-void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
- struct rcu_cblist *rclp);
void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
struct rcu_cblist *rclp);
void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
--
2.29.1.341.ge80a0c044ae-goog
On 11/3/2020 7:55 PM, 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.
>
> Also this patch remove hacks related to using donecbs's ->len field as a
> temporary variable to save the segmented callback list's length. This cannot be
> done anymore and is not needed.
>
> Reviewed-by: Frederic Weisbecker <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
Reviewed-by: Neeraj Upadhyay <[email protected]>
> include/linux/rcu_segcblist.h | 1 +
> kernel/rcu/rcu_segcblist.c | 120 ++++++++++++++++++++++------------
> kernel/rcu/rcu_segcblist.h | 2 -
> 3 files changed, 79 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> index b36afe7b22c9..6c01f09a6456 100644
> --- a/include/linux/rcu_segcblist.h
> +++ b/include/linux/rcu_segcblist.h
> @@ -72,6 +72,7 @@ struct rcu_segcblist {
> #else
> long len;
> #endif
> + long seglen[RCU_CBLIST_NSEGS];
> u8 enabled;
> u8 offloaded;
> };
> diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> index bb246d8c6ef1..357c19bbcb00 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -7,10 +7,11 @@
> * Authors: Paul E. McKenney <[email protected]>
> */
>
> -#include <linux/types.h>
> -#include <linux/kernel.h>
> +#include <linux/cpu.h>
> #include <linux/interrupt.h>
> +#include <linux/kernel.h>
> #include <linux/rcupdate.h>
> +#include <linux/types.h>
>
> #include "rcu_segcblist.h"
>
> @@ -88,6 +89,46 @@ 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)
> +{
> + return READ_ONCE(rsclp->seglen[seg]);
> +}
> +
> +/* 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)
> +{
> + WRITE_ONCE(rsclp->seglen[seg], v);
> +}
> +
> +/* 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)
> +{
> + WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
> +}
> +
> +/* 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
> @@ -119,26 +160,6 @@ void rcu_segcblist_inc_len(struct rcu_segcblist *rsclp)
> rcu_segcblist_add_len(rsclp, 1);
> }
>
> -/*
> - * Exchange the numeric length of the specified rcu_segcblist structure
> - * with the specified value. This can cause the ->len field to disagree
> - * with the actual number of callbacks on the structure. This exchange is
> - * fully ordered with respect to the callers accesses both before and after.
> - */
> -static long rcu_segcblist_xchg_len(struct rcu_segcblist *rsclp, long v)
> -{
> -#ifdef CONFIG_RCU_NOCB_CPU
> - return atomic_long_xchg(&rsclp->len, v);
> -#else
> - long ret = rsclp->len;
> -
> - smp_mb(); /* Up to the caller! */
> - WRITE_ONCE(rsclp->len, v);
> - smp_mb(); /* Up to the caller! */
> - return ret;
> -#endif
> -}
> -
> /*
> * Initialize an rcu_segcblist structure.
> */
> @@ -149,8 +170,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;
> }
> @@ -246,6 +269,7 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> {
> rcu_segcblist_inc_len(rsclp);
> smp_mb(); /* Ensure counts are updated before callback is enqueued. */
> + rcu_segcblist_inc_seglen(rsclp, RCU_NEXT_TAIL);
> rhp->next = NULL;
> WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
> WRITE_ONCE(rsclp->tails[RCU_NEXT_TAIL], &rhp->next);
> @@ -274,27 +298,13 @@ 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);
> return true;
> }
>
> -/*
> - * Extract only the counts from the specified rcu_segcblist structure,
> - * and place them in the specified rcu_cblist structure. This function
> - * supports both callback orphaning and invocation, hence the separation
> - * of counts and callbacks. (Callbacks ready for invocation must be
> - * orphaned and adopted separately from pending callbacks, but counts
> - * apply to all callbacks. Locking must be used to make sure that
> - * both orphaned-callbacks lists are consistent.)
> - */
> -void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> - struct rcu_cblist *rclp)
> -{
> - rclp->len = rcu_segcblist_xchg_len(rsclp, 0);
> -}
> -
> /*
> * Extract only those callbacks ready to be invoked from the specified
> * rcu_segcblist structure and place them in the specified rcu_cblist
> @@ -307,6 +317,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 +325,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 +342,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);
> + }
> }
>
> /*
> @@ -345,7 +362,6 @@ void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp)
> {
> rcu_segcblist_add_len(rsclp, rclp->len);
> - rclp->len = 0;
> }
>
> /*
> @@ -359,6 +375,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 +396,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 +422,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 +443,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 +465,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 +508,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"
> @@ -514,13 +539,24 @@ void rcu_segcblist_merge(struct rcu_segcblist *dst_rsclp,
> struct rcu_cblist donecbs;
> struct rcu_cblist pendcbs;
>
> + lockdep_assert_cpus_held();
> +
> rcu_cblist_init(&donecbs);
> rcu_cblist_init(&pendcbs);
> - rcu_segcblist_extract_count(src_rsclp, &donecbs);
> +
> rcu_segcblist_extract_done_cbs(src_rsclp, &donecbs);
> rcu_segcblist_extract_pend_cbs(src_rsclp, &pendcbs);
> +
> + /*
> + * No need smp_mb() before setting length to 0, because CPU hotplug
> + * lock excludes rcu_barrier.
> + */
> + rcu_segcblist_set_len(src_rsclp, 0);
> +
> rcu_segcblist_insert_count(dst_rsclp, &donecbs);
> + rcu_segcblist_insert_count(dst_rsclp, &pendcbs);
> rcu_segcblist_insert_done_cbs(dst_rsclp, &donecbs);
> rcu_segcblist_insert_pend_cbs(dst_rsclp, &pendcbs);
> +
> rcu_segcblist_init(src_rsclp);
> }
> diff --git a/kernel/rcu/rcu_segcblist.h b/kernel/rcu/rcu_segcblist.h
> index 1d2d61406463..cd35c9faaf51 100644
> --- a/kernel/rcu/rcu_segcblist.h
> +++ b/kernel/rcu/rcu_segcblist.h
> @@ -89,8 +89,6 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> struct rcu_head *rhp);
> bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> struct rcu_head *rhp);
> -void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> - struct rcu_cblist *rclp);
> void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp);
> void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation
On Tue, Nov 03, 2020 at 09:25:58AM -0500, 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.
>
> Also this patch remove hacks related to using donecbs's ->len field as a
> temporary variable to save the segmented callback list's length. This cannot be
> done anymore and is not needed.
>
> Reviewed-by: Frederic Weisbecker <[email protected]>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
Thank you both!
A few questions and comments below.
Thanx, Paul
> ---
> include/linux/rcu_segcblist.h | 1 +
> kernel/rcu/rcu_segcblist.c | 120 ++++++++++++++++++++++------------
> kernel/rcu/rcu_segcblist.h | 2 -
> 3 files changed, 79 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/rcu_segcblist.h b/include/linux/rcu_segcblist.h
> index b36afe7b22c9..6c01f09a6456 100644
> --- a/include/linux/rcu_segcblist.h
> +++ b/include/linux/rcu_segcblist.h
> @@ -72,6 +72,7 @@ struct rcu_segcblist {
> #else
> long len;
> #endif
> + long seglen[RCU_CBLIST_NSEGS];
> u8 enabled;
> u8 offloaded;
> };
> diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> index bb246d8c6ef1..357c19bbcb00 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -7,10 +7,11 @@
> * Authors: Paul E. McKenney <[email protected]>
> */
>
> -#include <linux/types.h>
> -#include <linux/kernel.h>
> +#include <linux/cpu.h>
> #include <linux/interrupt.h>
> +#include <linux/kernel.h>
> #include <linux/rcupdate.h>
> +#include <linux/types.h>
>
> #include "rcu_segcblist.h"
>
> @@ -88,6 +89,46 @@ 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)
> +{
> + return READ_ONCE(rsclp->seglen[seg]);
> +}
> +
> +/* 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)
> +{
> + WRITE_ONCE(rsclp->seglen[seg], v);
> +}
> +
> +/* Return number of callbacks in a segment of the segmented callback list. */
A casual reader might be forgiven for being confused by the combination
of "Return" in the above comment and the "void" function type below.
So shouldn't this comment be something like "Add the specified number
of callbacks to the specified segment..."?
> +static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
> +{
> + WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
> +}
> +
> +/* 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
> @@ -119,26 +160,6 @@ void rcu_segcblist_inc_len(struct rcu_segcblist *rsclp)
> rcu_segcblist_add_len(rsclp, 1);
> }
>
> -/*
> - * Exchange the numeric length of the specified rcu_segcblist structure
> - * with the specified value. This can cause the ->len field to disagree
> - * with the actual number of callbacks on the structure. This exchange is
> - * fully ordered with respect to the callers accesses both before and after.
> - */
> -static long rcu_segcblist_xchg_len(struct rcu_segcblist *rsclp, long v)
> -{
> -#ifdef CONFIG_RCU_NOCB_CPU
> - return atomic_long_xchg(&rsclp->len, v);
> -#else
> - long ret = rsclp->len;
> -
> - smp_mb(); /* Up to the caller! */
> - WRITE_ONCE(rsclp->len, v);
> - smp_mb(); /* Up to the caller! */
> - return ret;
> -#endif
> -}
> -
> /*
> * Initialize an rcu_segcblist structure.
> */
> @@ -149,8 +170,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;
> }
> @@ -246,6 +269,7 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> {
> rcu_segcblist_inc_len(rsclp);
> smp_mb(); /* Ensure counts are updated before callback is enqueued. */
> + rcu_segcblist_inc_seglen(rsclp, RCU_NEXT_TAIL);
> rhp->next = NULL;
> WRITE_ONCE(*rsclp->tails[RCU_NEXT_TAIL], rhp);
> WRITE_ONCE(rsclp->tails[RCU_NEXT_TAIL], &rhp->next);
> @@ -274,27 +298,13 @@ 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);
> return true;
> }
>
> -/*
> - * Extract only the counts from the specified rcu_segcblist structure,
> - * and place them in the specified rcu_cblist structure. This function
> - * supports both callback orphaning and invocation, hence the separation
> - * of counts and callbacks. (Callbacks ready for invocation must be
> - * orphaned and adopted separately from pending callbacks, but counts
> - * apply to all callbacks. Locking must be used to make sure that
> - * both orphaned-callbacks lists are consistent.)
> - */
> -void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> - struct rcu_cblist *rclp)
> -{
> - rclp->len = rcu_segcblist_xchg_len(rsclp, 0);
> -}
> -
> /*
> * Extract only those callbacks ready to be invoked from the specified
> * rcu_segcblist structure and place them in the specified rcu_cblist
> @@ -307,6 +317,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 +325,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 +342,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);
This should be a "for" loop. Yes, the number and names of the segments
hasn't changed for a good long time, but nothing like code as above to
inspire Murphy to more mischief. :-/
Actually, why not put the summation in the existing "for" loop below?
That would save a line of code in addition to providing less inspiration
for Mr. Murphy.
> *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);
> + }
> }
>
> /*
> @@ -345,7 +362,6 @@ void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp)
> {
> rcu_segcblist_add_len(rsclp, rclp->len);
> - rclp->len = 0;
You audited the callers, correct?
> }
>
> /*
> @@ -359,6 +375,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 +396,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 +422,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 +443,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 +465,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 +508,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"
> @@ -514,13 +539,24 @@ void rcu_segcblist_merge(struct rcu_segcblist *dst_rsclp,
> struct rcu_cblist donecbs;
> struct rcu_cblist pendcbs;
>
> + lockdep_assert_cpus_held();
> +
> rcu_cblist_init(&donecbs);
> rcu_cblist_init(&pendcbs);
> - rcu_segcblist_extract_count(src_rsclp, &donecbs);
> +
> rcu_segcblist_extract_done_cbs(src_rsclp, &donecbs);
> rcu_segcblist_extract_pend_cbs(src_rsclp, &pendcbs);
> +
> + /*
> + * No need smp_mb() before setting length to 0, because CPU hotplug
> + * lock excludes rcu_barrier.
> + */
> + rcu_segcblist_set_len(src_rsclp, 0);
> +
> rcu_segcblist_insert_count(dst_rsclp, &donecbs);
> + rcu_segcblist_insert_count(dst_rsclp, &pendcbs);
> rcu_segcblist_insert_done_cbs(dst_rsclp, &donecbs);
> rcu_segcblist_insert_pend_cbs(dst_rsclp, &pendcbs);
> +
> rcu_segcblist_init(src_rsclp);
> }
> diff --git a/kernel/rcu/rcu_segcblist.h b/kernel/rcu/rcu_segcblist.h
> index 1d2d61406463..cd35c9faaf51 100644
> --- a/kernel/rcu/rcu_segcblist.h
> +++ b/kernel/rcu/rcu_segcblist.h
> @@ -89,8 +89,6 @@ void rcu_segcblist_enqueue(struct rcu_segcblist *rsclp,
> struct rcu_head *rhp);
> bool rcu_segcblist_entrain(struct rcu_segcblist *rsclp,
> struct rcu_head *rhp);
> -void rcu_segcblist_extract_count(struct rcu_segcblist *rsclp,
> - struct rcu_cblist *rclp);
> void rcu_segcblist_extract_done_cbs(struct rcu_segcblist *rsclp,
> struct rcu_cblist *rclp);
> void rcu_segcblist_extract_pend_cbs(struct rcu_segcblist *rsclp,
> --
> 2.29.1.341.ge80a0c044ae-goog
>
On Wed, Nov 04, 2020 at 09:01:33AM -0800, Paul E. McKenney wrote:
> A casual reader might be forgiven for being confused by the combination
> of "Return" in the above comment and the "void" function type below.
> So shouldn't this comment be something like "Add the specified number
> of callbacks to the specified segment..."?
You are right, sorry and will fix it.
> > @@ -330,11 +342,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);
>
> This should be a "for" loop. Yes, the number and names of the segments
> hasn't changed for a good long time, but nothing like code as above to
> inspire Murphy to more mischief. :-/
>
> Actually, why not put the summation in the existing "for" loop below?
> That would save a line of code in addition to providing less inspiration
> for Mr. Murphy.
I can do that. Actually Frederic suggested the same thing but I was reluctant
as I felt it did not give much LOC benefit. Will revisit it.
>
> > *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);
> > + }
> > }
> >
> > /*
> > @@ -345,7 +362,6 @@ void rcu_segcblist_insert_count(struct rcu_segcblist *rsclp,
> > struct rcu_cblist *rclp)
> > {
> > rcu_segcblist_add_len(rsclp, rclp->len);
> > - rclp->len = 0;
>
> You audited the callers, correct?
Yep.
thanks,
- Joel
On Fri, Nov 06, 2020 at 07:01:57PM -0500, Joel Fernandes wrote:
> On Wed, Nov 04, 2020 at 09:01:33AM -0800, Paul E. McKenney wrote:
>
> > A casual reader might be forgiven for being confused by the combination
> > of "Return" in the above comment and the "void" function type below.
> > So shouldn't this comment be something like "Add the specified number
> > of callbacks to the specified segment..."?
>
> You are right, sorry and will fix it.
>
> > > @@ -330,11 +342,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);
> >
> > This should be a "for" loop. Yes, the number and names of the segments
> > hasn't changed for a good long time, but nothing like code as above to
> > inspire Murphy to more mischief. :-/
> >
> > Actually, why not put the summation in the existing "for" loop below?
> > That would save a line of code in addition to providing less inspiration
> > for Mr. Murphy.
>
> I can do that. Actually Frederic suggested the same thing but I was reluctant
> as I felt it did not give much LOC benefit. Will revisit it.
It reduces 1 line of code :) I changed it to the below, will update the patch:
---8<-----------------------
diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
index 9b43d686b1f3..bff9b2253e50 100644
--- a/kernel/rcu/rcu_segcblist.c
+++ b/kernel/rcu/rcu_segcblist.c
@@ -101,7 +101,7 @@ static void rcu_segcblist_set_seglen(struct rcu_segcblist *rsclp, int seg, long
WRITE_ONCE(rsclp->seglen[seg], v);
}
-/* Return number of callbacks in a segment of the segmented callback list. */
+/* Increase the numeric length of a segment by a specified amount. */
static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
{
WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
@@ -406,13 +406,12 @@ 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->len = 0;
*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++) {
+ rclp->len += rcu_segcblist_get_seglen(rsclp, i);
WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_DONE_TAIL]);
rcu_segcblist_set_seglen(rsclp, i, 0);
}
On Fri, Nov 06, 2020 at 07:18:47PM -0500, Joel Fernandes wrote:
> On Fri, Nov 06, 2020 at 07:01:57PM -0500, Joel Fernandes wrote:
> > On Wed, Nov 04, 2020 at 09:01:33AM -0800, Paul E. McKenney wrote:
> >
> > > A casual reader might be forgiven for being confused by the combination
> > > of "Return" in the above comment and the "void" function type below.
> > > So shouldn't this comment be something like "Add the specified number
> > > of callbacks to the specified segment..."?
> >
> > You are right, sorry and will fix it.
> >
> > > > @@ -330,11 +342,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);
> > >
> > > This should be a "for" loop. Yes, the number and names of the segments
> > > hasn't changed for a good long time, but nothing like code as above to
> > > inspire Murphy to more mischief. :-/
> > >
> > > Actually, why not put the summation in the existing "for" loop below?
> > > That would save a line of code in addition to providing less inspiration
> > > for Mr. Murphy.
> >
> > I can do that. Actually Frederic suggested the same thing but I was reluctant
> > as I felt it did not give much LOC benefit. Will revisit it.
>
> It reduces 1 line of code :) I changed it to the below, will update the patch:
Thank you! And yes, I am much more concerned about the constraints on
Mr. Murphy than on the lines of code. ;-)
Thanx, Paul
> ---8<-----------------------
>
> diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> index 9b43d686b1f3..bff9b2253e50 100644
> --- a/kernel/rcu/rcu_segcblist.c
> +++ b/kernel/rcu/rcu_segcblist.c
> @@ -101,7 +101,7 @@ static void rcu_segcblist_set_seglen(struct rcu_segcblist *rsclp, int seg, long
> WRITE_ONCE(rsclp->seglen[seg], v);
> }
>
> -/* Return number of callbacks in a segment of the segmented callback list. */
> +/* Increase the numeric length of a segment by a specified amount. */
> static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
> {
> WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
> @@ -406,13 +406,12 @@ 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->len = 0;
> *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++) {
> + rclp->len += rcu_segcblist_get_seglen(rsclp, i);
> WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_DONE_TAIL]);
> rcu_segcblist_set_seglen(rsclp, i, 0);
> }
On Fri, Nov 06, 2020 at 04:48:15PM -0800, Paul E. McKenney wrote:
> On Fri, Nov 06, 2020 at 07:18:47PM -0500, Joel Fernandes wrote:
> > On Fri, Nov 06, 2020 at 07:01:57PM -0500, Joel Fernandes wrote:
> > > On Wed, Nov 04, 2020 at 09:01:33AM -0800, Paul E. McKenney wrote:
> > >
> > > > A casual reader might be forgiven for being confused by the combination
> > > > of "Return" in the above comment and the "void" function type below.
> > > > So shouldn't this comment be something like "Add the specified number
> > > > of callbacks to the specified segment..."?
> > >
> > > You are right, sorry and will fix it.
> > >
> > > > > @@ -330,11 +342,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);
> > > >
> > > > This should be a "for" loop. Yes, the number and names of the segments
> > > > hasn't changed for a good long time, but nothing like code as above to
> > > > inspire Murphy to more mischief. :-/
> > > >
> > > > Actually, why not put the summation in the existing "for" loop below?
> > > > That would save a line of code in addition to providing less inspiration
> > > > for Mr. Murphy.
> > >
> > > I can do that. Actually Frederic suggested the same thing but I was reluctant
> > > as I felt it did not give much LOC benefit. Will revisit it.
> >
> > It reduces 1 line of code :) I changed it to the below, will update the patch:
>
> Thank you! And yes, I am much more concerned about the constraints on
> Mr. Murphy than on the lines of code. ;-)
And I have pulled in the updated commit, thank you all!
Thanx, Paul
> > ---8<-----------------------
> >
> > diff --git a/kernel/rcu/rcu_segcblist.c b/kernel/rcu/rcu_segcblist.c
> > index 9b43d686b1f3..bff9b2253e50 100644
> > --- a/kernel/rcu/rcu_segcblist.c
> > +++ b/kernel/rcu/rcu_segcblist.c
> > @@ -101,7 +101,7 @@ static void rcu_segcblist_set_seglen(struct rcu_segcblist *rsclp, int seg, long
> > WRITE_ONCE(rsclp->seglen[seg], v);
> > }
> >
> > -/* Return number of callbacks in a segment of the segmented callback list. */
> > +/* Increase the numeric length of a segment by a specified amount. */
> > static void rcu_segcblist_add_seglen(struct rcu_segcblist *rsclp, int seg, long v)
> > {
> > WRITE_ONCE(rsclp->seglen[seg], rsclp->seglen[seg] + v);
> > @@ -406,13 +406,12 @@ 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->len = 0;
> > *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++) {
> > + rclp->len += rcu_segcblist_get_seglen(rsclp, i);
> > WRITE_ONCE(rsclp->tails[i], rsclp->tails[RCU_DONE_TAIL]);
> > rcu_segcblist_set_seglen(rsclp, i, 0);
> > }