Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 25 Sep 2020 02:42:08 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     qianjun.kernel@gmail.com, peterz@infradead.org, will@kernel.org,
        luto@kernel.org, linux-kernel@vger.kernel.org,
        laoar.shao@gmail.com, qais.yousef@arm.com, urezki@gmail.com
Subject: Re: [PATCH V7 4/4] softirq: Allow early break the softirq processing
 loop
Message-ID: <20200925004207.GE19346@lenoir>
References: <20200915115609.85106-1-qianjun.kernel@gmail.com>
 <20200915115609.85106-5-qianjun.kernel@gmail.com>
 <878scz89tl.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <878scz89tl.fsf@nanos.tec.linutronix.de>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Sep 24, 2020 at 05:37:42PM +0200, Thomas Gleixner wrote:
> Subject: softirq; Prevent starvation of higher softirq vectors
[...]
> +	/*
> +	 * Word swap pending to move the not yet handled bits of the previous
> +	 * run first and then clear the duplicates in the newly raised ones.
> +	 */
> +	swahw32s(&cur_pending);
> +	pending = cur_pending & ~(cur_pending << SIRQ_PREV_SHIFT);
> +
>  	for_each_set_bit(vec_nr, &pending, NR_SOFTIRQS) {
>  		int prev_count;
>  
> +		vec_nr &= SIRQ_VECTOR_MASK;

Shouldn't NR_SOFTIRQS above protect from that?

>  		__clear_bit(vec_nr, &pending);
>  		kstat_incr_softirqs_this_cpu(vec_nr);
>
[...]
> +	} else {
> +		/*
> +		 * Retain the unprocessed bits and swap @cur_pending back
> +		 * into normal ordering
> +		 */
> +		cur_pending = (u32)pending;
> +		swahw32s(&cur_pending);
> +		/*
> +		 * If the previous bits are done move the low word of
> +		 * @pending into the high word so it's processed first.
> +		 */
> +		if (!(cur_pending & SIRQ_PREV_MASK))
> +			cur_pending <<= SIRQ_PREV_SHIFT;

If the previous bits are done and there is no timeout, should
we consider to restart a loop?

A common case would be to enter do_softirq() with RCU_SOFTIRQ set
in the SIRQ_PREV_MASK and NET_RX_SOFTIRQ set in the normal mask.

You would always end up processing the RCU_SOFTIRQ here and trigger
ksoftirqd for the NET_RX_SOFTIRQ.

Although that's probably no big deal as we should be already in ksoftirqd
if we processed prev bits. We are just going to iterate the kthread loop
instead of the do_softirq loop. Probably no real issue then...


>  
> +		/* Merge the newly pending ones into the low word */
> +		cur_pending |= new_pending;
> +	}
> +	set_softirq_pending(cur_pending);
>  	wakeup_softirqd();
>  out:
>  	lockdep_softirq_end(in_hardirq);