Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] PCI: hv: early activation in hv_pci_vec_irq_domain_activate() skews CPU0 load - [Medium] PCI: hv: cpu_spi_count corruption in hv_pci_arm64_set_affinity() when inactive --
commit 9d112d599453572154cd3fbc90e3b6ea7dedf0e3 Author: Michael Kelley <[email protected]> PCI: hv: Spread load across CPUs when selecting effective affinity CPU This patch extends the arm64 Hyper-V vPCI MSI irq chip to track per-CPU SPI assignments and select the least-loaded CPU from the affinity mask before forwarding the single-CPU effective affinity to the parent GICv3 chip. It also updates the domain activate and deactivate callbacks to increment and decrement the per-CPU SPI counters. > diff --git a/drivers/pci/controller/pci-hyperv.c > b/drivers/pci/controller/pci-hyperv.c > index 824cf27e85e67..7c293e39c9208 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c [ ... ] > +/* > + * Track the count of SPIs assigned to each CPU. Pick a single CPU from the > + * affinity mask based on the SPI loading data, and have the parent IRQ > + * chip (should be "GICv3") set the effective affinity to that single CPU. > + */ > +static int hv_pci_arm64_set_affinity(struct irq_data *d, > + const struct cpumask *mask_val, > + bool force) > +{ > + unsigned int cpu, prev_cpu; > + int ret; > + > + prev_cpu = cpumask_first(irq_data_get_effective_affinity_mask(d)); > + if (prev_cpu < nr_cpu_ids) > + hv_pci_dec_spi_count(d, prev_cpu); > + > + if (force) > + cpu = hv_pci_arm64_get_min_load(d, mask_val); > + else > + cpu = hv_pci_arm64_select_cpu(d, mask_val); > + > + if (cpu >= nr_cpu_ids) > + goto err; > + > + ret = irq_chip_set_affinity_parent(d, cpumask_of(cpu), force); > + if (ret < 0) > + goto err; > + > + hv_pci_inc_spi_count(d, cpu); > + return ret; > + > +err: > + if (prev_cpu < nr_cpu_ids) > + hv_pci_inc_spi_count(d, prev_cpu); > + return -EINVAL; > +} [Severity: Medium] Can hv_pci_arm64_set_affinity() corrupt cpu_spi_count when affinity is changed on a deactivated interrupt? When an allocated MSI or MSI-X interrupt is deactivated (for example, when free_irq() -> __free_irq() calls irq_domain_deactivate_irq() while MSI vectors remain allocated across ndo_stop/ndo_open or queue reconfiguration, or when migrate_one_irq() calls irq_shutdown_and_deactivate()), hv_pci_vec_irq_domain_deactivate() decrements cpu_spi_count for the CPU in effective_affinity and IRQD_ACTIVATED is cleared, but the old CPU remains set in the effective_affinity mask: drivers/pci/controller/pci-hyperv.c:hv_pci_vec_irq_domain_deactivate() { cpu = cpumask_first(irq_data_get_effective_affinity_mask(irqd)); if (cpu < nr_cpu_ids) hv_pci_dec_spi_count(irqd, cpu); } Because hv_pci_vec_irq_domain_alloc() does not set IRQD_AFFINITY_ON_ACTIVATE (unlike its_irq_domain_alloc() in drivers/irqchip/irq-gic-v3-its.c), irq_set_affinity_deactivated() does not defer affinity changes on a deactivated interrupt: kernel/irq/manage.c:irq_set_affinity_deactivated() { if (!IS_ENABLED(CONFIG_IRQ_DOMAIN_HIERARCHY) || irqd_is_activated(data) || !irqd_affinity_on_activate(data)) return false; ... } If irq_set_affinity() or irq_force_affinity() is called while the interrupt is deactivated (for example, via /proc/irq/<irq>/smp_affinity, irq_set_affinity_hint(), or irq_restore_affinity_of_irq()), irq_set_affinity_locked() -> irq_do_set_affinity() invokes hv_pci_arm64_set_affinity(). That reads the stale prev_cpu from irq_data_get_effective_affinity_mask(d), decrements prev_cpu's cpu_spi_count a second time, and increments the newly selected cpu's cpu_spi_count while IRQD_ACTIVATED remains false. If the vector is later freed via pci_free_irq_vectors() -> __msi_domain_free_irqs() in kernel/irq/msi.c, irq_domain_deactivate_irq() is skipped because !irqd_is_activated(irqd), leaking the count on the new CPU; or if the vector is re-activated via request_irq() -> irq_activate() -> hv_pci_vec_irq_domain_activate(), effective_affinity is overwritten with cpumask_first(cpu_online_mask) without decrementing the CPU selected while deactivated. Should hv_pci_vec_irq_domain_alloc() set IRQD_AFFINITY_ON_ACTIVATE, or should hv_pci_vec_irq_domain_deactivate() clear effective_affinity? [ ... ] > @@ -885,21 +1007,39 @@ static int hv_pci_vec_irq_domain_alloc(struct > irq_domain *domain, > * composing MSI from the hypervisor. GIC will eventually set the right > * affinity for the irq and the 'unmask' will retarget the interrupt to that > * cpu. > + * > + * The first cpu is always used for initial activation because spreading the > + * initial CPU would have to be done without information about affinity > groups > + * that may exist. The resulting assignments would pollute the "least loaded" > + * data for making the final assignments. Everything would still work, but > + * the spreading would be less even than it could be. > */ > static int hv_pci_vec_irq_domain_activate(struct irq_domain *domain, > struct irq_data *irqd, bool reserve) > { > - int cpu = cpumask_first(cpu_present_mask); > + int cpu = cpumask_first(cpu_online_mask); > > + hv_pci_inc_spi_count(irqd, cpu); > irq_data_update_effective_affinity(irqd, cpumask_of(cpu)); > > return 0; > } [Severity: Medium] Won't incrementing cpu_spi_count in hv_pci_vec_irq_domain_activate() skew the load calculation away from the first online CPU during multi-vector MSI/MSI-X setup? Because pci_msi_template and pci_msix_template in drivers/pci/msi/irqdomain.c set MSI_FLAG_ACTIVATE_EARLY, allocating N vectors via pci_alloc_irq_vectors() or pci_alloc_irq_vectors_affinity() causes __msi_domain_alloc_irqs() to activate all N vectors in a batch via msi_init_virq() -> irq_domain_activate_irq() -> hv_pci_vec_irq_domain_activate() before returning and before request_irq() is called on any vector: kernel/irq/msi.c:__msi_domain_alloc_irqs() { ... for (i = 0; i < desc->nvec_used; i++) { irq_set_msi_desc_off(virq, i, desc); irq_debugfs_copy_devname(virq + i, dev); ret = msi_init_virq(domain, virq + i, vflags); if (ret) return ret; } } This increments cpu_spi_count on cpumask_first(cpu_online_mask) (typically CPU 0) for all N vectors up front, inflating CPU 0's count by N. When the driver then calls request_irq() for each vector i = 0..N-1 in sequence (such as in nvme_setup_io_queues()), irq_startup() -> irq_do_set_affinity() -> hv_pci_arm64_set_affinity() decrements CPU 0's cpu_spi_count by only 1 for vector i, leaving CPU 0's count inflated by N - 1 - i from the remaining unstarted vectors of the same device still parked on CPU 0. When hv_pci_arm64_select_cpu() selects a CPU for the first queue (whose affinity mask includes CPU 0), it sees CPU 0's load inflated by N - 1 and avoids assigning interrupts to CPU 0 for the first N - 1 devices probed. Furthermore, if a driver allocates M vectors and only calls request_irq() on K < M of them (or defers request_irq() until queues or network ports are opened), the M - K unrequested vectors remain activated on CPU 0 and keep its cpu_spi_count inflated by M - K. Could hv_pci_vec_irq_domain_activate() leave effective_affinity empty and avoid incrementing cpu_spi_count until hv_pci_arm64_set_affinity() selects the actual target CPU? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
