| Message ID | 20210720232624.1493424-1-nitesh@redhat.com |
|---|---|
| Headers | show |
| Series | genirq: Cleanup the abuse of irq_set_affinity_hint() | expand |
On 7/20/2021 4:26 PM, Nitesh Narayan Lal wrote: > The driver uses irq_set_affinity_hint to set the affinity for the lpfc > interrupts to a mask corresponding to the local NUMA node to avoid > performance overhead on AMD architectures. > > However, irq_set_affinity_hint() setting the affinity is an undocumented > side effect that this function also sets the affinity under the hood. > To remove this side effect irq_set_affinity_hint() has been marked as > deprecated and new interfaces have been introduced. > > Also, as per the commit dcaa21367938 ("scsi: lpfc: Change default IRQ model > on AMD architectures"): > "On AMD architecture, revert the irq allocation to the normal style > (non-managed) and then use irq_set_affinity_hint() to set the cpu affinity > and disable user-space rebalancing." > we don't really need to set the affinity_hint as user-space rebalancing for > the lpfc interrupts is not desired. > > Hence, replace the irq_set_affinity_hint() with irq_set_affinity() which > only applies the affinity for the interrupts. > > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > --- > drivers/scsi/lpfc/lpfc_init.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > Looks good. Thanks Reviewed-by: James Smart <jsmart2021@gmail.com> -- james
Nitesh, > Gentle ping. > Any comments on the following patches: > > scsi: megaraid_sas: Use irq_set_affinity_and_hint > scsi: mpt3sas: Use irq_set_affinity_and_hint Sumit and Sreekanth: Please review. Thanks! -- Martin K. Petersen Oracle Linux Engineering
On Wed, Jul 21, 2021 at 4:57 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > The driver uses irq_set_affinity_hint() specifically for the high IOPS > queue interrupts for two purposes: > > - To set the affinity_hint which is consumed by the userspace for > distributing the interrupts > > - To apply an affinity that it provides > > The driver enforces its own affinity to bind the high IOPS queue interrupts > to the local NUMA node. However, irq_set_affinity_hint() applying the > provided cpumask as an affinity (if not NULL) for the interrupt is an > undocumented side effect. > > To remove this side effect irq_set_affinity_hint() has been marked > as deprecated and new interfaces have been introduced. Hence, replace the > irq_set_affinity_hint() with the new interface irq_set_affinity_and_hint() > where the provided mask needs to be applied as the affinity and > affinity_hint pointer needs to be set and replace with > irq_update_affinity_hint() where only affinity_hint needs to be updated. > Changes looks good and also verified that the high iops queue's IRQs are affinitied to local numa node. Reviewed-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com> > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > --- > drivers/scsi/mpt3sas/mpt3sas_base.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c > index c39955239d1c..c1a11962f227 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_base.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c > @@ -2991,6 +2991,7 @@ _base_check_enable_msix(struct MPT3SAS_ADAPTER *ioc) > static void > _base_free_irq(struct MPT3SAS_ADAPTER *ioc) > { > + unsigned int irq; > struct adapter_reply_queue *reply_q, *next; > > if (list_empty(&ioc->reply_queue_list)) > @@ -2998,9 +2999,10 @@ _base_free_irq(struct MPT3SAS_ADAPTER *ioc) > > list_for_each_entry_safe(reply_q, next, &ioc->reply_queue_list, list) { > list_del(&reply_q->list); > - if (ioc->smp_affinity_enable) > - irq_set_affinity_hint(pci_irq_vector(ioc->pdev, > - reply_q->msix_index), NULL); > + if (ioc->smp_affinity_enable) { > + irq = pci_irq_vector(ioc->pdev, reply_q->msix_index); > + irq_update_affinity_hint(irq, NULL); > + } > free_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index), > reply_q); > kfree(reply_q); > @@ -3056,16 +3058,13 @@ _base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index) > * @ioc: per adapter object > * > * The enduser would need to set the affinity via /proc/irq/#/smp_affinity > - * > - * It would nice if we could call irq_set_affinity, however it is not > - * an exported symbol > */ > static void > _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc) > { > - unsigned int cpu, nr_cpus, nr_msix, index = 0; > + unsigned int cpu, nr_cpus, nr_msix, index = 0, irq; > struct adapter_reply_queue *reply_q; > - int local_numa_node; > + const struct cpumask *mask; > > if (!_base_is_controller_msix_enabled(ioc)) > return; > @@ -3088,11 +3087,11 @@ _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc) > * corresponding to high iops queues. > */ > if (ioc->high_iops_queues) { > - local_numa_node = dev_to_node(&ioc->pdev->dev); > + mask = cpumask_of_node(dev_to_node(&ioc->pdev->dev)); > for (index = 0; index < ioc->high_iops_queues; > index++) { > - irq_set_affinity_hint(pci_irq_vector(ioc->pdev, > - index), cpumask_of_node(local_numa_node)); > + irq = pci_irq_vector(ioc->pdev, index); > + irq_set_affinity_and_hint(irq, mask); > } > } > > -- > 2.27.0 >
On Wed, Jul 21, 2021 at 7:26 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > From: Thomas Gleixner <tglx@linutronix.de> > > The discussion about removing the side effect of irq_set_affinity_hint() of > actually applying the cpumask (if not NULL) as affinity to the interrupt, > unearthed a few unpleasantries: > > 1) The modular perf drivers rely on the current behaviour for the very > wrong reasons. > > 2) While none of the other drivers prevents user space from changing > the affinity, a cursorily inspection shows that there are at least > expectations in some drivers. > > #1 needs to be cleaned up anyway, so that's not a problem > > #2 might result in subtle regressions especially when irqbalanced (which > nowadays ignores the affinity hint) is disabled. > > Provide new interfaces: > > irq_update_affinity_hint() - Only sets the affinity hint pointer > irq_set_affinity_and_hint() - Set the pointer and apply the affinity to > the interrupt > > Make irq_set_affinity_hint() a wrapper around irq_apply_affinity_hint() and > document it to be phased out. > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > Link: https://lore.kernel.org/r/20210501021832.743094-1-jesse.brandeburg@intel.com Reviewed-by: Ming Lei <ming.lei@redhat.com>
On Mon, Aug 16, 2021 at 11:50 AM Nitesh Lal <nilal@redhat.com> wrote: > > On Mon, Aug 2, 2021 at 11:26 AM Nitesh Lal <nilal@redhat.com> wrote: > > > > On Tue, Jul 20, 2021 at 7:26 PM Nitesh Narayan Lal <nitesh@redhat.com> wrote: > > > > > > The drivers currently rely on irq_set_affinity_hint() to either set the > > > affinity_hint that is consumed by the userspace and/or to enforce a custom > > > affinity. > > > > [...] > > Any comments on the following patches: > > enic: Use irq_update_affinity_hint > be2net: Use irq_update_affinity_hint > mailbox: Use irq_update_affinity_hint > hinic: Use irq_set_affinity_and_hint > > or any other patches? > Any help in testing will also be very useful. > Gentle ping. Any comments on the following patches: be2net: Use irq_update_affinity_hint hinic: Use irq_set_affinity_and_hint or any other patches? -- Thanks Nitesh
The drivers currently rely on irq_set_affinity_hint() to either set the affinity_hint that is consumed by the userspace and/or to enforce a custom affinity. irq_set_affinity_hint() as the name suggests is originally introduced to only set the affinity_hint to help the userspace in guiding the interrupts and not the affinity itself. However, since the commit e2e64a932556 "genirq: Set initial affinity in irq_set_affinity_hint()" irq_set_affinity_hint() also started applying the provided cpumask (if not NULL) as the affinity for the interrupts. The issue that this commit was trying to solve is to allow the drivers to enforce their affinity mask to distribute the interrupts across the CPUs such that they don't always end up on CPU0. This issue has been resolved within the irq subsystem since the commit a0c9259dc4e1 "irq/matrix: Spread interrupts on allocation" Hence, there is no need for the drivers to overwrite the affinity to spread as it is dynamically performed at the time of allocation. Also, irq_set_affinity_hint() setting affinity unconditionally introduces issues for the drivers that only want to set their affinity_hint and not the affinity itself as for these driver interrupts the default_smp_affinity_mask is completely ignored (for detailed investigation please refer to [1]). Unfortunately reverting the commit e2e64a932556 is not an option at this point for two reasons [2]: - Several drivers for a valid reason (performance) rely on this API to enforce their affinity mask - Until very recently this was the only exported interface that was available To clear this out Thomas has come up with the following interfaces: - irq_set_affinity(): only sets affinity of an IRQ [3] - irq_update_affinity_hint(): Only sets the hint [4] - irq_set_affinity_and_hint(): Sets both affinity and the hint mask [4] The first API is already merged in the linus's tree and the patch that introduces the other two interfaces is included with this patch-set. To move to the stage where we can safely get rid of the irq_set_affinity_hint(), which has been marked deprecated, we have to move all its consumers to these new interfaces. In this patch-set, I have done that for a few drivers and will hopefully try to move the remaining of them in the coming days. Testing ------- In terms of testing, I have performed some basic testing on x86 to verify things such as the interrupts are evenly spread on all CPUs, hint mask is correctly set etc. for the drivers - i40e, iavf, mlx5, mlx4, ixgbe, and enic on top of: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git So more testing is probably required for these and the drivers that I didn't test and any help will be much appreciated. Notes ----- - For the mpt3sas driver I decided to go with the usage of irq_set_affinity_and_hint over irq_set_affinity based on my analysis of it and the megaraid driver. However, if we are sure that it is not required then I can replace it with just irq_set_affinity as one of its comment suggests. Change from v4 [5] ------------------ - Fixed kernel-doc warnings (Jakub Kicinski) - Renamed the cover to "Cleanup the abuse of irq_set_affinity_hint()", what Thomas originally used as this series is an extension of the work that he started and proposed [6]. Change from v3 [7] ------------------ - Replaced irq_set_affinity_and_hint with irq_update_affinity_hint in irdma (Leon Romanovsky) - rebased the patches on top of 5.14-rc2 Change from v2 [8] ------------------ - Rebased on top of 5.14-rc1 (Leon Romanovsky) + After discussion with Leon [9], made changes in the mlx5 patch to use irq_set_affinity_and_hint over irq_update_affinity_hint + i40iw is replaced with irdma driver, hence made the respective changes in irdma (also replcaed irq_update_affinity_hint with irq_set_affinity_and_hint). Change from v1 [10] ------------------ - Fixed compilation error by adding the new interface definitions for cases where CONFIG_SMP is not defined - Fixed function usage in megaraid_sas and removed unnecessary variable (Robin Murphy) - Removed unwanted #if/endif from mlx4 (Leon Romanovsky) - Other indentation related fixes [1] https://lore.kernel.org/lkml/1a044a14-0884-eedb-5d30-28b4bec24b23@redhat.com/ [2] https://lore.kernel.org/linux-pci/d1d5e797-49ee-4968-88c6-c07119343492@arm.com/ [3] https://lore.kernel.org/linux-arm-kernel/20210518091725.046774792@linutronix.de/ [4] https://lore.kernel.org/patchwork/patch/1434326/ [5] https://lore.kernel.org/lkml/20210719180746.1008665-1-nitesh@redhat.com/ [6] https://lore.kernel.org/linux-arm-kernel/20210518091725.046774792@linutronix.de/ [7] https://lore.kernel.org/linux-scsi/20210713211502.464259-1-nitesh@redhat.com/ [8] https://lore.kernel.org/lkml/20210629152746.2953364-1-nitesh@redhat.com/ [9] https://lore.kernel.org/lkml/YO0eKv2GJcADQTHH@unreal/ [10] https://lore.kernel.org/linux-scsi/20210617182242.8637-1-nitesh@redhat.com/ Nitesh Narayan Lal (13): iavf: Use irq_update_affinity_hint i40e: Use irq_update_affinity_hint scsi: megaraid_sas: Use irq_set_affinity_and_hint scsi: mpt3sas: Use irq_set_affinity_and_hint RDMA/irdma: Use irq_update_affinity_hint enic: Use irq_update_affinity_hint be2net: Use irq_update_affinity_hint ixgbe: Use irq_update_affinity_hint mailbox: Use irq_update_affinity_hint scsi: lpfc: Use irq_set_affinity hinic: Use irq_set_affinity_and_hint net/mlx5: Use irq_set_affinity_and_hint net/mlx4: Use irq_update_affinity_hint Thomas Gleixner (1): genirq: Provide new interfaces for affinity hints drivers/infiniband/hw/irdma/hw.c | 4 +- drivers/mailbox/bcm-flexrm-mailbox.c | 4 +- drivers/net/ethernet/cisco/enic/enic_main.c | 8 +-- drivers/net/ethernet/emulex/benet/be_main.c | 4 +- drivers/net/ethernet/huawei/hinic/hinic_rx.c | 4 +- drivers/net/ethernet/intel/i40e/i40e_main.c | 8 +-- drivers/net/ethernet/intel/iavf/iavf_main.c | 8 +-- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 ++-- drivers/net/ethernet/mellanox/mlx4/eq.c | 8 ++- .../net/ethernet/mellanox/mlx5/core/pci_irq.c | 8 +-- drivers/scsi/lpfc/lpfc_init.c | 4 +- drivers/scsi/megaraid/megaraid_sas_base.c | 27 +++++----- drivers/scsi/mpt3sas/mpt3sas_base.c | 21 ++++---- include/linux/interrupt.h | 53 ++++++++++++++++++- kernel/irq/manage.c | 8 +-- 15 files changed, 114 insertions(+), 65 deletions(-) --