Skip to contentSkip to conversation

softirq: Per vector threading v3

RFC PATCH In review Frederic Weisbecker 9 participants 39 messages Jan 19, 2018 – Mar 1, 2018

Subsystems (6)
lore
Share & tools
lore

Frederic Weisbecker

author maintainer
#0 lore raw

cover softirq: Per vector threading v3

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

Thanks,
Frederic
---

Frederic Weisbecker (4):
softirq: Limit vector to a single iteration on IRQ tail
softirq: Per vector deferment to workqueue
softirq: Defer to workqueue when rescheduling is needed
softirq: Replace ksoftirqd with workqueues entirely

 Documentation/RCU/stallwarn.txt |   4 +-
 include/linux/interrupt.h       |   7 +-
 kernel/sched/cputime.c          |  12 +--
 kernel/sched/sched.h            |   4 +-
 kernel/softirq.c                | 223 +++++++++++++++++++++++-----------------
 net/ipv4/tcp_output.c           |   5 +-
 6 files changed, 144 insertions(+), 111 deletions(-)
author maintainer
#1 lore raw

RFC PATCH 1/4 softirq: Limit vector to a single iteration on IRQ tail

Softirqs are currently allowed to execute as much pending work as needed as long as it doesn't overrun 10 iterations or 2 ms. The rest of the pending work is deferred to ksoftirqd beyond that limit.

In fact 2 ms is a very high threshold that shouldn't be reachable after only 10 iterations. And just counting iterations doesn't help diagnose which vector forces softirqs to be deferred to a thread at the expense of other vectors that could need quick service. Instead of blindly kicking ksoftirqd when the softirq load becomes too high, we rather want to offload the culprit vector only and let the other vectors to be handled on IRQ tail as usual.

To achieve this, we rather keep track of which vectors have run on past iterations and kick them off to threading if they get re-enqueued. We assume that a vector only needs one iteration on normal loads while re-enqueuing is a matter of stress, so this should be a simple yet reliable way to detect which vector needs to be queued off in a task.

For now, vectors that get re-enqueued trigger ksoftirqd but they are going to be handled by per-vector workqueues on subsequent patches.

Cc Dmitry Safonov <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc David Miller <[email protected]>
Cc Hannes Frederic Sowa <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Levin Alexander <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Radu Rendec <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Stanislaw Gruszka <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Wanpeng Li <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
---
 kernel/softirq.c | 30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)
kernel/softirq.c +7 -23
index 2f5e87f..c8c6841 100644
@@ -190,22 +190,6 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
 }
 EXPORT_SYMBOL(__local_bh_enable_ip);
 
