Message-Id: <8C2A7D23-12EC-404D-9207-CC38EEC27DF7@gmail.com>
Date:   Thu, 10 Aug 2023 23:47:30 +0800
From:   Alan Huang <mmpgouride@...il.com>
To:     Huacai Chen <chenhuacai@...ngson.cn>
Cc:     "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Josh Triplett <josh@...htriplett.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        John Stultz <jstultz@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Zqiang <qiang.zhang1211@...il.com>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        chenhuacai@...nel.org, rcu@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 1/2] tick: Rename tick_do_update_jiffies64() and allow
 external usage


> 2023年8月10日 20:24，Huacai Chen <chenhuacai@...ngson.cn> 写道：
> 
> Rename tick_do_update_jiffies64() to do_update_jiffies_64() and move it
> to jiffies.c. This keeps the same naming style in jiffies.c and allow it
> be used by external components. This patch is a preparation for the next
> one which attempts to avoid necessary rcu stall warnings.
> 
> Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
> ---
> V2: Fix build.
> V3: Fix build again.
> 
> include/linux/jiffies.h   |   2 +
> kernel/time/jiffies.c     | 113 ++++++++++++++++++++++++++++++++++++-
> kernel/time/tick-sched.c  | 115 ++------------------------------------
> kernel/time/timekeeping.h |   1 +
> 4 files changed, 118 insertions(+), 113 deletions(-)
> 
> diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
> index 5e13f801c902..48866314c68b 100644
> --- a/include/linux/jiffies.h
> +++ b/include/linux/jiffies.h
> @@ -88,6 +88,8 @@ static inline u64 get_jiffies_64(void)
> }
> #endif
> 
> +void do_update_jiffies_64(s64 now); /* typedef s64 ktime_t */
> +
> /*
>  * These inlines deal with timer wrapping correctly. You are 
>  * strongly encouraged to use them
> diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
> index bc4db9e5ab70..507a1e7e619e 100644
> --- a/kernel/time/jiffies.c
> +++ b/kernel/time/jiffies.c
> @@ -5,14 +5,14 @@
>  * Copyright (C) 2004, 2005 IBM, John Stultz (johnstul@...ibm.com)
>  */
> #include <linux/clocksource.h>
> +#include <linux/init.h>
> #include <linux/jiffies.h>
> #include <linux/module.h>
> -#include <linux/init.h>
> +#include <linux/sched/loadavg.h>
> 
> #include "timekeeping.h"
> #include "tick-internal.h"
> 
> -
> static u64 jiffies_read(struct clocksource *cs)
> {
> return (u64) jiffies;
> @@ -61,6 +61,115 @@ EXPORT_SYMBOL(get_jiffies_64);
> 
> EXPORT_SYMBOL(jiffies);
> 
> +/*
> + * The time, when the last jiffy update happened. Write access must hold
> + * jiffies_lock and jiffies_seq. Because tick_nohz_next_event() needs to
> + * get a consistent view of jiffies and last_jiffies_update.
> + */
> +ktime_t last_jiffies_update;
> +
> +/*
> + * Must be called with interrupts disabled !
> + */
> +void do_update_jiffies_64(ktime_t now)
> +{
> +#if defined(CONFIG_NO_HZ_COMMON) || defined(CONFIG_HIGH_RES_TIMERS)

Would it be better define the function like this?

#if defined(CONFIG_NO_HZ_COMMON) || defined(CONFIG_HIGH_RES_TIMERS)

void do_update_jiffies_64(ktime_t now)

#else

void do_update_jiffies_64(ktime_t now)

#endif


> + unsigned long ticks = 1;
> + ktime_t delta, nextp;
> +
> + /*
> + * 64bit can do a quick check without holding jiffies lock and
> + * without looking at the sequence count. The smp_load_acquire()
> + * pairs with the update done later in this function.
> + *
> + * 32bit cannot do that because the store of tick_next_period
> + * consists of two 32bit stores and the first store could move it
> + * to a random point in the future.
> + */
> + if (IS_ENABLED(CONFIG_64BIT)) {
> + if (ktime_before(now, smp_load_acquire(&tick_next_period)))
> + return;
> + } else {
> + unsigned int seq;
> +
> + /*
> + * Avoid contention on jiffies_lock and protect the quick
> + * check with the sequence count.
> + */
> + do {
> + seq = read_seqcount_begin(&jiffies_seq);
> + nextp = tick_next_period;
> + } while (read_seqcount_retry(&jiffies_seq, seq));
> +
> + if (ktime_before(now, nextp))
> + return;
> + }
> +
> + /* Quick check failed, i.e. update is required. */
> + raw_spin_lock(&jiffies_lock);
> + /*
> + * Reevaluate with the lock held. Another CPU might have done the
> + * update already.
> + */
> + if (ktime_before(now, tick_next_period)) {
> + raw_spin_unlock(&jiffies_lock);
> + return;
> + }
> +
> + write_seqcount_begin(&jiffies_seq);
> +
> + delta = ktime_sub(now, tick_next_period);
> + if (unlikely(delta >= TICK_NSEC)) {
> + /* Slow path for long idle sleep times */
> + s64 incr = TICK_NSEC;
> +
> + ticks += ktime_divns(delta, incr);
> +
> + last_jiffies_update = ktime_add_ns(last_jiffies_update,
> +   incr * ticks);
> + } else {
> + last_jiffies_update = ktime_add_ns(last_jiffies_update,
> +   TICK_NSEC);
> + }
> +
> + /* Advance jiffies to complete the jiffies_seq protected job */
> + jiffies_64 += ticks;
> +
> + /*
> + * Keep the tick_next_period variable up to date.
> + */
> + nextp = ktime_add_ns(last_jiffies_update, TICK_NSEC);
> +
> + if (IS_ENABLED(CONFIG_64BIT)) {
> + /*
> + * Pairs with smp_load_acquire() in the lockless quick
> + * check above and ensures that the update to jiffies_64 is
> + * not reordered vs. the store to tick_next_period, neither
> + * by the compiler nor by the CPU.
> + */
> + smp_store_release(&tick_next_period, nextp);
> + } else {
> + /*
> + * A plain store is good enough on 32bit as the quick check
> + * above is protected by the sequence count.
> + */
> + tick_next_period = nextp;
> + }
> +
> + /*
> + * Release the sequence count. calc_global_load() below is not
> + * protected by it, but jiffies_lock needs to be held to prevent
> + * concurrent invocations.
> + */
> + write_seqcount_end(&jiffies_seq);
> +
> + calc_global_load();
> +
> + raw_spin_unlock(&jiffies_lock);
> + update_wall_time();
> +#endif
> +}
> +
> static int __init init_jiffies_clocksource(void)
> {
> return __clocksource_register(&clocksource_jiffies);
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 4df14db4da49..c993c7dfe79d 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -44,113 +44,6 @@ struct tick_sched *tick_get_tick_sched(int cpu)
> }
> 
> #if defined(CONFIG_NO_HZ_COMMON) || defined(CONFIG_HIGH_RES_TIMERS)
> -/*
> - * The time, when the last jiffy update happened. Write access must hold
> - * jiffies_lock and jiffies_seq. tick_nohz_next_event() needs to get a
> - * consistent view of jiffies and last_jiffies_update.
> - */
> -static ktime_t last_jiffies_update;
> -
> -/*
> - * Must be called with interrupts disabled !
> - */
> -static void tick_do_update_jiffies64(ktime_t now)
> -{
> - unsigned long ticks = 1;
> - ktime_t delta, nextp;
> -
> - /*
> - * 64bit can do a quick check without holding jiffies lock and
> - * without looking at the sequence count. The smp_load_acquire()
> - * pairs with the update done later in this function.
> - *
> - * 32bit cannot do that because the store of tick_next_period
> - * consists of two 32bit stores and the first store could move it
> - * to a random point in the future.
> - */
> - if (IS_ENABLED(CONFIG_64BIT)) {
> - if (ktime_before(now, smp_load_acquire(&tick_next_period)))
> - return;
> - } else {
> - unsigned int seq;
> -
> - /*
> - * Avoid contention on jiffies_lock and protect the quick
> - * check with the sequence count.
> - */
> - do {
> - seq = read_seqcount_begin(&jiffies_seq);
> - nextp = tick_next_period;
> - } while (read_seqcount_retry(&jiffies_seq, seq));
> -
> - if (ktime_before(now, nextp))
> - return;
> - }
> -
> - /* Quick check failed, i.e. update is required. */
> - raw_spin_lock(&jiffies_lock);
> - /*
> - * Reevaluate with the lock held. Another CPU might have done the
> - * update already.
> - */
> - if (ktime_before(now, tick_next_period)) {
> - raw_spin_unlock(&jiffies_lock);
> - return;
> - }
> -
> - write_seqcount_begin(&jiffies_seq);
> -
> - delta = ktime_sub(now, tick_next_period);
> - if (unlikely(delta >= TICK_NSEC)) {
> - /* Slow path for long idle sleep times */
> - s64 incr = TICK_NSEC;
> -
> - ticks += ktime_divns(delta, incr);
> -
> - last_jiffies_update = ktime_add_ns(last_jiffies_update,
> -   incr * ticks);
> - } else {
> - last_jiffies_update = ktime_add_ns(last_jiffies_update,
> -   TICK_NSEC);
> - }
> -
> - /* Advance jiffies to complete the jiffies_seq protected job */
> - jiffies_64 += ticks;
> -
> - /*
> - * Keep the tick_next_period variable up to date.
> - */
> - nextp = ktime_add_ns(last_jiffies_update, TICK_NSEC);
> -
> - if (IS_ENABLED(CONFIG_64BIT)) {
> - /*
> - * Pairs with smp_load_acquire() in the lockless quick
> - * check above and ensures that the update to jiffies_64 is
> - * not reordered vs. the store to tick_next_period, neither
> - * by the compiler nor by the CPU.
> - */
> - smp_store_release(&tick_next_period, nextp);
> - } else {
> - /*
> - * A plain store is good enough on 32bit as the quick check
> - * above is protected by the sequence count.
> - */
> - tick_next_period = nextp;
> - }
> -
> - /*
> - * Release the sequence count. calc_global_load() below is not
> - * protected by it, but jiffies_lock needs to be held to prevent
> - * concurrent invocations.
> - */
> - write_seqcount_end(&jiffies_seq);
> -
> - calc_global_load();
> -
> - raw_spin_unlock(&jiffies_lock);
> - update_wall_time();
> -}
> -
> /*
>  * Initialize and return retrieve the jiffies update.
>  */
> @@ -207,7 +100,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
> 
> /* Check, if the jiffies need an update */
> if (tick_do_timer_cpu == cpu)
> - tick_do_update_jiffies64(now);
> + do_update_jiffies_64(now);
> 
> /*
> * If jiffies update stalled for too long (timekeeper in stop_machine()
> @@ -218,7 +111,7 @@ static void tick_sched_do_timer(struct tick_sched *ts, ktime_t now)
> ts->last_tick_jiffies = READ_ONCE(jiffies);
> } else {
> if (++ts->stalled_jiffies == MAX_STALLED_JIFFIES) {
> - tick_do_update_jiffies64(now);
> + do_update_jiffies_64(now);
> ts->stalled_jiffies = 0;
> ts->last_tick_jiffies = READ_ONCE(jiffies);
> }
> @@ -652,7 +545,7 @@ static void tick_nohz_update_jiffies(ktime_t now)
> __this_cpu_write(tick_cpu_sched.idle_waketime, now);
> 
> local_irq_save(flags);
> - tick_do_update_jiffies64(now);
> + do_update_jiffies_64(now);
> local_irq_restore(flags);
> 
> touch_softlockup_watchdog_sched();
> @@ -975,7 +868,7 @@ static void tick_nohz_stop_sched_tick(struct tick_sched *ts, int cpu)
> static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
> {
> /* Update jiffies first */
> - tick_do_update_jiffies64(now);
> + do_update_jiffies_64(now);
> 
> /*
> * Clear the timer idle flag, so we avoid IPIs on remote queueing and
> diff --git a/kernel/time/timekeeping.h b/kernel/time/timekeeping.h
> index 543beba096c7..21670f6c7421 100644
> --- a/kernel/time/timekeeping.h
> +++ b/kernel/time/timekeeping.h
> @@ -28,6 +28,7 @@ extern void update_wall_time(void);
> 
> extern raw_spinlock_t jiffies_lock;
> extern seqcount_raw_spinlock_t jiffies_seq;
> +extern ktime_t last_jiffies_update;
> 
> #define CS_NAME_LEN 32
> 
> -- 
> 2.39.3
> 

Powered by blists - more mailing lists