Re: Potential problem with 31e77c93e432dec7 ("sched/fair: Update blocked load when newly idle")

[Peter Zijlstra]

On Thu, Apr 26, 2018 at 12:31:33PM +0200, Vincent Guittot wrote:
> From: Vincent Guittot 
> Date: Thu, 26 Apr 2018 12:19:32 +0200
> Subject: [PATCH] sched/fair: fix the update of blocked load when newly idle
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> With commit 31e77c93e432 ("sched/fair: Update blocked load when newly idle"),
> we release the rq->lock when updating blocked load of idle CPUs. This open
> a time window during which another CPU can add a task to this CPU's cfs_rq.
> The check for newly added task of idle_balance() is not in the common path.
> Move the out label to include this check.

Ah quite so indeed. This could result in us running idle even though
there is a runnable task around -- which is bad.

> Fixes: 31e77c93e432 ("sched/fair: Update blocked load when newly idle")
> Reported-by: Heiner Kallweit 
> Reported-by: Niklas Söderlund 
> Signed-off-by: Vincent Guittot 
> ---
>  kernel/sched/fair.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0951d1c..15a9f5e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -9847,6 +9847,7 @@ static int idle_balance(struct rq *this_rq, struct 
> rq_flags *rf)
>   if (curr_cost > this_rq->max_idle_balance_cost)
>   this_rq->max_idle_balance_cost = curr_cost;
>  
> +out:
>   /*
>* While browsing the domains, we released the rq lock, a task could
>* have been enqueued in the meantime. Since we're not going idle,
> @@ -9855,7 +9856,6 @@ static int idle_balance(struct rq *this_rq, struct 
> rq_flags *rf)
>   if (this_rq->cfs.h_nr_running && !pulled_task)
>   pulled_task = 1;
>  
> -out:
>   /* Move the next balance forward */
>   if (time_after(this_rq->next_balance, next_balance))
>   this_rq->next_balance = next_balance;

Re: [PATCH v3 0/2] Ajust lockdep static allocations for sparc

[Peter Zijlstra]

On Tue, Nov 29, 2016 at 02:39:20PM +0100, Geert Uytterhoeven wrote:

> > Not understanding, why would a user ever need it? The platform knows if
> > its has funny boot image size limits, no?
> 
> The boot loader does not come with the kernel, so the platform cannot
> know for sure.

Why would anybody use a bootloader that imposes weird restrictions on
their platform? That is, isn't that simply a broken bootloader?

Re: [PATCH v3 0/2] Ajust lockdep static allocations for sparc

[Peter Zijlstra]

On Tue, Nov 29, 2016 at 02:26:47PM +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 29, 2016 at 1:29 PM, Peter Zijlstra <pet...@infradead.org> wrote:
> > On Tue, Nov 29, 2016 at 12:52:04PM +0100, Geert Uytterhoeven wrote:

> >> Not because of platforms with not limited memory, but because of platforms
> >> with boot loaders that have silly kernel size limitations, and start
> >> scribbling over the DTB or even theirselves when copying a large kernel 
> >> image.
> >
> > Right, that's the weird platforms clause above, and those can select the
> > option.
> 
> Their Kconfig files can select it. The users can't.
> What about making it visible depending on EXPERT?

Not understanding, why would a user ever need it? The platform knows if
its has funny boot image size limits, no?

Re: [PATCH v3 0/2] Ajust lockdep static allocations for sparc

[Peter Zijlstra]

On Tue, Nov 29, 2016 at 01:29:07PM +0100, Peter Zijlstra wrote:
> > BTW, is there any particular reason these huge arrays are in BSS, and not
> > allocated dynamically? That would solve my problems as well...
> 
> Is there a memory allocator available before _any_ locks are used, and
> that itself also doesn't use locks?

And if that is, the platform could use that to allocate .bss, no?

Re: [PATCH v3 0/2] Ajust lockdep static allocations for sparc

[Peter Zijlstra]

On Tue, Nov 29, 2016 at 12:52:04PM +0100, Geert Uytterhoeven wrote:
> > Nah, users don't need more senseless options. This is really only useful
> > for dinky platforms or platforms with limited static image size (like
> > sparc64).
> >
> > If you make this user selectable, someone will do, and then an endless
> > stream of table not big enough warnings will be posted.
> >
> > Also, its only 4MB (IIRC), so who cares.
> 
> I care :-)
> 
> Not because of platforms with not limited memory, but because of platforms
> with boot loaders that have silly kernel size limitations, and start
> scribbling over the DTB or even theirselves when copying a large kernel image.

Right, that's the weird platforms clause above, and those can select the
option.

> BTW, is there any particular reason these huge arrays are in BSS, and not
> allocated dynamically? That would solve my problems as well...

Is there a memory allocator available before _any_ locks are used, and
that itself also doesn't use locks?

Re: [PATCH v3 0/2] Ajust lockdep static allocations for sparc

[Peter Zijlstra]

On Tue, Nov 29, 2016 at 12:14:48PM +0100, Geert Uytterhoeven wrote:
> CC linux-renesas-soc
> 
> On Tue, Sep 27, 2016 at 9:33 PM, Babu Moger  wrote:
> > These patches limit the static allocations for lockdep data structures
> > used for debugging locking correctness. For sparc, all the kernel's code,
> > data, and bss, must have locked translations in the TLB so that we don't
> > get TLB misses on kernel code and data. Current sparc chips have 8 TLB
> > entries available that may be locked down, and with a 4mb page size,
> > this gives a maximum of 32MB. With PROVE_LOCKING we could go over this
> > limit and cause system boot-up problems. These patches limit the static
> > allocations so that everything fits in current required size limit.
> >
> > patch 1 : Adds new config parameter CONFIG_PROVE_LOCKING_SMALL
> > Patch 2 : Adjusts the sizes based on the new config parameter
> 
> Cool, this is also useful on other platforms!
> E.g. on r8a7791/koelsch, I cannot boot an shmobile_defconfig plus
> CONFIG_PROVE_LOCKING kernel.
> Forcing CONFIG_PROVE_LOCKING_SMALL=y in Kconfig fixes that.
> 
> Should it become a user-selectable symbol?

Nah, users don't need more senseless options. This is really only useful
for dinky platforms or platforms with limited static image size (like
sparc64).

If you make this user selectable, someone will do, and then an endless
stream of table not big enough warnings will be posted.

Also, its only 4MB (IIRC), so who cares.