All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: peterz@infradead.org
Cc: linux-kernel@vger.kernel.org, io-uring@vger.kernel.org,
	Ingo Molnar <mingo@redhat.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Jens Axboe <axboe@kernel.dk>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [RFC PATCH] sched: Invoke io_wq_worker_sleeping() with enabled preemption
Date: Wed, 19 Aug 2020 15:33:20 +0200	[thread overview]
Message-ID: <20200819133320.bxwb3ikjswyhmsyg@linutronix.de> (raw)
In-Reply-To: <20200819131507.GC2674@hirez.programming.kicks-ass.net>

On 2020-08-19 15:15:07 [+0200], peterz@infradead.org wrote:

> > -	if (tsk->flags & (PF_WQ_WORKER | PF_IO_WORKER)) {
> > +	if (tsk->flags & PF_WQ_WORKER) {
> >  		preempt_disable();
> > -		if (tsk->flags & PF_WQ_WORKER)
> > -			wq_worker_sleeping(tsk);
> > -		else
> > -			io_wq_worker_sleeping(tsk);
> > +		wq_worker_sleeping(tsk);
> >  		preempt_enable_no_resched();
> >  	}
> >  
> >  	if (tsk_is_pi_blocked(tsk))
> >  		return;
> >  
> > +	if (tsk->flags & PF_IO_WORKER)
> > +		io_wq_worker_sleeping(tsk);
> > +
> 
> Urgh, so this adds a branch in what is normally considered a fairly hot
> path.
> 
> I'm thinking that the raw_spinlock_t option would permit leaving that
> single:
> 
> 	if (tsk->flags & (PF_WQ_WORKER | PF_IO_WORKER))
> 
> branch intact?

The compiler generates code to test for both flags at once. If none of
both possible flags are set then there is one branch (get out and bring
me to tst_is_pi…).
And yes, with raw_spinlock_t we could keep that one branch.

If you want to optimize further, we could move PF_IO_WORKER to an lower
bit. x86 can test for both via
(gcc-10)
|         testl   $536870944, 44(%rbp)    #, _11->flags
|         jne     .L1635  #,

(clang-9)
|         testl   $536870944, 44(%rbx)    # imm = 0x20000020
|         je      .LBB112_6


but ARM can't and does
|          ldr     r1, [r5, #16]   @ tsk_3->flags, tsk_3->flags
|         mov     r2, #32 @ tmp157,
|         movt    r2, 8192        @ tmp157,
|         tst     r2, r1  @ tmp157, tsk_3->flags
|         beq     .L998           @,

same ARM64
|         ldr     w0, [x20, 60]   //, _11->flags
|         and     w0, w0, 1073741792      // tmp117, _11->flags,
|         and     w0, w0, -536870849      // tmp117, tmp117,
|         cbnz    w0, .L453       // tmp117,

using 0x10 for PF_IO_WORKER instead will turn this into:
|         ldr     w0, [x20, 60]   //, _11->flags
|         tst     w0, 48  // _11->flags,
|         bne     .L453           //,

ARM:
|         ldr     r2, [r5, #16]   @ tsk_3->flags, tsk_3->flags
|         tst     r2, #48 @ tsk_3->flags,
|         beq     .L998           @,

Sebastian

  parent reply	other threads:[~2020-08-19 13:33 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-19 12:37 [RFC PATCH] sched: Invoke io_wq_worker_sleeping() with enabled preemption Sebastian Andrzej Siewior
2020-08-19 13:15 ` peterz
2020-08-19 13:18   ` Jens Axboe
2020-08-19 19:44     ` [PATCH] io_wq: Make io_wqe::lock a raw_spinlock_t Sebastian Andrzej Siewior
2020-09-01  8:41       ` [PATCH v2] " Sebastian Andrzej Siewior
2020-09-01 14:17         ` Jens Axboe
2020-08-19 13:33   ` Sebastian Andrzej Siewior [this message]
2020-08-19 14:21     ` [RFC PATCH] sched: Invoke io_wq_worker_sleeping() with enabled preemption peterz
2020-08-19 19:55       ` [PATCH 1/2] sched: Bring the PF_IO_WORKER and PF_WQ_WORKER bits closer together Sebastian Andrzej Siewior
2020-08-19 20:00         ` [PATCH 2/2] sched: Cache task_struct::flags in sched_submit_work() Sebastian Andrzej Siewior
2020-08-19 20:11           ` Peter Zijlstra
2020-08-27  7:54           ` [tip: sched/core] " tip-bot2 for Sebastian Andrzej Siewior
2020-08-27  7:54         ` [tip: sched/core] sched: Bring the PF_IO_WORKER and PF_WQ_WORKER bits closer together tip-bot2 for Sebastian Andrzej Siewior
2020-09-07 12:58         ` [PATCH 1/2] " Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200819133320.bxwb3ikjswyhmsyg@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=axboe@kernel.dk \
    --cc=bsegall@google.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=io-uring@vger.kernel.org \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=vincent.guittot@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.