Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221003232033.3404802-1-jstultz@google.com> <20221003232033.3404802-3-jstultz@google.com>
 <Y01NPB4sa8Z98ntK@li-4a3a4a4c-28e5-11b2-a85c-a8d192c6f089.ibm.com> <CANDhNCoTGNANDnOQ6touhreG_UEn1-N4T4BktWFxpLPSWVfrdA@mail.gmail.com>
In-Reply-To: <CANDhNCoTGNANDnOQ6touhreG_UEn1-N4T4BktWFxpLPSWVfrdA@mail.gmail.com>
From:   John Stultz <jstultz@google.com>
Date:   Mon, 17 Oct 2022 22:32:16 -0700
Message-ID: <CANDhNCrw=-p1kNAog+m6ipM1j5QMdBCibU5_p2M75oqhfHWnvw@mail.gmail.com>
Subject: Re: [PATCH RFC v4 2/3] sched: Avoid placing RT threads on cores
 handling long softirqs
To:     Alexander Gordeev <agordeev@linux.ibm.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "Connor O'Brien" <connoro@google.com>,
        John Dias <joaodias@google.com>, Rick Yiu <rickyiu@google.com>,
        John Kacur <jkacur@redhat.com>,
        Qais Yousef <qais.yousef@arm.com>,
        Chris Redpath <chris.redpath@arm.com>,
        Abhijeet Dharmapurikar <adharmap@quicinc.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>, kernel-team@android.com,
        "J . Avila" <elavila@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 17, 2022 at 8:42 PM John Stultz <jstultz@google.com> wrote:
>  On Mon, Oct 17, 2022 at 5:40 AM Alexander Gordeev
> <agordeev@linux.ibm.com> wrote:
> > On Mon, Oct 03, 2022 at 11:20:32PM +0000, John Stultz wrote:
> > > +               preempt_count() & SOFTIRQ_MASK);
> >
> > Could you please clarify this whole check in more detail?
> >
> > What is the point in checking if a remote CPU is handling
> > a "long" softirq while the local one is handling any softirq?
>
> Good question! This looks like an error from my rework of the earlier
> version of the patch.
> In v1 it was:
>    task_thread_info(curr)->preempt_count & SOFTIRQ_MASK));
> And it looks like I erroneously condensed that to preempt_count() &
> SOFTIRQ_MASK  treating curr (from the target cpu rq) as if it were
> current. :P
>
> I'll fix this. Thank you for catching it!

Ah, it's not so trivial to fix as I see part of my motivation for
condensing it was task_thread_info(curr)->preempt_count doesn't build
on x86 (which uses percpu values instead of threadinfo).

So I'll have to think a bit more about how to do this generically, and
if the conditionalization can be otherwise simplified.

Thanks again for pointing the issue out!
-john