linux-kernel - Re: [PATCH 05/23] sched,drbd: Convert to sched_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200423085735.GU20713@hirez.programming.kicks-ass.net>
Date:   Thu, 23 Apr 2020 10:57:35 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     mingo@...nel.org, linux-kernel@...r.kernel.org
Cc:     tglx@...utronix.de, rostedt@...dmis.org, qais.yousef@....com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, bsegall@...gle.com, mgorman@...e.de,
        axboe@...nel.dk
Subject: Re: [PATCH 05/23] sched,drbd: Convert to sched_set_fifo*()

On Wed, Apr 22, 2020 at 01:27:24PM +0200, Peter Zijlstra wrote:
> Because SCHED_FIFO is a broken scheduler model (see previous patches)
> take away the priority field, the kernel can't possibly make an
> informed decision.
> 
> In this case, use fifo_low, because it only cares about being above
> SCHED_NORMAL. Effectively changes prio from 2 to 1.
> 
> Cc: axboe@...nel.dk
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Reviewed-by: Ingo Molnar <mingo@...nel.org>
> ---
>  drivers/block/drbd/drbd_receiver.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- a/drivers/block/drbd/drbd_receiver.c
> +++ b/drivers/block/drbd/drbd_receiver.c
> @@ -6020,9 +6020,8 @@ int drbd_ack_receiver(struct drbd_thread
>  	unsigned int header_size = drbd_header_size(connection);
>  	int expect   = header_size;
>  	bool ping_timeout_active = false;
> -	struct sched_param param = { .sched_priority = 2 };
>  
> -	rv = sched_setscheduler(current, SCHED_RR, &param);
> +	rv = sched_set_fifo_low(current);

As noted by Dietmar, I forgot to mention loosing RR in the changelog,
bad me.

In this case I'm not actually 100% sure, but there was no comment with
it that justified it being RR, and RR-SMP is not optimal (in fact it's
rather buggered).

In general RR is even more 'interesting' to get right thatn FIFO and
therefore I figured it probably didn't want to be RR, but given there
can be multiple of such threads, it might have been an attempt at
providing some sort of fairness between the multiple threads.

At the same time, if you're running the threads so hard that RR makes a
difference, it's unlikely there is any actual NORMAL time left and
things will be unhappy anyway.

Therefore, and me being lazy, make it FIFO.