LinuxLists.cc - [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

[permalink] [raw]

Subject: [PATCH v2 rcu/dev 2/2] rcu: Disable laziness if lazy-tracking says so

During suspend, we see failures to suspend 1 in 300-500 suspends.
Looking closer, it appears that asynchronous RCU callbacks are being
queued as lazy even though synchronous callbacks are expedited. These
delays appear to not be very welcome by the suspend/resume code as
evidenced by these occasional suspend failures.

This commit modifies call_rcu() to check if rcu_async_should_hurry(),
which will return true if we are in suspend or in-kernel boot.

Ignoring the lazy hint makes the 3000 suspend/resume cycles pass
reliably on a 12th gen 12-core Intel CPU, and there is some evidence
that it also slightly speeds up boot performance.

Signed-off-by: Joel Fernandes (Google) <[email protected]>
---
kernel/rcu/tree.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 78b2e999c904..c71e38d7815a 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2594,12 +2594,12 @@ static void check_cb_ovld(struct rcu_data *rdp)
}

static void
-__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
+__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
{
static atomic_t doublefrees;
unsigned long flags;
struct rcu_data *rdp;
- bool was_alldone;
+ bool was_alldone, lazy;

/* Misaligned rcu_head! */
WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1));
@@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
kasan_record_aux_stack_noalloc(head);
local_irq_save(flags);
rdp = this_cpu_ptr(&rcu_data);
+ lazy = lazy_in && !rcu_async_should_hurry();

