From: Thomas Gleixner <tglx@linutronix.de>
To: Costa Shulyupin <costa.shul@redhat.com>,
longman@redhat.com, pauld@redhat.com, juri.lelli@redhat.com,
prarit@redhat.com, vschneid@redhat.com,
Anna-Maria Behnsen <anna-maria@linutronix.de>,
Frederic Weisbecker <frederic@kernel.org>,
Zefan Li <lizefan.x@bytedance.com>, Tejun Heo <tj@kernel.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Petr Mladek <pmladek@suse.com>,
Andrew Morton <akpm@linux-foundation.org>,
Masahiro Yamada <masahiroy@kernel.org>,
Randy Dunlap <rdunlap@infradead.org>,
Yoann Congal <yoann.congal@smile.fr>,
"Gustavo A. R. Silva" <gustavoars@kernel.org>,
Nhat Pham <nphamcs@gmail.com>,
Costa Shulyupin <costa.shul@redhat.com>,
linux-kernel@vger.kernel.org, cgroups@vger.kernel.org
Subject: Re: [PATCH v1 4/7] sched/isolation: Adjust affinity of managed irqs according to change of housekeeping cpumask
Date: Sat, 18 May 2024 03:17:24 +0200 [thread overview]
Message-ID: <87wmnrj4uz.ffs@tglx> (raw)
In-Reply-To: <20240516190437.3545310-5-costa.shul@redhat.com>
On Thu, May 16 2024 at 22:04, Costa Shulyupin wrote:
> irq_affinity_adjust() is prototyped from irq_affinity_online_cpu()
> and irq_restore_affinity_of_irq().
I'm used to this prototyped phrase by now. It still does not justify to
expose me to this POC hackery.
My previous comments about change logs still apply.
> +static int irq_affinity_adjust(cpumask_var_t disable_mask)
> +{
> + unsigned int irq;
> + cpumask_var_t mask;
> +
> + if (!alloc_cpumask_var(&mask, GFP_KERNEL))
> + return -ENOMEM;
> +
> + irq_lock_sparse();
> + for_each_active_irq(irq) {
> + struct irq_desc *desc = irq_to_desc(irq);
> +
> + raw_spin_lock_irq(&desc->lock);
That's simply broken. This is not CPU hotplug on an outgoing CPU. Why
are you assuming that your isolation change code can rely on the
implicit guarantees of CPU hot(un)plug?
Also there is a reason why interrupt related code is in kernel/irq/* and
not in some random other location. Even if C allows you to fiddle with
everything that does not mean that hiding random hacks in other places
is correct in any way.
> + struct irq_data *data = irq_desc_get_irq_data(desc);
> +
> + if (irqd_affinity_is_managed(data) && cpumask_weight_and(disable_mask,
> + irq_data_get_affinity_mask(data))) {
Interrupt target isolation is only relevant for managed interrupts and
non-managed interrupts clearly are going to migrate themself away
magically, right?
> +
> + cpumask_and(mask, cpu_online_mask, irq_default_affinity);
> + cpumask_and(mask, mask, housekeeping_cpumask(HK_TYPE_MANAGED_IRQ));
There are clearly a lot of comments explaining what this is doing and
why it is correct as there is a guarantee that these masks overlap by
definition.
> + irq_set_affinity_locked(data, mask, true);
Plus the extensive explanation why using 'force=true' is even remotely
correct here.
I conceed that the documentation of that function and its arguments is
close to non-existant, but if you follow the call chain of that function
there are enough hints down the road, no?
> + WARN_ON(cpumask_weight_and(irq_data_get_effective_affinity_mask(data),
> + disable_mask));
> + WARN_ON(!cpumask_subset(irq_data_get_effective_affinity_mask(data),
> + cpu_online_mask));
> + WARN_ON(!cpumask_subset(irq_data_get_effective_affinity_mask(data),
> + housekeeping_cpumask(HK_TYPE_MANAGED_IRQ)));
These warnings are required and useful within the spinlock held and
interrupt disabled section because of what?
- Because the resulting stack trace provides a well known call chain
- Because the resulting warnings do not tell anything about the
affected interrupt number
- Because the resulting warnings do not tell anything about the CPU
masks which cause the problem
- Because the aggregate information of the above is utterly useless
Impressive...
Thanks,
tglx
next prev parent reply other threads:[~2024-05-18 1:17 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 19:04 [PATCH v1 0/7] sched: Adjust affinity according to change of housekeeping cpumask Costa Shulyupin
2024-05-16 19:04 ` [PATCH v1 1/7] sched/isolation: Add infrastructure to adjust affinity for dynamic CPU isolation Costa Shulyupin
2024-05-17 21:37 ` Thomas Gleixner
2024-05-16 19:04 ` [PATCH v1 2/7] sched/isolation: Adjust affinity of timers according to change of housekeeping cpumask Costa Shulyupin
2024-05-17 22:39 ` Thomas Gleixner
2024-05-17 22:52 ` Thomas Gleixner
2024-05-16 19:04 ` [PATCH v1 3/7] sched/isolation: Adjust affinity of hrtimers " Costa Shulyupin
2024-05-17 23:42 ` Thomas Gleixner
2024-05-16 19:04 ` [PATCH v1 4/7] sched/isolation: Adjust affinity of managed irqs " Costa Shulyupin
2024-05-18 1:17 ` Thomas Gleixner [this message]
2024-05-18 1:25 ` Thomas Gleixner
2024-05-16 19:04 ` [PATCH v1 5/7] [NOT-FOR-MERGE] test timers affinity adjustment Costa Shulyupin
2024-05-16 19:04 ` [PATCH v1 6/7] [NOT-FOR-MERGE] test timers and hrtimers " Costa Shulyupin
2024-05-16 19:04 ` [PATCH v1 7/7] [NOT-FOR-MERGE] test managed irqs " Costa Shulyupin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87wmnrj4uz.ffs@tglx \
--to=tglx@linutronix.de \
--cc=akpm@linux-foundation.org \
--cc=anna-maria@linutronix.de \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=cgroups@vger.kernel.org \
--cc=costa.shul@redhat.com \
--cc=dietmar.eggemann@arm.com \
--cc=frederic@kernel.org \
--cc=gustavoars@kernel.org \
--cc=hannes@cmpxchg.org \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizefan.x@bytedance.com \
--cc=longman@redhat.com \
--cc=masahiroy@kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=nphamcs@gmail.com \
--cc=pauld@redhat.com \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=prarit@redhat.com \
--cc=rdunlap@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tj@kernel.org \
--cc=vincent.guittot@linaro.org \
--cc=vschneid@redhat.com \
--cc=yoann.congal@smile.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).