Message-ID: <CAAhV-H7Wy5T9iPoJBKrH9pU7oLtC0wfc+dr1-Wqu2y38BsFfDw@mail.gmail.com>
Date:   Sun, 13 Aug 2023 21:22:33 +0800
From:   Huacai Chen <chenhuacai@...nel.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Alan Huang <mmpgouride@...il.com>,
        Huacai Chen <chenhuacai@...ngson.cn>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Neeraj Upadhyay <quic_neeraju@...cinc.com>,
        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>,
        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

Hi, Joel,

On Sun, Aug 13, 2023 at 10:07 AM Joel Fernandes <joel@...lfernandes.org> wrote:
>
>
>
> > On Aug 11, 2023, at 12:33 AM, Huacai Chen <chenhuacai@...nel.org> wrote:
> >
> > Hi, Alan,
> >
> >> On Thu, Aug 10, 2023 at 11:47 PM Alan Huang <mmpgouride@...il.com> wrote:
> >>
> >>
> >>> 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
> > OK, thanks. But since I have sent three versions in one day (very
> > sorry for that), the next version will wait for some more comments.
>
> For the RCU part, looks fine to me.
>
> Another option for the jiffies update part is to just expose a wrapper
> around the main update function and use that wrapper.
> That way you do not need to move a lot of code and that keeps git blame intact.
Thank you for your review. But since tick_do_update_jiffies64() is
static and tick-sched.c itself is conditionally compiled. It seems
impossible to make a wrapper without touching the original function.

Huacai
>
> Thanks,
>
>  - Joel
>
>
> >
> > Huacai
> >>
> >>
> >>> + 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