-/*
- * We restart softirq processing for at most MAX_SOFTIRQ_RESTART times,
- * but break the loop if need_resched() is set or after 2 ms.
- * The MAX_SOFTIRQ_TIME provides a nice upper bound in most cases, but in
- * certain cases, such as stop_machine(), jiffies may cease to
- * increment and so we need the MAX_SOFTIRQ_RESTART limit as
- * well to make sure we eventually return from this method.
- *
- * These limits have been established via experimentation.
- * The two things to balance is latency against fairness -
- * we want to handle softirqs as soon as possible, but they
- * should not be able to lock up the box.
- */
-#define MAX_SOFTIRQ_TIME  msecs_to_jiffies(2)
-#define MAX_SOFTIRQ_RESTART 10
-
 #ifdef CONFIG_TRACE_IRQFLAGS
 /*
  * When we run softirqs from irq_exit() and thus on the hardirq stack we need
@@ -241,12 +225,10 @@ static inline void lockdep_softirq_end(bool in_hardirq) { }
 
 asmlinkage __visible void __softirq_entry __do_softirq(void)
 {
-	unsigned long end = jiffies + MAX_SOFTIRQ_TIME;
 	unsigned long old_flags = current->flags;
-	int max_restart = MAX_SOFTIRQ_RESTART;
 	struct softirq_action *h;
 	bool in_hardirq;
-	__u32 pending;
+	__u32 pending, executed = 0;
 	int softirq_bit;
 
 	/*
@@ -284,6 +266,9 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 		trace_softirq_entry(vec_nr);
 		h->action(h);
 		trace_softirq_exit(vec_nr);
+
+		executed |= 1 << vec_nr;
+
 		if (unlikely(prev_count != preempt_count())) {
 			pr_err("huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
 			       vec_nr, softirq_to_name[vec_nr], h->action,
@@ -299,11 +284,10 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 
 	pending = local_softirq_pending();
 	if (pending) {
-		if (time_before(jiffies, end) && !need_resched() &&
-		    --max_restart)
+		if (pending & executed || need_resched())
+			wakeup_softirqd();
+		else
 			goto restart;
-
-		wakeup_softirqd();
 	}
 
 	lockdep_softirq_end(in_hardirq);
Signature · 1 line
2.7.4
maintainer
#2 lore raw

Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail

From: Frederic Weisbecker <[email protected]>
Date: Fri, 19 Jan 2018 16:46:11 +0100

Quoted 2 lines

For now, vectors that get re-enqueued trigger ksoftirqd but they are going to be handled by per-vector workqueues on subsequent patches.

Frederic, first of all, thanks for doing all of this work.

So this "get requeued" condition I think will trigger always for networking tunnel decapsulation.

Each decap will (eventually) do a:

	__raise_softirq_irqoff(NET_RX_SOFTIRQ);

via ____napi_schedule() in net/core/dev.c

Example code path:

	ip_tunnel_rcv()
		gro_cells_receive()
			napi_schedule()
			__napi_schedule()
				____napi_schedule();
				__raise_softirq_irqoff(NET_RX_SOFTIRQ);

Anyways, just FYI...

#3 lore raw

Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail

Quoted 3 lines · David Miller

So this "get requeued" condition I think will trigger always for networking tunnel decapsulation.

Hmm. Interesting and a perhaps bit discouraging.

Will it always be just a _single_ level of indirection, or will double tunnels (I assume some people do that, just because the universe is out to get us) then result in this perhaps repeating several times?

Because it would be not very complicated to just have two bitmasks (and still no per-softirq counting), and have that "switch to threading only on the _second_ time this happens".

But let's have people test the behavior of this simpler model first?

Linus

maintainer
#4 lore raw

Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail

From: Linus Torvalds <[email protected]>
Date: Fri, 19 Jan 2018 10:25:03 -0800

Quoted 10 lines · David Miller
Quoted 3 lines

So this "get requeued" condition I think will trigger always for networking tunnel decapsulation.

Hmm. Interesting and a perhaps bit discouraging.

Will it always be just a _single_ level of indirection, or will double tunnels (I assume some people do that, just because the universe is out to get us) then result in this perhaps repeating several times?

Every level of tunnel encapsulation will trigger a new softirq.

So if you have an IP tunnel inside of an IP tunnel that will trigger twice.

author maintainer
#5 lore raw

Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail

Quoted 10 lines · David Miller

From: Linus Torvalds <[email protected]>
Date: Fri, 19 Jan 2018 10:25:03 -0800

Quoted 7 lines · David Miller
Quoted 3 lines

So this "get requeued" condition I think will trigger always for networking tunnel decapsulation.

Hmm. Interesting and a perhaps bit discouraging.

Quoted 8 lines
Quoted 3 lines

Will it always be just a _single_ level of indirection, or will double tunnels (I assume some people do that, just because the universe is out to get us) then result in this perhaps repeating several times?

Every level of tunnel encapsulation will trigger a new softirq.

So if you have an IP tunnel inside of an IP tunnel that will trigger twice.

So we may likely need to come back to a call counter based limit :-s

maintainer
#6 lore raw

Re: [RFC PATCH 1/4] softirq: Limit vector to a single iteration on IRQ tail

From: Frederic Weisbecker <[email protected]>
Date: Sun, 21 Jan 2018 17:30:09 +0100

Quoted 15 lines · David Miller
Quoted 14 lines

From: Linus Torvalds <[email protected]>
Date: Fri, 19 Jan 2018 10:25:03 -0800

Quoted 10 lines · David Miller
Quoted 3 lines

So this "get requeued" condition I think will trigger always for networking tunnel decapsulation.

Hmm. Interesting and a perhaps bit discouraging.

Will it always be just a _single_ level of indirection, or will double tunnels (I assume some people do that, just because the universe is out to get us) then result in this perhaps repeating several times?

Quoted 6 lines
Quoted 4 lines

Every level of tunnel encapsulation will trigger a new softirq.

So if you have an IP tunnel inside of an IP tunnel that will trigger twice.

So we may likely need to come back to a call counter based limit :-s

I'm not so sure exactly to what extent we should try to handle that, and if so exactly how.

The only reason we do this is to control stack usage. The re-softirq on tunnel decapsulation is functioning as a continuation of sorts. If we are already running in the net_rx_action() softirq it would be so much nicer and efficient to just make sure net_rx_action() runs the rest of the packet processing.

Right now that doesn't happen because net_rx_action() runs only one round of NAPI polling. It doesn't re-snapshot the list and try again before returning.

net_rx_action() has it's own logic like do_softirq() does for timing out in the middle of it's work which may or may not have some further influence upon fairness to other softirqs.

Basically, it runs a snapshot the NAPI poll list for this CPU until either usecs_to_jiffies(netdev_budget_usecs) jiffies have elapsed or the list snapshot has been fully processed.

The default netdev_budget_usecs is 2000, which is my math isn't broken is 2 jiffies when HZ=1000. I know why we use 2000 instead of 1000, it's to handle the case where we are invoked very close to the end of a jiffy. That situation does happen often enough in practice to cause performance problems.

It would seem that all of these issues are why the tendency is to deal with measuring cost using time rather than a simpler heuristic such as whether softirqs were retriggered during a softirq run.

author maintainer
#7 lore raw

RFC PATCH 2/4 softirq: Per vector deferment to workqueue

In reply to Frederic Weisbecker

Some softirq vectors can be more CPU hungry than others. Especially networking may sometimes deal with packet storm and need more CPU than IRQ tail can offer without inducing scheduler latencies. In this case the current code defers to ksoftirqd that behaves nicer. Now this nice behaviour can be bad for other IRQ vectors that usually need quick processing.

To solve this we only defer to threading the vectors that got re-enqueued on IRQ tail processing and leave the others inline on IRQ tail service. This is achieved using workqueues with per-CPU/per-vector worklets.

Note ksoftirqd is not yet removed as it is still needed for threaded IRQs mode.

Cc Dmitry Safonov <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc David Miller <[email protected]>
Cc Hannes Frederic Sowa <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Levin Alexander <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Radu Rendec <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Stanislaw Gruszka <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Wanpeng Li <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
---
 include/linux/interrupt.h |   2 +
 kernel/sched/cputime.c    |   5 +-
 kernel/softirq.c          | 120 ++++++++++++++++++++++++++++++++++++++++++----
 net/ipv4/tcp_output.c     |   3 +-
 4 files changed, 119 insertions(+), 11 deletions(-)
include/linux/interrupt.h +2 -0
index 69c2382..92d044d 100644
@@ -514,6 +514,8 @@ static inline struct task_struct *this_cpu_ksoftirqd(void)
 	return this_cpu_read(ksoftirqd);
 }
 
+extern int softirq_serving_workqueue(void);
+
 /* Tasklets --- multithreaded analogue of BHs.
 
    Main feature differing them of generic softirqs: tasklet
kernel/sched/cputime.c +3 -2
index bac6ac9..30f70e5 100644
@@ -71,7 +71,8 @@ void irqtime_account_irq(struct task_struct *curr)
 	 */
 	if (hardirq_count())
 		irqtime_account_delta(irqtime, delta, CPUTIME_IRQ);
-	else if (in_serving_softirq() && curr != this_cpu_ksoftirqd())
+	else if (in_serving_softirq() && curr != this_cpu_ksoftirqd() &&
+		 !softirq_serving_workqueue())
 		irqtime_account_delta(irqtime, delta, CPUTIME_SOFTIRQ);
 }
 EXPORT_SYMBOL_GPL(irqtime_account_irq);
@@ -375,7 +376,7 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
 
 	cputime -= other;
 
