Re: [PATCH v1 3/6] irqchip/gic-v3: support SGI broadcast

[Marc Zyngier <maz@kernel.org>]

On Fri, 25 Oct 2024 06:07:45 +0100,
Yu Zhao <yuzhao@google.com> wrote:
> 
> Hi Marc,
> 
> On Tue, Oct 22, 2024 at 9:03 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Mon, 21 Oct 2024 05:22:15 +0100,
> > Yu Zhao <yuzhao@google.com> wrote:
> > >
> > > @@ -1407,6 +1418,13 @@ static void gic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
> > >        */
> > >       dsb(ishst);
> > >
> > > +     cpumask_copy(&broadcast, cpu_present_mask);
> >
> > Why cpu_present_mask? I'd expect that cpu_online_mask should be the
> > correct mask to use -- we don't IPI offline CPUs, in general.
> 
> This is exactly because "we don't IPI offline CPUs, in general",
> assuming "we" means the kernel, not GIC.
> 
> My interpretation of what the GIC spec says ("0b1: Interrupts routed
> to all PEs in the system, excluding self") is that it broadcasts IPIs to
> "cpu_present_mask" (minus the local one). So if the kernel uses
> "cpu_online_mask" here, GIC would send IPIs to offline CPUs
> (cpu_present_mask ^ cpu_online_mask), which I don't know whether it's
> a defined behavior.

Offline CPUs are not known to the kernel. Most likely, they are either
powered off, or spending quality time in Secure or Realm mode. Either
way, this is none of our business.

Your approach would make also the kernel perform pretty inconsistently
depending on whether CPUs are offline and not.

> 
> But if you actually meant GIC doesn't IPI offline CPUs, then yes, here
> the kernel should use "cpu_online_mask".
> 
> > > +     cpumask_clear_cpu(smp_processor_id(), &broadcast);
> > > +     if (cpumask_equal(&broadcast, mask)) {
> > > +             gic_broadcast_sgi(d->hwirq);
> > > +             goto done;
> > > +     }
> >
> > So the (valid) case where you would IPI *everyone* is not handled as a
> > fast path? That seems a missed opportunity.
> 
> You are right: it should handle that case.
> 
> > This also seem an like expensive way to do it. How about something
> > like:
> >
> >         int mcnt = cpumask_weight(mask);
> >         int ocnt = cpumask_weight(cpu_online_mask);
> >         if (mcnt == ocnt)  {
> >                 /* Broadcast to all CPUs including self */
> 
> Does the comment mean the following two steps?
> 1. Broadcasting to everyone else.
> 2. Sending to self.

Correct.

> My understanding of the "Interrupt Routing Mode" is that it can't
> broadcast to all CPUs including self, and therefore we need the above
> two steps, which still can be a lot faster. Is my understanding
> correct?

Yes.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.