On Tue, Jul 30, 2019 at 06:43:10PM +0200, Frederic Weisbecker wrote:
> On Mon, Jul 29, 2019 at 03:32:38PM -0700, Paul E. McKenney wrote:
> > On Wed, Jul 24, 2019 at 06:12:43PM -0700, Paul E. McKenney wrote:
> >
> > The patch below (which includes your patch) does help considerably.
> > However, it does have some shortcomings:
> >
> > 1. Adds an atomic operation (albeit a cache-local one) to
> > the scheduler fastpath. One approach would be to have
> > a way of testing this bit and clearing it only if set.
> >
> > Another approach would be to instead clear it on the
> > transition to nohz_full userspace or to idle.
>
> Well, the latter would be costly as it is going to restart the tick on every
> user -> kernel transitions.
You lost me on this one. I would be turning off RCU's request to
maintain the tick on transition to nohz_full userspace or to idle.
Why would the tick get turned on by a later user->kernel transition?
> > 2. There are a lot of other places in the kernel that are in
> > need of this bit being set. I am therefore considering making
> > multi_cpu_stop() or its callers set this bit on all CPUs upon
> > entry and clear it upon exit. While in this state, it is
> > likely necessary to disable clearing this bit. Or it would
> > be necessary to make multi_cpu_stop() repeat clearing the bit
> > every so often.
> >
> > As it stands, I have CPU hotplug removal operations taking
> > more than 400 seconds.
> >
> > 3. It was tempting to ask for this bit to be tracked on a per-task
> > basis, but from what I can see that adds at least as much
> > complexity as it removes.
>
> Yeah I forgot to answer, you can use tick_dep_set_task() for that.
Ah, good, that would remove my need to clear things on the scheduler
fastpaths. My guess is that I use both the per-CPU and the per-task
variant in different places, but testing will tell! ;-)
Thank you!
Thanx, Paul
> > Thoughts?
> >
> > Thanx, Paul
> >
> > PS. Outage on @linux.ibm.com, hence the CC of my gmail address.
> >
> > ------------------------------------------------------------------------
> >
> > diff --git a/include/linux/tick.h b/include/linux/tick.h
> > index 196a0a7bfc4f..0dea6fb33a11 100644
> > --- a/include/linux/tick.h
> > +++ b/include/linux/tick.h
> > @@ -108,7 +108,8 @@ enum tick_dep_bits {
> > TICK_DEP_BIT_POSIX_TIMER = 0,
> > TICK_DEP_BIT_PERF_EVENTS = 1,
> > TICK_DEP_BIT_SCHED = 2,
> > - TICK_DEP_BIT_CLOCK_UNSTABLE = 3
> > + TICK_DEP_BIT_CLOCK_UNSTABLE = 3,
> > + TICK_DEP_BIT_RCU = 4
> > };
> >
> > #define TICK_DEP_MASK_NONE 0
> > @@ -116,6 +117,7 @@ enum tick_dep_bits {
> > #define TICK_DEP_MASK_PERF_EVENTS (1 << TICK_DEP_BIT_PERF_EVENTS)
> > #define TICK_DEP_MASK_SCHED (1 << TICK_DEP_BIT_SCHED)
> > #define TICK_DEP_MASK_CLOCK_UNSTABLE (1 << TICK_DEP_BIT_CLOCK_UNSTABLE)
> > +#define TICK_DEP_MASK_RCU (1 << TICK_DEP_BIT_RCU)
> >
> > #ifdef CONFIG_NO_HZ_COMMON
> > extern bool tick_nohz_enabled;
> > @@ -258,6 +260,9 @@ static inline bool tick_nohz_full_enabled(void) { return false; }
> > static inline bool tick_nohz_full_cpu(int cpu) { return false; }
> > static inline void tick_nohz_full_add_cpus_to(struct cpumask *mask) { }
> >
> > +static inline void tick_nohz_dep_set_cpu(int cpu, enum tick_dep_bits bit) { }
> > +static inline void tick_nohz_dep_clear_cpu(int cpu, enum tick_dep_bits bit) { }
>
> And I gave you the wrong APIs. Please consider_using tick_dep_set_cpu()
> and tick_dep_clear_cpu() that first check if the CPU uses nohz_full.
>
> Those should have the !CONFIG_NO_HZ_FULL stub implemented as well.
>
> Thanks.
>