-	if (this_cpu_ksoftirqd() == p) {
+	if (this_cpu_ksoftirqd() == p || softirq_serving_workqueue()) {
 		/*
 		 * ksoftirqd time do not get accounted in cpu_softirq_time.
 		 * So, we have to handle it separately here.
kernel/softirq.c +112 -8

163 lines not on this page · raw message

net/ipv4/tcp_output.c +2 -1
index a4d214c..b4e4160 100644
@@ -919,7 +919,8 @@ void tcp_wfree(struct sk_buff *skb)
 	 * - chance for incoming ACK (processed by another cpu maybe)
 	 *   to migrate this flow (skb->ooo_okay will be eventually set)
 	 */
-	if (refcount_read(&sk->sk_wmem_alloc) >= SKB_TRUESIZE(1) && this_cpu_ksoftirqd() == current)
+	if (refcount_read(&sk->sk_wmem_alloc) >= SKB_TRUESIZE(1) &&
+	    (this_cpu_ksoftirqd() == current || softirq_serving_workqueue()))
 		goto out;
 
 	for (oval = READ_ONCE(sk->sk_tsq_flags);; oval = nval) {
Signature · 1 line
2.7.4
#8 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Hi Frederic,

Quoted 15 lines · Frederic Weisbecker

Some softirq vectors can be more CPU hungry than others. Especially networking may sometimes deal with packet storm and need more CPU than IRQ tail can offer without inducing scheduler latencies. In this case the current code defers to ksoftirqd that behaves nicer. Now this nice behaviour can be bad for other IRQ vectors that usually need quick processing.

To solve this we only defer to threading the vectors that got re-enqueued on IRQ tail processing and leave the others inline on IRQ tail service. This is achieved using workqueues with per-CPU/per-vector worklets.

Note ksoftirqd is not yet removed as it is still needed for threaded IRQs mode.

<snip>

Quoted 12 lines
kernel/softirq.c +12 -0
+static void do_softirq_workqueue(u32 pending)
+{
+	struct softirq *softirq = this_cpu_ptr(&softirq_cpu);
+	struct softirq_action *h = softirq_vec;
+	int softirq_bit;
+
+	pending &= ~softirq->pending_work_mask;
+
+	while ((softirq_bit = ffs(pending))) {
+		struct vector *vector;
+		unsigned int vec_nr;
+

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 306

Quoted 10 lines
kernel/softirq.c +10 -0
+		h += softirq_bit - 1;
+		vec_nr = h - softirq_vec;
+		softirq->pending_work_mask |= BIT(vec_nr);
+		vector = &softirq->vector[vec_nr];
+		schedule_work_on(smp_processor_id(), &vector->work);
+		h++;
+		pending >>= softirq_bit;
+	}
+}
+

I have couple questions/comments.

(1) Since the work is queued on a bounded per-cpu worker, we may run into a deadlock if a TASKLET is killed from another work running on the same bounded per-cpu worker.

For example,

(1) Schedule a TASKLET on CPU#0 from IRQ.
(2) Another IRQ comes on the same CPU and we queue a work to kill the TASKLET.
(3) The TASKLET vector is deferred to workqueue.
(4) We run the TASKLET kill work and wait for the TASKLET to finish, which won't happen.

We can fix this by queueing the TASKLET kill work on an unbounded workqueue so that this runs in parallel with TASKLET vector work.

Just wanted to know if we have to be aware of this *condition*.

(2) Ksoftirqd thread gets parked when a CPU is hotplugged out. So there is a gaurantee that the softirq handling never happens on another CPU. Where as a bounded worker gets detached and the queued work can run on another CPU. I guess, some special handling is needed to handle hotplug.

Thanks,
Pavan

Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author maintainer
#9 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

On Sat, Jan 20, 2018 at 02:11:39PM +0530, Pavan Kondeti wrote:

Hi Pavan,

Quoted 15 lines

I have couple questions/comments.

(1) Since the work is queued on a bounded per-cpu worker, we may run into a deadlock if a TASKLET is killed from another work running on the same bounded per-cpu worker.

For example,

(1) Schedule a TASKLET on CPU#0 from IRQ.
(2) Another IRQ comes on the same CPU and we queue a work to kill the TASKLET.
(3) The TASKLET vector is deferred to workqueue.
(4) We run the TASKLET kill work and wait for the TASKLET to finish, which won't happen.

Quoted 4 lines

We can fix this by queueing the TASKLET kill work on an unbounded workqueue so that this runs in parallel with TASKLET vector work.

Just wanted to know if we have to be aware of this *condition*.

But IIRC the workqueues have several workers per CPU so the tasklet to be killed can run while the tasklet killer yields.

Quoted 6 lines

(2) Ksoftirqd thread gets parked when a CPU is hotplugged out. So there is a gaurantee that the softirq handling never happens on another CPU. Where as a bounded worker gets detached and the queued work can run on another CPU. I guess, some special handling is needed to handle hotplug.

Good catch. Funny, I worried a bit about CPU hotplug but I assumed the pending CPU-bound worklets would be simply sync'ed before CPU gets down.

Breaking their CPU-bound properties doesn't look sane to me.

Anyway, I'll need to make a CPU hotplug hook.

Thanks for reporting that!

#10 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 20 lines · Frederic Weisbecker

On Sat, Jan 20, 2018 at 02:11:39PM +0530, Pavan Kondeti wrote:

Hi Pavan,

Quoted 15 lines

I have couple questions/comments.

(1) Since the work is queued on a bounded per-cpu worker, we may run into a deadlock if a TASKLET is killed from another work running on the same bounded per-cpu worker.

For example,

(1) Schedule a TASKLET on CPU#0 from IRQ.
(2) Another IRQ comes on the same CPU and we queue a work to kill the TASKLET.
(3) The TASKLET vector is deferred to workqueue.
(4) We run the TASKLET kill work and wait for the TASKLET to finish, which won't happen.

Quoted 8 lines
Quoted 4 lines

We can fix this by queueing the TASKLET kill work on an unbounded workqueue so that this runs in parallel with TASKLET vector work.

Just wanted to know if we have to be aware of this *condition*.

But IIRC the workqueues have several workers per CPU so the tasklet to be killed can run while the tasklet killer yields.

AFAIK, the work items queued via schedule_work_on() goes to the system_wq which is bounded to a CPU with concurrency restrictions. If any work item (in this case tasklet kill) is getting executed on this bounded worker, the next items have to wait. The forward progress happens only when the current work is finished or enters sleep.

This also makes me wonder what happens if a CPU hogging work gets executed on the system_wq while softirq work is pending? The softirq work gets starved which won't happen now with ksoftirqd design. Ideally the CPU hogging work should not be queued on the system_wq and instead should be queued on CPU intenstive workqueue (WQ_CPU_INTENSIVE) to exempt from concurrency management. May be we need some special workqueue which is bounded but not subjected to concurrency management.

Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author maintainer
#11 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 26 lines · Pavan Kondeti
Quoted 25 lines · Frederic Weisbecker

On Sat, Jan 20, 2018 at 02:11:39PM +0530, Pavan Kondeti wrote:

Hi Pavan,

Quoted 19 lines

I have couple questions/comments.

(1) Since the work is queued on a bounded per-cpu worker, we may run into a deadlock if a TASKLET is killed from another work running on the same bounded per-cpu worker.

For example,

(1) Schedule a TASKLET on CPU#0 from IRQ.
(2) Another IRQ comes on the same CPU and we queue a work to kill the TASKLET.
(3) The TASKLET vector is deferred to workqueue.
(4) We run the TASKLET kill work and wait for the TASKLET to finish, which won't happen.

We can fix this by queueing the TASKLET kill work on an unbounded workqueue so that this runs in parallel with TASKLET vector work.

Just wanted to know if we have to be aware of this *condition*.

Quoted 8 lines
Quoted 3 lines

But IIRC the workqueues have several workers per CPU so the tasklet to be killed can run while the tasklet killer yields.

AFAIK, the work items queued via schedule_work_on() goes to the system_wq which is bounded to a CPU with concurrency restrictions. If any work item (in this case tasklet kill) is getting executed on this bounded worker, the next items have to wait. The forward progress happens only when the current work is finished or enters sleep.

Workqueues have multiple workers to handle the pending works, unless they are __WQ_ORDERED.

The worker manager in a pool is supposed to create workers on-demand when necessary to make sure that no work is blocking the others.

Thanks.

Quoted 13 lines

This also makes me wonder what happens if a CPU hogging work gets executed on the system_wq while softirq work is pending? The softirq work gets starved which won't happen now with ksoftirqd design. Ideally the CPU hogging work should not be queued on the system_wq and instead should be queued on CPU intenstive workqueue (WQ_CPU_INTENSIVE) to exempt from concurrency management. May be we need some special workqueue which is bounded but not subjected to concurrency management.

Signature · 2 lines
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
#12 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

In reply to Frederic Weisbecker
Quoted 5 lines · Frederic Weisbecker
kernel/softirq.c +0 -0
index c8c6841..becb1d9 100644
@@ -62,6 +62,19 @@ const char * const softirq_to_name[NR_SOFTIRQS] = {

…

Quoted 13 lines
kernel/softirq.c +13 -0
+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();

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 266

Quoted 10 lines
kernel/softirq.c +10 -0
+	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…

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 275

Quoted 9 lines
kernel/softirq.c +9 -0
+		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.
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.

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.

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 283

Quoted 8 lines
kernel/softirq.c +8 -0
+	else
+		softirq->pending_work_mask &= ~vec_bit;
+
+	lockdep_softirq_exit();
+	account_irq_exit_time(current);
+	__local_bh_enable(SOFTIRQ_OFFSET);
+	local_irq_enable();
+}

Sebastian

maintainer
#13 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

From: Sebastian Andrzej Siewior <[email protected]>
Date: Thu, 8 Feb 2018 18:44:52 +0100

Quoted 5 lines

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.

Having read over this stuff for the past few weeks this is how I feel as well. Just make ksofbitrq do what we want (only execute the overloaded softirq vectors).

The more I look at the workqueue stuff, the more complications and weird behavioral artifacts we are getting for questionable gain.

#14 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 10 lines · David Miller

From: Sebastian Andrzej Siewior <[email protected]>
Date: Thu, 8 Feb 2018 18:44:52 +0100

Quoted 2 lines

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

Quoted 1 line

vector to ksoftirqd instead. You may want to play with the metric

on

Quoted 1 line

which you decide when you want switch to ksoftirqd / account how

long a

Quoted 8 lines
Quoted 1 line

vector runs.

Having read over this stuff for the past few weeks this is how I feel as well. Just make ksofbitrq do what we want (only execute the overloaded softirq vectors).

The more I look at the workqueue stuff, the more complications and weird behavioral artifacts we are getting for questionable gain.

What about creating several ksoftirqd threads per-cpu? Like I did with boot parameter to specify how many threads and which softirqs to serve.

Signature · 2 lines
Thanks,
             Dmitry
maintainer
#15 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

From: Dmitry Safonov <[email protected]>
Date: Thu, 08 Feb 2018 20:14:55 +0000

Quoted 17 lines · David Miller
Quoted 16 lines

From: Sebastian Andrzej Siewior <[email protected]>
Date: Thu, 8 Feb 2018 18:44:52 +0100

Quoted 2 lines

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

Quoted 1 line

vector to ksoftirqd instead. You may want to play with the metric

on

Quoted 1 line

which you decide when you want switch to ksoftirqd / account how

long a

Quoted 1 line

vector runs.

Having read over this stuff for the past few weeks this is how I feel as well. Just make ksofbitrq do what we want (only execute the overloaded softirq vectors).

Quoted 6 lines
Quoted 2 lines

The more I look at the workqueue stuff, the more complications and weird behavioral artifacts we are getting for questionable gain.

What about creating several ksoftirqd threads per-cpu? Like I did with boot parameter to specify how many threads and which softirqs to serve.

Why do we need more than one per cpu?

There is a set of vectors which are "overloaded" and ksoftirqd processes them one by one.

The only difference with what happens now is that one softirq being overloaded doesn't defer the processing of all softirqs to ksoftirqd.

#16 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 27 lines · David Miller

From: Dmitry Safonov <[email protected]>
Date: Thu, 08 Feb 2018 20:14:55 +0000

Quoted 5 lines · David Miller
Quoted 4 lines

From: Sebastian Andrzej Siewior <[email protected]>
Date: Thu, 8 Feb 2018 18:44:52 +0100

Quoted 1 line

May I instead suggest to stick to ksoftirqd? So you run in

softirq

Quoted 1 line
Quoted 1 line
Quoted 1 line

context (after return from IRQ) and if takes too long, you

offload

Quoted 2 lines
Quoted 2 lines

the

Quoted 1 line

vector to ksoftirqd instead. You may want to play with the

metric

Quoted 6 lines
Quoted 6 lines

on

Quoted 1 line

which you decide when you want switch to ksoftirqd / account how

long a

Quoted 1 line

vector runs.

Having read over this stuff for the past few weeks this is how I

feel

Quoted 6 lines
Quoted 5 lines

as well. Just make ksofbitrq do what we want (only execute the overloaded softirq vectors).

The more I look at the workqueue stuff, the more complications and weird behavioral artifacts we are getting for questionable gain.

Quoted 6 lines
Quoted 2 lines

What about creating several ksoftirqd threads per-cpu? Like I did with boot parameter to specify how many threads and

which

Quoted 1 line

softirqs to serve.

Why do we need more than one per cpu?

Ugh, yeah, I remember why I did it - I tried to reuse scheduler for each ksoftirqd thread to decide if it need to run now or later. That would give an admin a way to prioritise softirqs with nice. Not sure if it's a nice idea at all..

Quoted 7 lines

There is a set of vectors which are "overloaded" and ksoftirqd processes
them one by one.

The only difference with what happens now is that one softirq being overloaded doesn't defer the processing of all softirqs to ksoftirqd.

#17 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 31 lines · Dmitry Safonov
Quoted 30 lines · David Miller

From: Dmitry Safonov <[email protected]>
Date: Thu, 08 Feb 2018 20:14:55 +0000

Quoted 5 lines · David Miller
Quoted 4 lines

From: Sebastian Andrzej Siewior <[email protected]>
Date: Thu, 8 Feb 2018 18:44:52 +0100

Quoted 1 line

May I instead suggest to stick to ksoftirqd? So you run in

softirq

Quoted 1 line
Quoted 1 line
Quoted 1 line

context (after return from IRQ) and if takes too long, you

offload

Quoted 2 lines
Quoted 2 lines

the

Quoted 1 line

vector to ksoftirqd instead. You may want to play with the

metric

Quoted 6 lines
Quoted 6 lines

on

Quoted 1 line

which you decide when you want switch to ksoftirqd / account how

long a

Quoted 1 line

vector runs.

Having read over this stuff for the past few weeks this is how I

feel

Quoted 8 lines
Quoted 5 lines

as well. Just make ksofbitrq do what we want (only execute the overloaded softirq vectors).

The more I look at the workqueue stuff, the more complications and weird behavioral artifacts we are getting for questionable gain.

What about creating several ksoftirqd threads per-cpu? Like I did with boot parameter to specify how many threads and

which

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

softirqs to serve.

Why do we need more than one per cpu?

Ugh, yeah, I remember why I did it - I tried to reuse scheduler for each ksoftirqd thread to decide if it need to run now or later. That would give an admin a way to prioritise softirqs with nice. Not sure if it's a nice idea at all..

For RT that can be handy, but for the general case it's a waste of cycles, so would want to be opt-in.

-Mike

#18 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 11 lines · Mike Galbraith
Quoted 7 lines · Dmitry Safonov
Quoted 1 line · David Miller

Why do we need more than one per cpu?

Ugh, yeah, I remember why I did it - I tried to reuse scheduler for each ksoftirqd thread to decide if it need to run now or later. That would give an admin a way to prioritise softirqs with nice. Not sure if it's a nice idea at all..

For RT that can be handy, but for the general case it's a waste of cycles, so would want to be opt-in.

We used to have it in RT. It got replaced because the context switch from one thread to the other simply took time.
If you need to prioritise one softirqs over the other I think you would be better off running with threaded-interrupts. In "normal" mode, if you have a raised NAPI and say a TASKLET then once both IRQ handler complete (and interrupts get enabled again) then the softirqs are run. With threaded interrupts enabled, the NAPI-softirq is invoked once the threaded-handler completes that raised it - same goes the other threaded handler which raised the tasklet softirq.
So you could simply change the priority of the threaded interrupt in order to prefer NAPI over tasklet handling (or the other way around).

Quoted 1 line

-Mike

Sebastian

author maintainer
#19 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

In reply to Sebastian Andrzej Siewior
Quoted 34 lines · Sebastian Andrzej Siewior
Quoted 5 lines · Frederic Weisbecker
kernel/softirq.c +0 -0
index c8c6841..becb1d9 100644
@@ -62,6 +62,19 @@ const char * const softirq_to_name[NR_SOFTIRQS] = {

…

Quoted 13 lines
kernel/softirq.c +13 -0
+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();

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 266

Quoted 10 lines
kernel/softirq.c +10 -0
+	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

Quoted 8 lines

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.

Quoted 17 lines

kernel/softirq.c static inline bool lockdep_softirq_start(void) { return false; } · line 275

Quoted 10 lines
kernel/softirq.c +9 -0
+		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 :)

Quoted 5 lines

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.

Quoted 6 lines

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.

#20 lore raw

Re: [RFC PATCH 2/4] softirq: Per vector deferment to workqueue

Quoted 2 lines · Frederic Weisbecker

Yeah that makes sense. These workqueues are too much headaches eventually. I'm going to try that ksoftirqd thing.

I may have something for that USB problem, I just need some testing before posting…

Quoted 1 line

Thanks.

Sebastian

author maintainer
#21 lore raw

RFC PATCH 3/4 softirq: Defer to workqueue when rescheduling is needed

In reply to Frederic Weisbecker

One more step toward converting ksoftirqd to per vector workqueues.

Cc Dmitry Safonov <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc David Miller <[email protected]>
Cc Hannes Frederic Sowa <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Levin Alexander <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Radu Rendec <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Stanislaw Gruszka <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Wanpeng Li <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
---
 kernel/softirq.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
kernel/softirq.c +8 -10
index becb1d9..bb0cffa 100644
@@ -369,16 +369,14 @@ asmlinkage __visible void __softirq_entry __do_softirq(void)
 
 	pending = local_softirq_pending() & ~softirq->pending_work_mask;
 	if (pending) {
-		if (need_resched()) {
-			wakeup_softirqd();
-		} else {
-			/* Vectors that got re-enqueued are threaded */
-			if (executed & pending)
-				do_softirq_workqueue(executed & pending);
-			pending &= ~executed;
-			if (pending)
-				goto restart;
-		}
+		if (need_resched())
+			executed = pending;
+		/* Vectors that got re-enqueued are threaded */
+		if (executed & pending)
+			do_softirq_workqueue(executed & pending);
+		pending &= ~executed;
+		if (pending)
+			goto restart;
 	}
 
 	lockdep_softirq_end(in_hardirq);
Signature · 1 line
2.7.4
author maintainer
#22 lore raw

RFC PATCH 4/4 softirq: Replace ksoftirqd with workqueues entirely

In reply to Frederic Weisbecker

Ksoftirqd only remains to implement threaded IRQs. Convert it to existing per-vector workqueues to avoid code duplication.

Cc Dmitry Safonov <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc David Miller <[email protected]>
Cc Hannes Frederic Sowa <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Levin Alexander <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Paul E. McKenney <[email protected]>
Cc Radu Rendec <[email protected]>
Cc Rik van Riel <[email protected]>
Cc Stanislaw Gruszka <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Wanpeng Li <[email protected]>
Cc Mauro Carvalho Chehab <[email protected]>
---
 Documentation/RCU/stallwarn.txt |  4 +-
 include/linux/interrupt.h       |  7 ----
 kernel/sched/cputime.c          | 13 +++---
 kernel/sched/sched.h            |  4 +-
 kernel/softirq.c                | 87 +++++++++--------------------------------
 net/ipv4/tcp_output.c           |  4 +-
 6 files changed, 31 insertions(+), 88 deletions(-)
Documentation/RCU/stallwarn.txt +2 -2
index a08f928..ea3a8de 100644
@@ -17,8 +17,8 @@ o	A CPU looping in an RCU read-side critical section.
 o	A CPU looping with interrupts disabled.
 
 o	A CPU looping with preemption disabled.  This condition can
-	result in RCU-sched stalls and, if ksoftirqd is in use, RCU-bh
-	stalls.
+	result in RCU-sched stalls and, if softirq workqueue is in use,
+	RCU-bh stalls.
 
 o	A CPU looping with bottom halves disabled.  This condition can
 	result in RCU-sched and RCU-bh stalls.
include/linux/interrupt.h +0 -7
index 92d044d..680f620 100644
@@ -507,13 +507,6 @@ extern void __raise_softirq_irqoff(unsigned int nr);
 extern void raise_softirq_irqoff(unsigned int nr);
 extern void raise_softirq(unsigned int nr);
 
-DECLARE_PER_CPU(struct task_struct *, ksoftirqd);
-
-static inline struct task_struct *this_cpu_ksoftirqd(void)
-{
-	return this_cpu_read(ksoftirqd);
-}
-
 extern int softirq_serving_workqueue(void);
 
 /* Tasklets --- multithreaded analogue of BHs.
kernel/sched/cputime.c +6 -7
index 30f70e5..c5b8dbd 100644
@@ -64,15 +64,14 @@ void irqtime_account_irq(struct task_struct *curr)
 	irqtime->irq_start_time += delta;
 
 	/*
-	 * We do not account for softirq time from ksoftirqd here.
-	 * We want to continue accounting softirq time to ksoftirqd thread
+	 * We do not account for softirq time from workqueue here.
+	 * We want to continue accounting softirq time to workqueue thread
 	 * in that case, so as not to confuse scheduler with a special task
 	 * that do not consume any time, but still wants to run.
 	 */
 	if (hardirq_count())
 		irqtime_account_delta(irqtime, delta, CPUTIME_IRQ);
-	else if (in_serving_softirq() && curr != this_cpu_ksoftirqd() &&
-		 !softirq_serving_workqueue())
+	else if (in_serving_softirq() && !softirq_serving_workqueue())
 		irqtime_account_delta(irqtime, delta, CPUTIME_SOFTIRQ);
 }
 EXPORT_SYMBOL_GPL(irqtime_account_irq);
@@ -376,11 +375,11 @@ static void irqtime_account_process_tick(struct task_struct *p, int user_tick,
 
 	cputime -= other;
 
-	if (this_cpu_ksoftirqd() == p || softirq_serving_workqueue()) {
+	if (softirq_serving_workqueue()) {
 		/*
-		 * ksoftirqd time do not get accounted in cpu_softirq_time.
+		 * Softirq wq time do not get accounted in cpu_softirq_time.
 		 * So, we have to handle it separately here.
-		 * Also, p->stime needs to be updated for ksoftirqd.
+		 * Also, p->stime needs to be updated for workqueue.
 		 */
 		account_system_index_time(p, cputime, CPUTIME_SOFTIRQ);
 	} else if (user_tick) {
kernel/sched/sched.h +2 -2
index b19552a2..5d481f1 100644
@@ -2061,8 +2061,8 @@ struct irqtime {
 DECLARE_PER_CPU(struct irqtime, cpu_irqtime);
 
 /*
- * Returns the irqtime minus the softirq time computed by ksoftirqd.
- * Otherwise ksoftirqd's sum_exec_runtime is substracted its own runtime
+ * Returns the irqtime minus the softirq time computed by workqueue.
+ * Otherwise workqueue's sum_exec_runtime is substracted its own runtime
  * and never move forward.
  */
 static inline u64 irq_time_read(int cpu)
kernel/softirq.c +19 -68

162 lines not on this page · raw message

net/ipv4/tcp_output.c +2 -2
index b4e4160..3b4811e 100644
@@ -912,7 +912,7 @@ void tcp_wfree(struct sk_buff *skb)
 	 */
 	WARN_ON(refcount_sub_and_test(skb->truesize - 1, &sk->sk_wmem_alloc));
 
-	/* If this softirq is serviced by ksoftirqd, we are likely under stress.
+	/* If this softirq is serviced by workqueue, we are likely under stress.
 	 * Wait until our queues (qdisc + devices) are drained.
 	 * This gives :
 	 * - less callbacks to tcp_write_xmit(), reducing stress (batches)
@@ -920,7 +920,7 @@ void tcp_wfree(struct sk_buff *skb)
 	 *   to migrate this flow (skb->ooo_okay will be eventually set)
 	 */
 	if (refcount_read(&sk->sk_wmem_alloc) >= SKB_TRUESIZE(1) &&
-	    (this_cpu_ksoftirqd() == current || softirq_serving_workqueue()))
+	    softirq_serving_workqueue())
 		goto out;
 
 	for (oval = READ_ONCE(sk->sk_tsq_flags);; oval = nval) {
Signature · 1 line
2.7.4
#23 lore raw
In reply to Frederic Weisbecker

Em Fri, 19 Jan 2018 16:46:10 +0100
Frederic Weisbecker <[email protected]> escreveu:

Quoted 8 lines

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
softirq/thread-v3

Frederic,

FYI, I'm out of town for two weeks. I'll try to setup a test environment while traveling, but I'm unsure if it will work, as I won't have access to my DVB-S2 provider from here.

Quoted 14 lines

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

Thanks,
Frederic
---

Frederic Weisbecker (4):
softirq: Limit vector to a single iteration on IRQ tail
softirq: Per vector deferment to workqueue
softirq: Defer to workqueue when rescheduling is needed
softirq: Replace ksoftirqd with workqueues entirely

Quoted 7 lines
 Documentation/RCU/stallwarn.txt |   4 +-
 include/linux/interrupt.h       |   7 +-
 kernel/sched/cputime.c          |  12 +--
 kernel/sched/sched.h            |   4 +-
 kernel/softirq.c                | 223 +++++++++++++++++++++++-----------------
 net/ipv4/tcp_output.c           |   5 +-
 6 files changed, 144 insertions(+), 111 deletions(-)

Cheers,
Mauro

maintainer
#24 lore raw
In reply to Frederic Weisbecker

Hi,

Quoted 10 lines · Frederic Weisbecker

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

I tested this series in the UDP flood scenario, binding the user space process receiving the packets on the same CPU processing the related IRQ, and the tput sinks nearly to 0, like before Eric's patch.

The perf tool says that almost all the softirq processing is done inside the workqueue, but the user space process is scheduled very rarely, while before this series, in this scenario, ksoftirqd and the user space process got a fair share of the CPU time.

Cheers,

Paolo

#25 lore raw
Quoted 22 lines · Paolo Abeni

Hi,

Quoted 6 lines · Frederic Weisbecker

As per Linus suggestion, this take doesn't limit the number of occurences
per jiffy anymore but instead defers a vector to workqueues as soon as
it gets re-enqueued on IRQ tail.

Quoted 7 lines

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-
dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

I tested this series in the UDP flood scenario, binding the user space
process receiving the packets on the same CPU processing the related IRQ, and the tput sinks nearly to 0, like before Eric's patch.

Quoted 4 lines

The perf tool says that almost all the softirq processing is done inside the workqueue, but the user space process is scheduled very rarely, while before this series, in this scenario, ksoftirqd and the user space process got a fair share of the CPU time.

It get a fair share of the CPU time only on some lucky platforms (maybe on the most). On other not-so-lucky platforms it was also unfair - as I've previously described by the reason of slow raising softirqs :-/

Signature · 2 lines
Thanks,
             Dmitry
author maintainer
#26 lore raw
Quoted 23 lines · Dmitry Safonov
Quoted 22 lines · Paolo Abeni

Hi,

Quoted 6 lines · Frederic Weisbecker

As per Linus suggestion, this take doesn't limit the number of occurences
per jiffy anymore but instead defers a vector to workqueues as soon as
it gets re-enqueued on IRQ tail.

Quoted 7 lines

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-
dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

I tested this series in the UDP flood scenario, binding the user space
process receiving the packets on the same CPU processing the related IRQ, and the tput sinks nearly to 0, like before Eric's patch.

Quoted 8 lines
Quoted 4 lines

The perf tool says that almost all the softirq processing is done inside the workqueue, but the user space process is scheduled very rarely, while before this series, in this scenario, ksoftirqd and the user space process got a fair share of the CPU time.

It get a fair share of the CPU time only on some lucky platforms (maybe on the most). On other not-so-lucky platforms it was also unfair - as I've previously described by the reason of slow raising softirqs :-/

Indeed your case looks pretty special, as IRQs don't get the opportunity to interrupt softirqs but they fire right after.

I'm wondering if you shouldn't try threaded IRQs. Having softirqs always running in a thread instead of hardirq tail might help (through threadirqs= kernel param).

maintainer
#27 lore raw
In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Tue, 23 Jan 2018 11:13:52 +0100

Quoted 14 lines

Hi,

Quoted 10 lines · Frederic Weisbecker

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

Quoted 8 lines

I tested this series in the UDP flood scenario, binding the user space process receiving the packets on the same CPU processing the related IRQ, and the tput sinks nearly to 0, like before Eric's patch.

The perf tool says that almost all the softirq processing is done inside the workqueue, but the user space process is scheduled very rarely, while before this series, in this scenario, ksoftirqd and the user space process got a fair share of the CPU time.

Do workqueue threads get a higher scheduling priority than user processes? If so, that's going to work against the entire point of deferring softirq work into a thread.

Or is it that the workqueue execution is simply not yielding for some reason?

maintainer
#28 lore raw
Quoted 21 lines · David Miller

From: Paolo Abeni <[email protected]>
Date: Tue, 23 Jan 2018 11:13:52 +0100

Quoted 18 lines

Hi,

Quoted 10 lines · Frederic Weisbecker

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
softirq/thread-v3

HEAD: 6835e92cbd70ef4a056987d2e1ed383b294429d4

I tested this series in the UDP flood scenario, binding the user space process receiving the packets on the same CPU processing the related IRQ, and the tput sinks nearly to 0, like before Eric's patch.

Quoted 7 lines
Quoted 4 lines

The perf tool says that almost all the softirq processing is done inside the workqueue, but the user space process is scheduled very rarely, while before this series, in this scenario, ksoftirqd and the user space process got a fair share of the CPU time.

Do workqueue threads get a higher scheduling priority than user processes?

As far as I can see, no: the workqueue thread has the same priority and nice level than the user space process.

Quoted 2 lines

Or is it that the workqueue execution is simply not yielding for some reason?

It's like that.

I spent little time on it, so I haven't many data point. I'll try to investigate the scenario later this week.

Cheers,

Paolo

#29 lore raw
Quoted 8 lines · Paolo Abeni
Quoted 3 lines

Or is it that the workqueue execution is simply not yielding for some reason?

It's like that.

I spent little time on it, so I haven't many data point. I'll try to investigate the scenario later this week.

Hmm. workqueues seem to use cond_resched_rcu_qs(), which does a cond_resched() (and a RCU quiescent note).

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

Linus

#30 lore raw
Quoted 10 lines · Linus Torvalds
Quoted 8 lines · Paolo Abeni
Quoted 3 lines

Or is it that the workqueue execution is simply not yielding for some reason?

It's like that.

I spent little time on it, so I haven't many data point. I'll try to investigate the scenario later this week.

Quoted 7 lines

Hmm. workqueues seem to use cond_resched_rcu_qs(), which does a cond_resched() (and a RCU quiescent note).

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

If folks aren't careful with workqueues, they can be a generic starvation problem. Like the below in the here and now.

fs/nfs: Add a resched point to nfs_commit_release_pages()

During heavy NFS write, kworkers can do very large amounts of work without scheduling (82ms traced). Add a resched point.

---
 fs/nfs/write.c |    1 +
 1 file changed, 1 insertion(+)
fs/nfs/write.c +1 -0
@@ -1837,6 +1837,7 @@ static void nfs_commit_release_pages(str
 		set_bit(NFS_CONTEXT_RESEND_WRITES, &req->wb_context->flags);
 	next:
 		nfs_unlock_and_release_request(req);
+		cond_resched();
 	}
 	nfss = NFS_SERVER(data->inode);
 	if (atomic_long_read(&nfss->writeback) < NFS_CONGESTION_OFF_THRESH)
maintainer
#31 lore raw
In reply to Linus Torvalds

From: Linus Torvalds <[email protected]>
Date: Tue, 23 Jan 2018 09:42:32 -0800

Quoted 4 lines

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

If a single cpu's softirq deferral can end up running on multiple workqueue threads, indeed that's a serious problem.

So if we're in a workqueue and it does a:

	schedule_work_on(this_cpu, currently_executing_work);

it'll potentially make a new thread?

That's exactly the code path that will get exercised during a UDP flood the way that vector_work_func() is implemented.

author maintainer
#32 lore raw
Quoted 11 lines · David Miller

From: Linus Torvalds <[email protected]>
Date: Tue, 23 Jan 2018 09:42:32 -0800

Quoted 4 lines

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

If a single cpu's softirq deferral can end up running on multiple workqueue threads, indeed that's a serious problem.

Quoted 8 lines

So if we're in a workqueue and it does a:

	schedule_work_on(this_cpu, currently_executing_work);

it'll potentially make a new thread?

That's exactly the code path that will get exercised during a UDP flood the way that vector_work_func() is implemented.

It shouldn't create a new thread unless all other workers in the CPU are voluntarily sleeping while executing a work. Also since softirqs can't sleep, we shouldn't even have two vectors running concurrently on workqueues.

author maintainer
#33 lore raw

2018-01-24 2:57 UTC+01:00, Frederic Weisbecker <[email protected]>:

Quoted 18 lines · David Miller
Quoted 17 lines

From: Linus Torvalds <[email protected]>
Date: Tue, 23 Jan 2018 09:42:32 -0800

Quoted 4 lines

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

If a single cpu's softirq deferral can end up running on multiple workqueue threads, indeed that's a serious problem.

So if we're in a workqueue and it does a:

	schedule_work_on(this_cpu, currently_executing_work);

it'll potentially make a new thread?

Quoted 8 lines
Quoted 2 lines

That's exactly the code path that will get exercised during a UDP flood the way that vector_work_func() is implemented.

It shouldn't create a new thread unless all other workers in the CPU are voluntarily sleeping while executing a work. Also since softirqs can't sleep, we shouldn't even have two vectors running concurrently on workqueues.

...unless I misunderstood workqueue internals or missed something, in which case we may try ordered workqueue.

maintainer
#34 lore raw
In reply to Linus Torvalds
Quoted 10 lines · Linus Torvalds
Quoted 8 lines · Paolo Abeni
Quoted 3 lines

Or is it that the workqueue execution is simply not yielding for some reason?

It's like that.

I spent little time on it, so I haven't many data point. I'll try to investigate the scenario later this week.

Quoted 7 lines

Hmm. workqueues seem to use cond_resched_rcu_qs(), which does a cond_resched() (and a RCU quiescent note).

But I wonder if the test triggers the "lets run lots of workqueue threads", and then the single-threaded user space just gets blown out of the water by many kernel threads. Each thread gets its own "fair" amount of CPU, but..

Niklas suggested a possible relation with CONFIG_IRQ_TIME_ACCOUNTING=y and indeed he was right.

The patched kernel under test had CONFIG_IRQ_TIME_ACCOUNTING set, and very little CPU time was accounted to the kworker:

[2125 is the relevant kworker's pid]
grep sum_exec_runtime /proc/2125/sched; sleep 10; grep sum_exec_runtime /proc/2125/sched

se.sum_exec_runtime                          :         13408.239286
se.sum_exec_runtime                          :         13456.907197

despite such process was processing a lot of packets and basically burning a CPU.

Switching CONFIG_IRQ_TIME_ACCOUNTING off I see the expected behaviour:
top reports that the user space process and kworker share the CPU almost fairly and the user space process is able to receive a reasonable amount of packets.

Paolo

maintainer
#35 lore raw

From: Paolo Abeni <[email protected]>
Date: Wed, 24 Jan 2018 15:54:05 +0100

Quoted 6 lines

Niklas suggested a possible relation with CONFIG_IRQ_TIME_ACCOUNTING=y and indeed he was right.

The patched kernel under test had CONFIG_IRQ_TIME_ACCOUNTING set, and very little CPU time was accounted to the kworker:

Quoted 7 lines

[2125 is the relevant kworker's pid]
grep sum_exec_runtime /proc/2125/sched; sleep 10; grep sum_exec_runtime /proc/2125/sched

se.sum_exec_runtime                          :         13408.239286
se.sum_exec_runtime                          :         13456.907197

despite such process was processing a lot of packets and basically burning a CPU.

So IRQ_TIME_ACCOUNTING makes the scheduler think that the worker threads are using nearly no task time at all.

The existing ksoftirqd code should hit the same problem, right?

maintainer
#36 lore raw
Quoted 14 lines · David Miller

From: Paolo Abeni <[email protected]>
Date: Wed, 24 Jan 2018 15:54:05 +0100

Quoted 11 lines

Niklas suggested a possible relation with CONFIG_IRQ_TIME_ACCOUNTING=y and indeed he was right.

The patched kernel under test had CONFIG_IRQ_TIME_ACCOUNTING set, and very little CPU time was accounted to the kworker:

[2125 is the relevant kworker's pid]
grep sum_exec_runtime /proc/2125/sched; sleep 10; grep sum_exec_runtime /proc/2125/sched

se.sum_exec_runtime                          :         13408.239286
se.sum_exec_runtime                          :         13456.907197
Quoted 5 lines
Quoted 2 lines

despite such process was processing a lot of packets and basically burning a CPU.

So IRQ_TIME_ACCOUNTING makes the scheduler think that the worker threads are using nearly no task time at all.

Yes, this is the behavior I observe in the test. But a quick look at the scheduler code - I'm not very familiar with it - let me think this is not the intended/expected behaviour for the ksoftirqd (and after this series, for the kworker serving the softirq).

Quoted 1 line

The existing ksoftirqd code should hit the same problem, right?

I just tried the vanilla kernel with CONFIG_IRQ_TIME_ACCOUNTING=y, and in the same test scenario, I observe the 'good' behavior: the user space process and ksoftirqd share almost fairly the relevant CPU, and the CPU time spend in softirq processing is accounted to ksoftirqd.

Cheers,

Paolo

#37 lore raw
In reply to Frederic Weisbecker

Em Fri, 19 Jan 2018 16:46:10 +0100
Frederic Weisbecker <[email protected]> escreveu:

Quoted 5 lines

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

No tunable here, so testing should be easier.

Sorry for taking so long to test it. Just returned this week from a 2 weeks travel, and had first to submit media patches for the merge window.

Didn't work. With this patch series, it still lose frames with usb bulk transfers, while recording DVB programs with tvheadend.

Thanks,
Mauro

author maintainer
#38 lore raw
Quoted 7 lines · Mauro Carvalho Chehab

Em Fri, 19 Jan 2018 16:46:10 +0100
Frederic Weisbecker <[email protected]> escreveu:

Quoted 4 lines

As per Linus suggestion, this take doesn't limit the number of occurences per jiffy anymore but instead defers a vector to workqueues as soon as it gets re-enqueued on IRQ tail.

Quoted 7 lines
Quoted 1 line

No tunable here, so testing should be easier.

Sorry for taking so long to test it. Just returned this week from a 2 weeks travel, and had first to submit media patches for the merge window.

Didn't work. With this patch series, it still lose frames with usb bulk transfers, while recording DVB programs with tvheadend.

How can we reproduce this workload?