/* Add the callback to our list. */
if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) {
--
2.39.0.314.g84b9a713c41-goog

2023-01-12 02:25:03

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

> On Jan 11, 2023, at 8:29 PM, Paul E. McKenney <[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 12:52:22AM +0000, Joel Fernandes (Google) wrote:
>> During boot and suspend/resume, it is desired to prevent RCU laziness from
>> effecting performance and in some cases failures like with suspend.
>>
>> Track whether RCU laziness is to be ignored or not, in kernels with
>> CONFIG_RCU_LAZY enabled. We do such tracking for expedited-RCU already, however
>> since Android currently expedites synchronous_rcu() always, we cannot rely on
>> that. The next patch ignores laziness hints based on this tracking.
>>
>> Signed-off-by: Joel Fernandes (Google) <[email protected]>
>> ---
>> Paul, could we take this and the next one for 6.2 -rc cycle?
>
> Given how late it is in v6.2, I would need you to make it all depend on
> CONFIG_RCU_LAZY, so that it is blindingly and immediately obvious that
> there is no change in behavior for kernels built with CONFIG_RCU_LAZY=n.
>
> If you are OK with the v6.3 merge window and backports, what you have
> likely suffices, modulo review and testing.
>
> So, how would you like to proceed?

Yes that is fine with me. Though, it is right now fully compile-time dependent on the lazy config, except the definition of the integer which I can’t put behind a config because of ugly ifdefs. I would expect LTO to remove the integer definition automatically. But it has no effect otherwise.

I do worry a bit about users who are not using stable and have their own kernel versions with backports. But maybe a fixes tag will get their attention?

Android and ChromeOS should both be fine though, given they swear by stable.

Thanks,

- Joel

>
> Thanx, Paul
>
>> I also booted debian Linux and verified the flag is reset correctly after boot
>> completes. Thanks.
>>
>> kernel/rcu/rcu.h | 6 ++++++
>> kernel/rcu/tree.c | 2 ++
>> kernel/rcu/update.c | 40 +++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 55 insertions(+), 1 deletion(-)
>> create mode 100644 cc_list
>> create mode 100644 to_list
>>
>> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
>> index 5c8013f7085f..115616ac3bfa 100644
>> --- a/kernel/rcu/rcu.h
>> +++ b/kernel/rcu/rcu.h
>> @@ -449,14 +449,20 @@ do { \
>> /* Tiny RCU doesn't expedite, as its purpose in life is instead to be tiny. */
>> static inline bool rcu_gp_is_normal(void) { return true; }
>> static inline bool rcu_gp_is_expedited(void) { return false; }
>> +static inline bool rcu_async_should_hurry(void) { return false; }
>> static inline void rcu_expedite_gp(void) { }
>> static inline void rcu_unexpedite_gp(void) { }
>> +static inline void rcu_async_hurry(void) { }
>> +static inline void rcu_async_relax(void) { }
>> static inline void rcu_request_urgent_qs_task(struct task_struct *t) { }
>> #else /* #ifdef CONFIG_TINY_RCU */
>> bool rcu_gp_is_normal(void); /* Internal RCU use. */
>> bool rcu_gp_is_expedited(void); /* Internal RCU use. */
>> +bool rcu_async_should_hurry(void); /* Internal RCU use. */
>> void rcu_expedite_gp(void);
>> void rcu_unexpedite_gp(void);
>> +void rcu_async_hurry(void);
>> +void rcu_async_relax(void);
>> void rcupdate_announce_bootup_oddness(void);
>> #ifdef CONFIG_TASKS_RCU_GENERIC
>> void show_rcu_tasks_gp_kthreads(void);
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 63545d79da51..78b2e999c904 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -4504,11 +4504,13 @@ static int rcu_pm_notify(struct notifier_block *self,
>> switch (action) {
>> case PM_HIBERNATION_PREPARE:
>> case PM_SUSPEND_PREPARE:
>> + rcu_async_hurry();
>> rcu_expedite_gp();
>> break;
>> case PM_POST_HIBERNATION:
>> case PM_POST_SUSPEND:
>> rcu_unexpedite_gp();
>> + rcu_async_relax();
>> break;
>> default:
>> break;
>> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
>> index 3893022f8ed8..19bf6fa3ee6a 100644
>> --- a/kernel/rcu/update.c
>> +++ b/kernel/rcu/update.c
>> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
>> }
>> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
>>
>> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
>> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
>> +/*
>> + * Should call_rcu() callbacks be processed with urgency or are
>> + * they OK being executed with arbitrary delays?
>> + */
>> +bool rcu_async_should_hurry(void)
>> +{
>> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
>> + atomic_read(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
>> +
>> +/**
>> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
>> + *
>> + * After a call to this function, future calls to call_rcu()
>> + * will be processed in a timely fashion.
>> + */
>> +void rcu_async_hurry(void)
>> +{
>> + if (IS_ENABLED(CONFIG_RCU_LAZY))
>> + atomic_inc(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
>>
>> +/**
>> + * rcu_async_relax - Make future async RCU callbacks lazy.
>> + *
>> + * After a call to this function, future calls to call_rcu()
>> + * will be processed in a lazy fashion.
>> + */
>> +void rcu_async_relax(void)
>> +{
>> + if (IS_ENABLED(CONFIG_RCU_LAZY))
>> + atomic_dec(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_relax);
>> +
>> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
>> /*
>> * Should normal grace-period primitives be expedited? Intended for
>> * use within RCU. Note that this function takes the rcu_expedited
>> @@ -195,6 +232,7 @@ static bool rcu_boot_ended __read_mostly;
>> void rcu_end_inkernel_boot(void)
>> {
>> rcu_unexpedite_gp();
>> + rcu_async_relax();
>> if (rcu_normal_after_boot)
>> WRITE_ONCE(rcu_normal, 1);
>> rcu_boot_ended = true;
>> --
>> 2.39.0.314.g84b9a713c41-goog

2023-01-12 02:26:31

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Thu, Jan 12, 2023 at 12:52:22AM +0000, Joel Fernandes (Google) wrote:
> During boot and suspend/resume, it is desired to prevent RCU laziness from
> effecting performance and in some cases failures like with suspend.
>
> Track whether RCU laziness is to be ignored or not, in kernels with
> CONFIG_RCU_LAZY enabled. We do such tracking for expedited-RCU already, however
> since Android currently expedites synchronous_rcu() always, we cannot rely on
> that. The next patch ignores laziness hints based on this tracking.
>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> Paul, could we take this and the next one for 6.2 -rc cycle?

Given how late it is in v6.2, I would need you to make it all depend on
CONFIG_RCU_LAZY, so that it is blindingly and immediately obvious that
there is no change in behavior for kernels built with CONFIG_RCU_LAZY=n.

If you are OK with the v6.3 merge window and backports, what you have
likely suffices, modulo review and testing.

So, how would you like to proceed?

Thanx, Paul

> I also booted debian Linux and verified the flag is reset correctly after boot
> completes. Thanks.
>
> kernel/rcu/rcu.h | 6 ++++++
> kernel/rcu/tree.c | 2 ++
> kernel/rcu/update.c | 40 +++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 55 insertions(+), 1 deletion(-)
> create mode 100644 cc_list
> create mode 100644 to_list
>
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index 5c8013f7085f..115616ac3bfa 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -449,14 +449,20 @@ do { \
> /* Tiny RCU doesn't expedite, as its purpose in life is instead to be tiny. */
> static inline bool rcu_gp_is_normal(void) { return true; }
> static inline bool rcu_gp_is_expedited(void) { return false; }
> +static inline bool rcu_async_should_hurry(void) { return false; }
> static inline void rcu_expedite_gp(void) { }
> static inline void rcu_unexpedite_gp(void) { }
> +static inline void rcu_async_hurry(void) { }
> +static inline void rcu_async_relax(void) { }
> static inline void rcu_request_urgent_qs_task(struct task_struct *t) { }
> #else /* #ifdef CONFIG_TINY_RCU */
> bool rcu_gp_is_normal(void); /* Internal RCU use. */
> bool rcu_gp_is_expedited(void); /* Internal RCU use. */
> +bool rcu_async_should_hurry(void); /* Internal RCU use. */
> void rcu_expedite_gp(void);
> void rcu_unexpedite_gp(void);
> +void rcu_async_hurry(void);
> +void rcu_async_relax(void);
> void rcupdate_announce_bootup_oddness(void);
> #ifdef CONFIG_TASKS_RCU_GENERIC
> void show_rcu_tasks_gp_kthreads(void);
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 63545d79da51..78b2e999c904 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4504,11 +4504,13 @@ static int rcu_pm_notify(struct notifier_block *self,
> switch (action) {
> case PM_HIBERNATION_PREPARE:
> case PM_SUSPEND_PREPARE:
> + rcu_async_hurry();
> rcu_expedite_gp();
> break;
> case PM_POST_HIBERNATION:
> case PM_POST_SUSPEND:
> rcu_unexpedite_gp();
> + rcu_async_relax();
> break;
> default:
> break;
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 3893022f8ed8..19bf6fa3ee6a 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> }
> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
>
> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> +/*
> + * Should call_rcu() callbacks be processed with urgency or are
> + * they OK being executed with arbitrary delays?
> + */
> +bool rcu_async_should_hurry(void)
> +{
> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> + atomic_read(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> +
> +/**
> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a timely fashion.
> + */
> +void rcu_async_hurry(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_inc(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
>
> +/**
> + * rcu_async_relax - Make future async RCU callbacks lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a lazy fashion.
> + */
> +void rcu_async_relax(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_dec(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_relax);
> +
> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> /*
> * Should normal grace-period primitives be expedited? Intended for
> * use within RCU. Note that this function takes the rcu_expedited
> @@ -195,6 +232,7 @@ static bool rcu_boot_ended __read_mostly;
> void rcu_end_inkernel_boot(void)
> {
> rcu_unexpedite_gp();
> + rcu_async_relax();
> if (rcu_normal_after_boot)
> WRITE_ONCE(rcu_normal, 1);
> rcu_boot_ended = true;
> --
> 2.39.0.314.g84b9a713c41-goog

2023-01-12 19:56:08

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Thu, Jan 12, 2023 at 12:52:22AM +0000, Joel Fernandes (Google) wrote:
> During boot and suspend/resume, it is desired to prevent RCU laziness from
> effecting performance and in some cases failures like with suspend.
>
> Track whether RCU laziness is to be ignored or not, in kernels with
> CONFIG_RCU_LAZY enabled. We do such tracking for expedited-RCU already, however
> since Android currently expedites synchronous_rcu() always, we cannot rely on
> that. The next patch ignores laziness hints based on this tracking.
>
> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> ---
> Paul, could we take this and the next one for 6.2 -rc cycle?
>
> I also booted debian Linux and verified the flag is reset correctly after boot
> completes. Thanks.

After going back and forth a bit, I took these two as-is (aside from
the usual commit-log wordsmithing).

At some point, the state-change-notification from things like boot,
suspend/resume, sysrq-t, and panic() should probably be refactored.
But I do not believe that we are yet at that point, and there is not
much point in half-refactorings in cases such as this one.

I added the Fixes tags, and, if testing goes well, I plan to submit
them into the upcoming merge window. I have no reason to believe that
anyone is hitting this, it is only a few weeks away anyhow, and a good
chunk of that would be consumed by testing and review.

And if someone does hit it, we do have your fixes to send to them, so
thank you for that!

(These won't become visible on -rcu until I complete today's rebase,
in case you were wondering.)

Thanx, Paul

> kernel/rcu/rcu.h | 6 ++++++
> kernel/rcu/tree.c | 2 ++
> kernel/rcu/update.c | 40 +++++++++++++++++++++++++++++++++++++++-
> 5 files changed, 55 insertions(+), 1 deletion(-)
> create mode 100644 cc_list
> create mode 100644 to_list
>
> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> index 5c8013f7085f..115616ac3bfa 100644
> --- a/kernel/rcu/rcu.h
> +++ b/kernel/rcu/rcu.h
> @@ -449,14 +449,20 @@ do { \
> /* Tiny RCU doesn't expedite, as its purpose in life is instead to be tiny. */
> static inline bool rcu_gp_is_normal(void) { return true; }
> static inline bool rcu_gp_is_expedited(void) { return false; }
> +static inline bool rcu_async_should_hurry(void) { return false; }
> static inline void rcu_expedite_gp(void) { }
> static inline void rcu_unexpedite_gp(void) { }
> +static inline void rcu_async_hurry(void) { }
> +static inline void rcu_async_relax(void) { }
> static inline void rcu_request_urgent_qs_task(struct task_struct *t) { }
> #else /* #ifdef CONFIG_TINY_RCU */
> bool rcu_gp_is_normal(void); /* Internal RCU use. */
> bool rcu_gp_is_expedited(void); /* Internal RCU use. */
> +bool rcu_async_should_hurry(void); /* Internal RCU use. */
> void rcu_expedite_gp(void);
> void rcu_unexpedite_gp(void);
> +void rcu_async_hurry(void);
> +void rcu_async_relax(void);
> void rcupdate_announce_bootup_oddness(void);
> #ifdef CONFIG_TASKS_RCU_GENERIC
> void show_rcu_tasks_gp_kthreads(void);
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 63545d79da51..78b2e999c904 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -4504,11 +4504,13 @@ static int rcu_pm_notify(struct notifier_block *self,
> switch (action) {
> case PM_HIBERNATION_PREPARE:
> case PM_SUSPEND_PREPARE:
> + rcu_async_hurry();
> rcu_expedite_gp();
> break;
> case PM_POST_HIBERNATION:
> case PM_POST_SUSPEND:
> rcu_unexpedite_gp();
> + rcu_async_relax();
> break;
> default:
> break;
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 3893022f8ed8..19bf6fa3ee6a 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> }
> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
>
> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> +/*
> + * Should call_rcu() callbacks be processed with urgency or are
> + * they OK being executed with arbitrary delays?
> + */
> +bool rcu_async_should_hurry(void)
> +{
> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> + atomic_read(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> +
> +/**
> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a timely fashion.
> + */
> +void rcu_async_hurry(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_inc(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
>
> +/**
> + * rcu_async_relax - Make future async RCU callbacks lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a lazy fashion.
> + */
> +void rcu_async_relax(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_dec(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_relax);
> +
> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> /*
> * Should normal grace-period primitives be expedited? Intended for
> * use within RCU. Note that this function takes the rcu_expedited
> @@ -195,6 +232,7 @@ static bool rcu_boot_ended __read_mostly;
> void rcu_end_inkernel_boot(void)
> {
> rcu_unexpedite_gp();
> + rcu_async_relax();
> if (rcu_normal_after_boot)
> WRITE_ONCE(rcu_normal, 1);
> rcu_boot_ended = true;
> --
> 2.39.0.314.g84b9a713c41-goog

2023-01-12 23:03:49

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

> On Jan 12, 2023, at 2:27 PM, Paul E. McKenney <[email protected]> wrote:
>
> On Thu, Jan 12, 2023 at 12:52:22AM +0000, Joel Fernandes (Google) wrote:
>> During boot and suspend/resume, it is desired to prevent RCU laziness from
>> effecting performance and in some cases failures like with suspend.
>>
>> Track whether RCU laziness is to be ignored or not, in kernels with
>> CONFIG_RCU_LAZY enabled. We do such tracking for expedited-RCU already, however
>> since Android currently expedites synchronous_rcu() always, we cannot rely on
>> that. The next patch ignores laziness hints based on this tracking.
>>
>> Signed-off-by: Joel Fernandes (Google) <[email protected]>
>> ---
>> Paul, could we take this and the next one for 6.2 -rc cycle?
>>
>> I also booted debian Linux and verified the flag is reset correctly after boot
>> completes. Thanks.
>
> After going back and forth a bit, I took these two as-is (aside from
> the usual commit-log wordsmithing).
>
> At some point, the state-change-notification from things like boot,
> suspend/resume, sysrq-t, and panic() should probably be refactored.
> But I do not believe that we are yet at that point, and there is not
> much point in half-refactorings in cases such as this one.
>
> I added the Fixes tags, and, if testing goes well, I plan to submit
> them into the upcoming merge window. I have no reason to believe that
> anyone is hitting this, it is only a few weeks away anyhow, and a good
> chunk of that would be consumed by testing and review.
>
> And if someone does hit it, we do have your fixes to send to them, so
> thank you for that!
> (These won't become visible on -rcu until I complete today's rebase,
> in case you were wondering.)

Thanks and this sounds good to me. Meanwhile I pulled into our product kernel so all is good.

Thanks,

- Joel

>
> Thanx, Paul
>
>> kernel/rcu/rcu.h | 6 ++++++
>> kernel/rcu/tree.c | 2 ++
>> kernel/rcu/update.c | 40 +++++++++++++++++++++++++++++++++++++++-
>> 5 files changed, 55 insertions(+), 1 deletion(-)
>> create mode 100644 cc_list
>> create mode 100644 to_list
>>
>> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
>> index 5c8013f7085f..115616ac3bfa 100644
>> --- a/kernel/rcu/rcu.h
>> +++ b/kernel/rcu/rcu.h
>> @@ -449,14 +449,20 @@ do { \
>> /* Tiny RCU doesn't expedite, as its purpose in life is instead to be tiny. */
>> static inline bool rcu_gp_is_normal(void) { return true; }
>> static inline bool rcu_gp_is_expedited(void) { return false; }
>> +static inline bool rcu_async_should_hurry(void) { return false; }
>> static inline void rcu_expedite_gp(void) { }
>> static inline void rcu_unexpedite_gp(void) { }
>> +static inline void rcu_async_hurry(void) { }
>> +static inline void rcu_async_relax(void) { }
>> static inline void rcu_request_urgent_qs_task(struct task_struct *t) { }
>> #else /* #ifdef CONFIG_TINY_RCU */
>> bool rcu_gp_is_normal(void); /* Internal RCU use. */
>> bool rcu_gp_is_expedited(void); /* Internal RCU use. */
>> +bool rcu_async_should_hurry(void); /* Internal RCU use. */
>> void rcu_expedite_gp(void);
>> void rcu_unexpedite_gp(void);
>> +void rcu_async_hurry(void);
>> +void rcu_async_relax(void);
>> void rcupdate_announce_bootup_oddness(void);
>> #ifdef CONFIG_TASKS_RCU_GENERIC
>> void show_rcu_tasks_gp_kthreads(void);
>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>> index 63545d79da51..78b2e999c904 100644
>> --- a/kernel/rcu/tree.c
>> +++ b/kernel/rcu/tree.c
>> @@ -4504,11 +4504,13 @@ static int rcu_pm_notify(struct notifier_block *self,
>> switch (action) {
>> case PM_HIBERNATION_PREPARE:
>> case PM_SUSPEND_PREPARE:
>> + rcu_async_hurry();
>> rcu_expedite_gp();
>> break;
>> case PM_POST_HIBERNATION:
>> case PM_POST_SUSPEND:
>> rcu_unexpedite_gp();
>> + rcu_async_relax();
>> break;
>> default:
>> break;
>> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
>> index 3893022f8ed8..19bf6fa3ee6a 100644
>> --- a/kernel/rcu/update.c
>> +++ b/kernel/rcu/update.c
>> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
>> }
>> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
>>
>> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
>> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
>> +/*
>> + * Should call_rcu() callbacks be processed with urgency or are
>> + * they OK being executed with arbitrary delays?
>> + */
>> +bool rcu_async_should_hurry(void)
>> +{
>> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
>> + atomic_read(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
>> +
>> +/**
>> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
>> + *
>> + * After a call to this function, future calls to call_rcu()
>> + * will be processed in a timely fashion.
>> + */
>> +void rcu_async_hurry(void)
>> +{
>> + if (IS_ENABLED(CONFIG_RCU_LAZY))
>> + atomic_inc(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
>>
>> +/**
>> + * rcu_async_relax - Make future async RCU callbacks lazy.
>> + *
>> + * After a call to this function, future calls to call_rcu()
>> + * will be processed in a lazy fashion.
>> + */
>> +void rcu_async_relax(void)
>> +{
>> + if (IS_ENABLED(CONFIG_RCU_LAZY))
>> + atomic_dec(&rcu_async_hurry_nesting);
>> +}
>> +EXPORT_SYMBOL_GPL(rcu_async_relax);
>> +
>> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
>> /*
>> * Should normal grace-period primitives be expedited? Intended for
>> * use within RCU. Note that this function takes the rcu_expedited
>> @@ -195,6 +232,7 @@ static bool rcu_boot_ended __read_mostly;
>> void rcu_end_inkernel_boot(void)
>> {
>> rcu_unexpedite_gp();
>> + rcu_async_relax();
>> if (rcu_normal_after_boot)
>> WRITE_ONCE(rcu_normal, 1);
>> rcu_boot_ended = true;
>> --
>> 2.39.0.314.g84b9a713c41-goog

2023-01-13 00:04:49

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Thu, Jan 12, 2023 at 05:25:51PM -0500, Joel Fernandes wrote:
>
>
> > On Jan 12, 2023, at 2:27 PM, Paul E. McKenney <[email protected]> wrote:
> >
> > On Thu, Jan 12, 2023 at 12:52:22AM +0000, Joel Fernandes (Google) wrote:
> >> During boot and suspend/resume, it is desired to prevent RCU laziness from
> >> effecting performance and in some cases failures like with suspend.
> >>
> >> Track whether RCU laziness is to be ignored or not, in kernels with
> >> CONFIG_RCU_LAZY enabled. We do such tracking for expedited-RCU already, however
> >> since Android currently expedites synchronous_rcu() always, we cannot rely on
> >> that. The next patch ignores laziness hints based on this tracking.
> >>
> >> Signed-off-by: Joel Fernandes (Google) <[email protected]>
> >> ---
> >> Paul, could we take this and the next one for 6.2 -rc cycle?
> >>
> >> I also booted debian Linux and verified the flag is reset correctly after boot
> >> completes. Thanks.
> >
> > After going back and forth a bit, I took these two as-is (aside from
> > the usual commit-log wordsmithing).
> >
> > At some point, the state-change-notification from things like boot,
> > suspend/resume, sysrq-t, and panic() should probably be refactored.
> > But I do not believe that we are yet at that point, and there is not
> > much point in half-refactorings in cases such as this one.
> >
> > I added the Fixes tags, and, if testing goes well, I plan to submit
> > them into the upcoming merge window. I have no reason to believe that
> > anyone is hitting this, it is only a few weeks away anyhow, and a good
> > chunk of that would be consumed by testing and review.
> >
> > And if someone does hit it, we do have your fixes to send to them, so
> > thank you for that!
> > (These won't become visible on -rcu until I complete today's rebase,
> > in case you were wondering.)
>
> Thanks and this sounds good to me. Meanwhile I pulled into our product kernel so all is good.

Very good! Please let me know how it goes!

Thanx, Paul

> Thanks,
>
> - Joel
>
>
> >
> > Thanx, Paul
> >
> >> kernel/rcu/rcu.h | 6 ++++++
> >> kernel/rcu/tree.c | 2 ++
> >> kernel/rcu/update.c | 40 +++++++++++++++++++++++++++++++++++++++-
> >> 5 files changed, 55 insertions(+), 1 deletion(-)
> >> create mode 100644 cc_list
> >> create mode 100644 to_list
> >>
> >> diff --git a/kernel/rcu/rcu.h b/kernel/rcu/rcu.h
> >> index 5c8013f7085f..115616ac3bfa 100644
> >> --- a/kernel/rcu/rcu.h
> >> +++ b/kernel/rcu/rcu.h
> >> @@ -449,14 +449,20 @@ do { \
> >> /* Tiny RCU doesn't expedite, as its purpose in life is instead to be tiny. */
> >> static inline bool rcu_gp_is_normal(void) { return true; }
> >> static inline bool rcu_gp_is_expedited(void) { return false; }
> >> +static inline bool rcu_async_should_hurry(void) { return false; }
> >> static inline void rcu_expedite_gp(void) { }
> >> static inline void rcu_unexpedite_gp(void) { }
> >> +static inline void rcu_async_hurry(void) { }
> >> +static inline void rcu_async_relax(void) { }
> >> static inline void rcu_request_urgent_qs_task(struct task_struct *t) { }
> >> #else /* #ifdef CONFIG_TINY_RCU */
> >> bool rcu_gp_is_normal(void); /* Internal RCU use. */
> >> bool rcu_gp_is_expedited(void); /* Internal RCU use. */
> >> +bool rcu_async_should_hurry(void); /* Internal RCU use. */
> >> void rcu_expedite_gp(void);
> >> void rcu_unexpedite_gp(void);
> >> +void rcu_async_hurry(void);
> >> +void rcu_async_relax(void);
> >> void rcupdate_announce_bootup_oddness(void);
> >> #ifdef CONFIG_TASKS_RCU_GENERIC
> >> void show_rcu_tasks_gp_kthreads(void);
> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> >> index 63545d79da51..78b2e999c904 100644
> >> --- a/kernel/rcu/tree.c
> >> +++ b/kernel/rcu/tree.c
> >> @@ -4504,11 +4504,13 @@ static int rcu_pm_notify(struct notifier_block *self,
> >> switch (action) {
> >> case PM_HIBERNATION_PREPARE:
> >> case PM_SUSPEND_PREPARE:
> >> + rcu_async_hurry();
> >> rcu_expedite_gp();
> >> break;
> >> case PM_POST_HIBERNATION:
> >> case PM_POST_SUSPEND:
> >> rcu_unexpedite_gp();
> >> + rcu_async_relax();
> >> break;
> >> default:
> >> break;
> >> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> >> index 3893022f8ed8..19bf6fa3ee6a 100644
> >> --- a/kernel/rcu/update.c
> >> +++ b/kernel/rcu/update.c
> >> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> >> }
> >> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
> >>
> >> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> >> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> >> +/*
> >> + * Should call_rcu() callbacks be processed with urgency or are
> >> + * they OK being executed with arbitrary delays?
> >> + */
> >> +bool rcu_async_should_hurry(void)
> >> +{
> >> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> >> + atomic_read(&rcu_async_hurry_nesting);
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> >> +
> >> +/**
> >> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> >> + *
> >> + * After a call to this function, future calls to call_rcu()
> >> + * will be processed in a timely fashion.
> >> + */
> >> +void rcu_async_hurry(void)
> >> +{
> >> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> >> + atomic_inc(&rcu_async_hurry_nesting);
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> >>
> >> +/**
> >> + * rcu_async_relax - Make future async RCU callbacks lazy.
> >> + *
> >> + * After a call to this function, future calls to call_rcu()
> >> + * will be processed in a lazy fashion.
> >> + */
> >> +void rcu_async_relax(void)
> >> +{
> >> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> >> + atomic_dec(&rcu_async_hurry_nesting);
> >> +}
> >> +EXPORT_SYMBOL_GPL(rcu_async_relax);
> >> +
> >> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> >> /*
> >> * Should normal grace-period primitives be expedited? Intended for
> >> * use within RCU. Note that this function takes the rcu_expedited
> >> @@ -195,6 +232,7 @@ static bool rcu_boot_ended __read_mostly;
> >> void rcu_end_inkernel_boot(void)
> >> {
> >> rcu_unexpedite_gp();
> >> + rcu_async_relax();
> >> if (rcu_normal_after_boot)
> >> WRITE_ONCE(rcu_normal, 1);
> >> rcu_boot_ended = true;
> >> --
> >> 2.39.0.314.g84b9a713c41-goog

2023-01-15 21:08:42

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 2/2] rcu: Disable laziness if lazy-tracking says so

On Thu, 12 Jan 2023 00:52:23 +0000
"Joel Fernandes (Google)" <[email protected]> wrote:

>
> static void
> -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
> +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> {
> static atomic_t doublefrees;
> unsigned long flags;
> struct rcu_data *rdp;
> - bool was_alldone;
> + bool was_alldone, lazy;

I'm curious to why the the extra variable.

>
> /* Misaligned rcu_head! */
> WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1));
> @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
> kasan_record_aux_stack_noalloc(head);
> local_irq_save(flags);
> rdp = this_cpu_ptr(&rcu_data);
> + lazy = lazy_in && !rcu_async_should_hurry();

Wouldn't just having:

lazy = lazy && !rcu_async_should_hurry();

be sufficient?

-- Steve

>
> /* Add the callback to our list. */
> if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) {
> --

2023-01-15 21:38:44

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 2/2] rcu: Disable laziness if lazy-tracking says so

On Sun, Jan 15, 2023 at 3:55 PM Steven Rostedt <[email protected]> wrote:
>
> On Thu, 12 Jan 2023 00:52:23 +0000
> "Joel Fernandes (Google)" <[email protected]> wrote:
>
> >
> > static void
> > -__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
> > +__call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy_in)
> > {
> > static atomic_t doublefrees;
> > unsigned long flags;
> > struct rcu_data *rdp;
> > - bool was_alldone;
> > + bool was_alldone, lazy;
>
> I'm curious to why the the extra variable.
>
> >
> > /* Misaligned rcu_head! */
> > WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1));
> > @@ -2622,6 +2622,7 @@ __call_rcu_common(struct rcu_head *head, rcu_callback_t func, bool lazy)
> > kasan_record_aux_stack_noalloc(head);
> > local_irq_save(flags);
> > rdp = this_cpu_ptr(&rcu_data);
> > + lazy = lazy_in && !rcu_async_should_hurry();
>
> Wouldn't just having:
>
> lazy = lazy && !rcu_async_should_hurry();
>
> be sufficient?

I prefer to not overwrite function arguments, it makes debugging harder IMHO.

- Joel

>
> -- Steve
>
> >
> > /* Add the callback to our list. */
> > if (unlikely(!rcu_segcblist_is_enabled(&rdp->cblist))) {
> > --

2023-01-15 21:59:57

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Thu, 12 Jan 2023 00:52:22 +0000
"Joel Fernandes (Google)" <[email protected]> wrote:

> -- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> }
> EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
>
> -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> +/*
> + * Should call_rcu() callbacks be processed with urgency or are
> + * they OK being executed with arbitrary delays?
> + */
> +bool rcu_async_should_hurry(void)
> +{
> + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> + atomic_read(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> +
> +/**
> + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a timely fashion.
> + */
> +void rcu_async_hurry(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_inc(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_hurry);
>

Where do you plan on calling these externally, as they are being
marked exported?

If you allow random drivers to enable this, I can see something
enabling it and hitting an error path that causes it to never disable
it.

I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
needed externally. In fact, is this really needed outside of the RCU
subsystem?

-- Steve

> +/**
> + * rcu_async_relax - Make future async RCU callbacks lazy.
> + *
> + * After a call to this function, future calls to call_rcu()
> + * will be processed in a lazy fashion.
> + */
> +void rcu_async_relax(void)
> +{
> + if (IS_ENABLED(CONFIG_RCU_LAZY))
> + atomic_dec(&rcu_async_hurry_nesting);
> +}
> +EXPORT_SYMBOL_GPL(rcu_async_relax);
> +
> +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);

2023-01-15 22:07:36

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Sun, Jan 15, 2023 at 4:25 PM Steven Rostedt <[email protected]> wrote:
>
> On Thu, 12 Jan 2023 00:52:22 +0000
> "Joel Fernandes (Google)" <[email protected]> wrote:
>
> > -- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> > }
> > EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
> >
> > -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> > +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> > +/*
> > + * Should call_rcu() callbacks be processed with urgency or are
> > + * they OK being executed with arbitrary delays?
> > + */
> > +bool rcu_async_should_hurry(void)
> > +{
> > + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> > + atomic_read(&rcu_async_hurry_nesting);
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> > +
> > +/**
> > + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> > + *
> > + * After a call to this function, future calls to call_rcu()
> > + * will be processed in a timely fashion.
> > + */
> > +void rcu_async_hurry(void)
> > +{
> > + if (IS_ENABLED(CONFIG_RCU_LAZY))
> > + atomic_inc(&rcu_async_hurry_nesting);
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> >
>
> Where do you plan on calling these externally, as they are being
> marked exported?
>
> If you allow random drivers to enable this, I can see something
> enabling it and hitting an error path that causes it to never disable
> it.

You mean, just like rcu_expedite_gp() ?

> I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
> needed externally.

At the moment it is not called externally but in the future, it could
be from rcutorture. If you see rcu_expedite_gp(), that is exported
too. I was just modeling it around that API.

thanks,

- Joel

>
> -- Steve
>
>
> > +/**
> > + * rcu_async_relax - Make future async RCU callbacks lazy.
> > + *
> > + * After a call to this function, future calls to call_rcu()
> > + * will be processed in a lazy fashion.
> > + */
> > +void rcu_async_relax(void)
> > +{
> > + if (IS_ENABLED(CONFIG_RCU_LAZY))
> > + atomic_dec(&rcu_async_hurry_nesting);
> > +}
> > +EXPORT_SYMBOL_GPL(rcu_async_relax);
> > +
> > +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);

2023-01-16 05:34:15

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Sun, Jan 15, 2023 at 04:34:58PM -0500, Joel Fernandes wrote:
> On Sun, Jan 15, 2023 at 4:25 PM Steven Rostedt <[email protected]> wrote:
> >
> > On Thu, 12 Jan 2023 00:52:22 +0000
> > "Joel Fernandes (Google)" <[email protected]> wrote:
> >
> > > -- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -144,8 +144,45 @@ bool rcu_gp_is_normal(void)
> > > }
> > > EXPORT_SYMBOL_GPL(rcu_gp_is_normal);
> > >
> > > -static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);
> > > +static atomic_t rcu_async_hurry_nesting = ATOMIC_INIT(1);
> > > +/*
> > > + * Should call_rcu() callbacks be processed with urgency or are
> > > + * they OK being executed with arbitrary delays?
> > > + */
> > > +bool rcu_async_should_hurry(void)
> > > +{
> > > + return !IS_ENABLED(CONFIG_RCU_LAZY) ||
> > > + atomic_read(&rcu_async_hurry_nesting);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcu_async_should_hurry);
> > > +
> > > +/**
> > > + * rcu_async_hurry - Make future async RCU callbacks not lazy.
> > > + *
> > > + * After a call to this function, future calls to call_rcu()
> > > + * will be processed in a timely fashion.
> > > + */
> > > +void rcu_async_hurry(void)
> > > +{
> > > + if (IS_ENABLED(CONFIG_RCU_LAZY))
> > > + atomic_inc(&rcu_async_hurry_nesting);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> > >
> >
> > Where do you plan on calling these externally, as they are being
> > marked exported?
> >
> > If you allow random drivers to enable this, I can see something
> > enabling it and hitting an error path that causes it to never disable
> > it.
>
> You mean, just like rcu_expedite_gp() ?
>
> > I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
> > needed externally.
>
> At the moment it is not called externally but in the future, it could
> be from rcutorture. If you see rcu_expedite_gp(), that is exported
> too. I was just modeling it around that API.

It really should be invoked from rcutorture for testing purposes.
In current -rcu, TREE01 enables LAZY_RCU, so we are finally getting
coverage (aside from my manual enablement on part of the test grid that
I use).

So we need that export.

On the other hand, wasn't there some talk recently of targeted exports?
If that comes to pass, this could be exported to only rcutorture and
rcuscale.

Thanx, Paul

> thanks,
>
> - Joel
>
> >
> > -- Steve
> >
> >
> > > +/**
> > > + * rcu_async_relax - Make future async RCU callbacks lazy.
> > > + *
> > > + * After a call to this function, future calls to call_rcu()
> > > + * will be processed in a lazy fashion.
> > > + */
> > > +void rcu_async_relax(void)
> > > +{
> > > + if (IS_ENABLED(CONFIG_RCU_LAZY))
> > > + atomic_dec(&rcu_async_hurry_nesting);
> > > +}
> > > +EXPORT_SYMBOL_GPL(rcu_async_relax);
> > > +
> > > +static atomic_t rcu_expedited_nesting = ATOMIC_INIT(1);

2023-01-17 21:25:51

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Sun, 15 Jan 2023 16:34:58 -0500
Joel Fernandes <[email protected]> wrote:

> > > +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> > >
> >
> > Where do you plan on calling these externally, as they are being
> > marked exported?
> >
> > If you allow random drivers to enable this, I can see something
> > enabling it and hitting an error path that causes it to never disable
> > it.
>
> You mean, just like rcu_expedite_gp() ?
>
> > I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
> > needed externally.
>
> At the moment it is not called externally but in the future, it could
> be from rcutorture. If you see rcu_expedite_gp(), that is exported
> too. I was just modeling it around that API.

The reason for the export should have been mentioned in the change log if
the patch is not obvious to why it is being exported.

Thanks,

-- Steve

2023-01-17 21:26:34

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Tue, Jan 17, 2023 at 02:32:24PM -0500, Steven Rostedt wrote:
> On Sun, 15 Jan 2023 16:34:58 -0500
> Joel Fernandes <[email protected]> wrote:
>
> > > > +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> > > >
> > >
> > > Where do you plan on calling these externally, as they are being
> > > marked exported?
> > >
> > > If you allow random drivers to enable this, I can see something
> > > enabling it and hitting an error path that causes it to never disable
> > > it.
> >
> > You mean, just like rcu_expedite_gp() ?
> >
> > > I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
> > > needed externally.
> >
> > At the moment it is not called externally but in the future, it could
> > be from rcutorture. If you see rcu_expedite_gp(), that is exported
> > too. I was just modeling it around that API.
>
> The reason for the export should have been mentioned in the change log if
> the patch is not obvious to why it is being exported.

Would something like this suffice? With attribution, of course.

Export rcu_async_should_hurry(), rcu_async_hurry(), and
rcu_async_relax() for later use by rcutorture.

Thanx, Paul

2023-01-17 22:33:43

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Tue, Jan 17, 2023 at 7:37 PM Paul E. McKenney <[email protected]> wrote:
>
> On Tue, Jan 17, 2023 at 02:32:24PM -0500, Steven Rostedt wrote:
> > On Sun, 15 Jan 2023 16:34:58 -0500
> > Joel Fernandes <[email protected]> wrote:
> >
> > > > > +EXPORT_SYMBOL_GPL(rcu_async_hurry);
> > > > >
> > > >
> > > > Where do you plan on calling these externally, as they are being
> > > > marked exported?
> > > >
> > > > If you allow random drivers to enable this, I can see something
> > > > enabling it and hitting an error path that causes it to never disable
> > > > it.
> > >
> > > You mean, just like rcu_expedite_gp() ?
> > >
> > > > I wouldn't have EXPORT_SYMBOL_GPL() unless you really know that it is
> > > > needed externally.
> > >
> > > At the moment it is not called externally but in the future, it could
> > > be from rcutorture. If you see rcu_expedite_gp(), that is exported
> > > too. I was just modeling it around that API.
> >
> > The reason for the export should have been mentioned in the change log if
> > the patch is not obvious to why it is being exported.
>
> Would something like this suffice? With attribution, of course.
>
> Export rcu_async_should_hurry(), rcu_async_hurry(), and
> rcu_async_relax() for later use by rcutorture.

Looks good to me Paul, and thanks for the suggestion Steven!

- Joel

2023-01-17 23:09:34

[permalink] [raw]

Subject: Re: [PATCH v2 rcu/dev 1/2] rcu: Track laziness during boot and suspend

On Tue, 17 Jan 2023 11:37:34 -0800
"Paul E. McKenney" <[email protected]> wrote:

> > The reason for the export should have been mentioned in the change log if
> > the patch is not obvious to why it is being exported.
>
> Would something like this suffice? With attribution, of course.
>
> Export rcu_async_should_hurry(), rcu_async_hurry(), and
> rcu_async_relax() for later use by rcutorture.

Yes thanks. That way, at least a git blame will give some rationale for the
export.

-- Steve

2023-01-18 04:13:06