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

[Sebastian Andrzej Siewior]

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â

> +		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.

> +	else
> +		softirq->pending_work_mask &= ~vec_bit;
> +
> +	lockdep_softirq_exit();
> +	account_irq_exit_time(current);
> +	__local_bh_enable(SOFTIRQ_OFFSET);
> +	local_irq_enable();
> +}

Sebastian