Re: [RFC PATCH 00/30] softirq: Make softirqs soft-interruptible (+ per vector disablement)

[Frederic Weisbecker <frederic@kernel.org>]

On Tue, Oct 16, 2018 at 04:03:59PM -0600, Jonathan Corbet wrote:
> On Thu, 11 Oct 2018 01:11:47 +0200
> Frederic Weisbecker <frederic@kernel.org> wrote:
> 
> > 945 files changed, 13857 insertions(+), 9767 deletions(-)
> 
> Impressive :)

In the wrong way :)

> 
> I have to ask a dumb question, though.  Might it not be better to add a
> new set of functions like:
> 
> 	local_softirq_disable(mask);
> 	spin_lock_softirq(lock, mask);
> 
> Then just define the existing functions to call the new ones with
> SOFTIRQ_ALL_MASK?  It would achieve something like the same result with
> far less churn and conflict potential; then individual call sites could be
> changed at leisure?  For extra credit, somebody could do a checkpatch rule
> to keep new calls to the _bh functions from being added.

So it's not a dumb question at all. That's in fact the core of the suggestions
I got while discussing that lately on IRC with the initial Cc list.

That's definetly the direction I'll take on v2: keeping the current API,
introduce new ones with per vector granularity and convert iteratively.
The diffstat will shrink tremendously and it can make the change
eventually maintainable.

The reason why I didn't take that approach first in this version is because
of a little technical detail:

    _ Serving softirqs is done under SOFTIRQ_OFFSET: (1 << SOFTIRQ_SHIFT)

    _ Disabling softirqs is done under SOFTIRQ_OFFSET * 2

We do that to differentiate both state. Serving softirqs can't nest whereas disabling
softirqs can nest. So we just need to check the value is odd to identify a serving
softirq state.

Now things are going to change as serving softirqs will be able to nest too.
And having that bh saved state allowed me to make softirqs disablement not
nesting. So I just needed to invert both ways to account. I wanted to do that
because otherwise we need to share SOFTIRQ_MASK for two counters, which makes
a maximum of 16 for both. That's enough for serving softirqs as it's couldn't
nest further than NR_SOFTIRQS, but disabling softirqs depth was unpredictable,
even though 16 levels is already insane, anyway...

There is an easy alternative though:

    local_bh_enter()
    {
        bool nesting = false;

        if (preempt_count() & SOFTIRQ_OFFSET)
            nesting = true;
        else
            preempt_count() |= SOFTIRQ_OFFSET;

        return nesting;
    }

    local_bh_exit(bool nesting)
    {
        if (nesting)
            preempt_count() &= ~SOFTIRQ_OFFSET;
    }

    do_softirq()
    {
        bool nesting = local_bh_enter();

        // process softirqs ....

        local_bh_exit(nesting);
    }

But I guess it was just too obvious for me to be considered :-S