From: Frederic Weisbecker <frederic@kernel.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
Levin Alexander <alexander.levin@verizon.com>,
Peter Zijlstra <peterz@infradead.org>,
Mauro Carvalho Chehab <mchehab@s-opensource.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Hannes Frederic Sowa <hannes@stressinduktion.org>,
"Paul E . McKenney" <paulmck@linux.vnet.ibm.com>,
Wanpeng Li <wanpeng.li@hotmail.com>,
Dmitry Safonov <dima@arista.com>,
Thomas Gleixner <tglx@linutronix.de>,
Andrew Morton <akpm@linux-foundation.org>,
Paolo Abeni <pabeni@redhat.com>, Radu Rendec <rrendec@arista.com>,
Ingo Molnar <mingo@kernel.org>,
Stanislaw Gruszka <sgruszka@redhat.com>,
Rik van Riel <riel@redhat.com>,
Eric Dumazet <edumazet@google.com>,
David Miller <davem@davemloft.net>
Subject: Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue
Date: Thu, 15 Feb 2018 17:13:52 +0100 [thread overview]
Message-ID: <20180215161349.GA6956@lerouge> (raw)
In-Reply-To: <20180208174450.qjvjy752jf4ngt2g@breakpoint.cc>
On Thu, Feb 08, 2018 at 06:44:52PM +0100, Sebastian Andrzej Siewior wrote:
> On 2018-01-19 16:46:12 [+0100], Frederic Weisbecker wrote:
> > diff --git a/kernel/softirq.c b/kernel/softirq.c
> > index c8c6841..becb1d9 100644
> > --- a/kernel/softirq.c
> > +++ b/kernel/softirq.c
> > @@ -62,6 +62,19 @@ const char * const softirq_to_name[NR_SOFTIRQS] = {
> …
> > +static void vector_work_func(struct work_struct *work)
> > +{
> > + struct vector *vector = container_of(work, struct vector, work);
> > + struct softirq *softirq = this_cpu_ptr(&softirq_cpu);
> > + int vec_nr = vector->nr;
> > + int vec_bit = BIT(vec_nr);
> > + u32 pending;
> > +
> > + local_irq_disable();
> > + pending = local_softirq_pending();
> > + account_irq_enter_time(current);
> > + __local_bh_disable_ip(_RET_IP_, SOFTIRQ_OFFSET);
> > + lockdep_softirq_enter();
> > + set_softirq_pending(pending & ~vec_bit);
> > + local_irq_enable();
> > +
> > + if (pending & vec_bit) {
> > + struct softirq_action *sa = &softirq_vec[vec_nr];
> > +
> > + kstat_incr_softirqs_this_cpu(vec_nr);
> > + softirq->work_running = 1;
> > + trace_softirq_entry(vec_nr);
> > + sa->action(sa);
>
> You invoke the softirq handler while BH is disabled (not wrong, I just
> state the obvious). That means, the scheduler can't preempt/interrupt
> the workqueue/BH-handler while it is invoked so it has to wait until it
> completes its doing.
> In do_softirq_workqueue() you schedule multiple workqueue items (one for
> each softirq vector) which is unnecessary because they can't preempt one
> another and should be invoked the order they were enqueued. So it would
> be enough to enqueue one item because it is serialized after all. So one
> work_struct per CPU with a cond_resched_rcu_qs() while switching from one
> vector to another should accomplish that what you have now here (not
> sure if that cond_resched after each vector is needed). But…
Makes sense.
>
> > + trace_softirq_exit(vec_nr);
> > + softirq->work_running = 0;
> > + }
> > +
> > + local_irq_disable();
> > +
> > + pending = local_softirq_pending();
> > + if (pending & vec_bit)
> > + schedule_work_on(smp_processor_id(), &vector->work);
>
> … on a system that is using system_wq a lot, it might introduced a certain
> latency until your softirq-worker gets its turn. The workqueue will
> spawn new workers if the current worker schedules out but until that
> happens you have to wait. I am not sure if this is intended or whether
> this might be a problem. I think you could argue either way depending on
> what you currently think is more important.
Indeed :)
> Further, schedule_work_on(x, ) does not guarentee that the work item is
> invoked on CPU x. It tries that but if CPU x goes down due to
> CPU-hotplug then the workitem will be moved to random CPU. For that
> reason we have work_on_cpu_safe() but you don't want to use that / flush
> that workqueue while in here.
Yeah, someone also reported me that hotplug issue. I didn't think workqueue
would break the affinity but here it does. So we would need a hotplug hook
indeed.
>
> May I instead suggest to stick to ksoftirqd? So you run in softirq
> context (after return from IRQ) and if takes too long, you offload the
> vector to ksoftirqd instead. You may want to play with the metric on
> which you decide when you want switch to ksoftirqd / account how long a
> vector runs.
Yeah that makes sense. These workqueues are too much headaches eventually.
I'm going to try that ksoftirqd thing.
Thanks.
next prev parent reply other threads:[~2018-02-15 16:13 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-19 15:46 [RFC PATCH 0/4] softirq: Per vector threading v3 Frederic Weisbecker
2018-01-19 15:46 ` [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail Frederic Weisbecker
2018-01-19 16:16 ` David Miller
2018-01-19 18:25 ` Linus Torvalds
2018-01-19 18:47 ` David Miller
2018-01-21 16:30 ` Frederic Weisbecker
2018-01-21 16:57 ` David Miller
2018-01-19 15:46 ` [RFC PATCH 2/4] softirq: Per vector deferment to workqueue Frederic Weisbecker
2018-01-20 8:41 ` Pavan Kondeti
2018-01-21 16:11 ` Frederic Weisbecker
2018-01-21 17:50 ` Pavan Kondeti
2018-01-21 20:48 ` Frederic Weisbecker
2018-02-08 17:44 ` Sebastian Andrzej Siewior
2018-02-08 18:45 ` David Miller
2018-02-08 20:14 ` Dmitry Safonov
2018-02-08 20:22 ` David Miller
2018-02-08 20:30 ` Dmitry Safonov
2018-02-09 4:11 ` Mike Galbraith
2018-02-09 12:35 ` Sebastian Andrzej Siewior
2018-02-15 16:13 ` Frederic Weisbecker [this message]
2018-02-15 16:58 ` Sebastian Andrzej Siewior
2018-01-19 15:46 ` [RFC PATCH 3/4] softirq: Defer to workqueue when rescheduling is needed Frederic Weisbecker
2018-01-19 15:46 ` [RFC PATCH 4/4] softirq: Replace ksoftirqd with workqueues entirely Frederic Weisbecker
2018-01-22 19:58 ` [RFC PATCH 0/4] softirq: Per vector threading v3 Mauro Carvalho Chehab
2018-01-23 10:13 ` Paolo Abeni
2018-01-23 12:32 ` Dmitry Safonov
2018-01-24 2:12 ` Frederic Weisbecker
2018-01-23 16:22 ` David Miller
2018-01-23 16:57 ` Paolo Abeni
2018-01-23 17:42 ` Linus Torvalds
2018-01-23 18:01 ` Mike Galbraith
2018-01-23 18:24 ` David Miller
2018-01-24 1:57 ` Frederic Weisbecker
2018-01-24 2:01 ` Frederic Weisbecker
2018-01-24 14:54 ` Paolo Abeni
2018-01-24 15:05 ` David Miller
2018-01-24 16:11 ` Paolo Abeni
2018-02-07 14:18 ` Mauro Carvalho Chehab
2018-03-01 15:21 ` Frederic Weisbecker
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=20180215161349.GA6956@lerouge \
--to=frederic@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=alexander.levin@verizon.com \
--cc=bigeasy@linutronix.de \
--cc=davem@davemloft.net \
--cc=dima@arista.com \
--cc=edumazet@google.com \
--cc=hannes@stressinduktion.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab@s-opensource.com \
--cc=mingo@kernel.org \
--cc=pabeni@redhat.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=riel@redhat.com \
--cc=rrendec@arista.com \
--cc=sgruszka@redhat.com \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=wanpeng.li@hotmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).