* Re: [Intel-wired-lan] [EXTERNAL] [PATCH iwl-next] ice: use irq_update_affinity_hint()
2024-05-22 23:12 [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint() Michal Schmidt
@ 2024-05-23 6:33 ` Sunil Kovvuri Goutham
2024-05-23 17:53 ` [Intel-wired-lan] " Jacob Keller
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Sunil Kovvuri Goutham @ 2024-05-23 6:33 UTC (permalink / raw)
To: Michal Schmidt, Jesse Brandeburg, Tony Nguyen,
intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Thomas Gleixner, Nitesh Narayan Lal
> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Thursday, May 23, 2024 4:43 AM
> To: Jesse Brandeburg <jesse.brandeburg@intel.com>; Tony Nguyen
> <anthony.l.nguyen@intel.com>; intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Nitesh Narayan Lal <nitesh@redhat.com>;
> Thomas Gleixner <tglx@linutronix.de>
> Subject: [EXTERNAL] [PATCH iwl-next] ice: use irq_update_affinity_hint()
>
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
>
> ----------------------------------------------------------------------
> irq_set_affinity_hint() is deprecated. Use irq_update_affinity_hint() instead.
> This removes the side-effect of actually applying the affinity.
>
> The driver does not really need to worry about spreading its IRQs across CPUs.
> The core code already takes care of that.
> On the contrary, when the driver applies affinities by itself, it breaks the users'
> expectations:
> 1. The user configures irqbalance with IRQBALANCE_BANNED_CPULIST in
> order to prevent IRQs from being moved to certain CPUs that run a
> real-time workload.
> 2. ice reconfigures VSIs at runtime due to a MIB change
> (ice_dcb_process_lldp_set_mib_change). Reopening a VSI resets the
> affinity in ice_vsi_req_irq_msix().
> 3. ice has no idea about irqbalance's config, so it may move an IRQ to
> a banned CPU. The real-time workload suffers unacceptable latency.
>
> I am not sure if updating the affinity hints is at all useful, because irqbalance
> ignores them since 2016 ([1]), but at least it's harmless.
>
> This ice change is similar to i40e commit d34c54d1739c ("i40e: Use
> irq_update_affinity_hint()").
>
> [1] https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_Irqbalance_irqbalance_commit_dcc411e7bfdd&d=DwIDAg&
> c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTzHuhwawxR1P9_t
> MCN2FODU4&m=8_ejYsgRhUX90iDcRMQE2cUmtBDDCjGE8LVB-
> vhUlW0MMr-ZoqFeyCnI5qTQrKxf&s=r5rBcGd0wa-
> lQkDZdyzZwBeBviSd8DZpgYh028enkU8&e=
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++--
> drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 5371e91f6bbb..0f8b622db2b5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2587,8 +2587,8 @@ void ice_vsi_free_irq(struct ice_vsi *vsi)
> if (!IS_ENABLED(CONFIG_RFS_ACCEL))
> irq_set_affinity_notifier(irq_num, NULL);
>
> - /* clear the affinity_mask in the IRQ descriptor */
> - irq_set_affinity_hint(irq_num, NULL);
> + /* clear the affinity_hint in the IRQ descriptor */
> + irq_update_affinity_hint(irq_num, NULL);
> synchronize_irq(irq_num);
> devm_free_irq(ice_pf_to_dev(pf), irq_num, vsi-
> >q_vectors[i]);
> }
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index f60c022f7960..a5d369b8fed5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2607,7 +2607,7 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi,
> char *basename)
> }
>
> /* assign the mask for this irq */
> - irq_set_affinity_hint(irq_num, &q_vector->affinity_mask);
> + irq_update_affinity_hint(irq_num, &q_vector->affinity_mask);
> }
>
> err = ice_set_cpu_rx_rmap(vsi);
> @@ -2625,7 +2625,7 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi,
> char *basename)
> irq_num = vsi->q_vectors[vector]->irq.virq;
> if (!IS_ENABLED(CONFIG_RFS_ACCEL))
> irq_set_affinity_notifier(irq_num, NULL);
> - irq_set_affinity_hint(irq_num, NULL);
> + irq_update_affinity_hint(irq_num, NULL);
> devm_free_irq(dev, irq_num, &vsi->q_vectors[vector]);
> }
> return err;
> --
> 2.44.0
>
LGTM
Reviewed-by: Sunil Goutham <sgoutham@marvell.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint()
2024-05-22 23:12 [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint() Michal Schmidt
2024-05-23 6:33 ` [Intel-wired-lan] [EXTERNAL] " Sunil Kovvuri Goutham
@ 2024-05-23 17:53 ` Jacob Keller
2024-05-24 21:04 ` Jacob Keller
2024-05-28 16:00 ` Pucha, HimasekharX Reddy
3 siblings, 0 replies; 6+ messages in thread
From: Jacob Keller @ 2024-05-23 17:53 UTC (permalink / raw)
To: Michal Schmidt, Jesse Brandeburg, Tony Nguyen, intel-wired-lan
Cc: netdev, Thomas Gleixner, Nitesh Narayan Lal
On 5/22/2024 4:12 PM, Michal Schmidt wrote:
> irq_set_affinity_hint() is deprecated. Use irq_update_affinity_hint()
> instead. This removes the side-effect of actually applying the affinity.
>
> The driver does not really need to worry about spreading its IRQs across
> CPUs. The core code already takes care of that.
> On the contrary, when the driver applies affinities by itself, it breaks
> the users' expectations:
> 1. The user configures irqbalance with IRQBALANCE_BANNED_CPULIST in
> order to prevent IRQs from being moved to certain CPUs that run a
> real-time workload.
> 2. ice reconfigures VSIs at runtime due to a MIB change
> (ice_dcb_process_lldp_set_mib_change). Reopening a VSI resets the
> affinity in ice_vsi_req_irq_msix().
On an unrelated note, I wonder if this sort of reconfiguration could be
avoided so we don't lose such configuration....
> 3. ice has no idea about irqbalance's config, so it may move an IRQ to
> a banned CPU. The real-time workload suffers unacceptable latency.
>
Given all of these problems, what is remaining for us to completely
remove this API so that future driver authors don't make this mistake again?
> I am not sure if updating the affinity hints is at all useful, because
> irqbalance ignores them since 2016 ([1]), but at least it's harmless.
>
Yea. To be honest, I suspect this sort of code originates from
micro-bench marking with irqbalance disabled. I certainly remember folks
telling me that irqbalance wasn't helpful. In reality I think its
because when doing bench marking or testing you want run-to-run behavior
to stay consistent while irqbalance might be changing and tuning
parameters differently due to other system load.
> This ice change is similar to i40e commit d34c54d1739c ("i40e: Use
> irq_update_affinity_hint()").
>
> [1] https://github.com/Irqbalance/irqbalance/commit/dcc411e7bfdd
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
Makes sense.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++--
> drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 5371e91f6bbb..0f8b622db2b5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2587,8 +2587,8 @@ void ice_vsi_free_irq(struct ice_vsi *vsi)
> if (!IS_ENABLED(CONFIG_RFS_ACCEL))
> irq_set_affinity_notifier(irq_num, NULL);
>
> - /* clear the affinity_mask in the IRQ descriptor */
> - irq_set_affinity_hint(irq_num, NULL);
> + /* clear the affinity_hint in the IRQ descriptor */
> + irq_update_affinity_hint(irq_num, NULL);
> synchronize_irq(irq_num);
> devm_free_irq(ice_pf_to_dev(pf), irq_num, vsi->q_vectors[i]);
> }
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index f60c022f7960..a5d369b8fed5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2607,7 +2607,7 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi, char *basename)
> }
>
> /* assign the mask for this irq */
> - irq_set_affinity_hint(irq_num, &q_vector->affinity_mask);
> + irq_update_affinity_hint(irq_num, &q_vector->affinity_mask);
> }
>
> err = ice_set_cpu_rx_rmap(vsi);
> @@ -2625,7 +2625,7 @@ static int ice_vsi_req_irq_msix(struct ice_vsi *vsi, char *basename)
> irq_num = vsi->q_vectors[vector]->irq.virq;
> if (!IS_ENABLED(CONFIG_RFS_ACCEL))
> irq_set_affinity_notifier(irq_num, NULL);
> - irq_set_affinity_hint(irq_num, NULL);
> + irq_update_affinity_hint(irq_num, NULL);
> devm_free_irq(dev, irq_num, &vsi->q_vectors[vector]);
> }
> return err;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint()
2024-05-22 23:12 [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint() Michal Schmidt
2024-05-23 6:33 ` [Intel-wired-lan] [EXTERNAL] " Sunil Kovvuri Goutham
2024-05-23 17:53 ` [Intel-wired-lan] " Jacob Keller
@ 2024-05-24 21:04 ` Jacob Keller
2024-05-28 6:16 ` Michal Schmidt
2024-05-28 16:00 ` Pucha, HimasekharX Reddy
3 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2024-05-24 21:04 UTC (permalink / raw)
To: Michal Schmidt, Jesse Brandeburg, Tony Nguyen, intel-wired-lan
Cc: netdev, Thomas Gleixner, Nitesh Narayan Lal
On 5/22/2024 4:12 PM, Michal Schmidt wrote:
> irq_set_affinity_hint() is deprecated. Use irq_update_affinity_hint()
> instead. This removes the side-effect of actually applying the affinity.
>
> The driver does not really need to worry about spreading its IRQs across
> CPUs. The core code already takes care of that.
> On the contrary, when the driver applies affinities by itself, it breaks
> the users' expectations:
> 1. The user configures irqbalance with IRQBALANCE_BANNED_CPULIST in
> order to prevent IRQs from being moved to certain CPUs that run a
> real-time workload.
> 2. ice reconfigures VSIs at runtime due to a MIB change
> (ice_dcb_process_lldp_set_mib_change). Reopening a VSI resets the
> affinity in ice_vsi_req_irq_msix().
> 3. ice has no idea about irqbalance's config, so it may move an IRQ to
> a banned CPU. The real-time workload suffers unacceptable latency.
>
> I am not sure if updating the affinity hints is at all useful, because
> irqbalance ignores them since 2016 ([1]), but at least it's harmless.
>
> This ice change is similar to i40e commit d34c54d1739c ("i40e: Use
> irq_update_affinity_hint()").
>
> [1] https://github.com/Irqbalance/irqbalance/commit/dcc411e7bfdd
>
This is sent tagged for next, but the commit description reads more like
it fixes deployments running irqbalance. Would it make sense to mark
this with Fixes and target net instead?
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint()
2024-05-24 21:04 ` Jacob Keller
@ 2024-05-28 6:16 ` Michal Schmidt
0 siblings, 0 replies; 6+ messages in thread
From: Michal Schmidt @ 2024-05-28 6:16 UTC (permalink / raw)
To: Jacob Keller
Cc: Nitesh Narayan Lal, netdev, intel-wired-lan, Tony Nguyen,
Thomas Gleixner
On Fri, May 24, 2024 at 11:04 PM Jacob Keller <jacob.e.keller@intel.com> wrote:
> On 5/22/2024 4:12 PM, Michal Schmidt wrote:
> > irq_set_affinity_hint() is deprecated. Use irq_update_affinity_hint()
> > instead. This removes the side-effect of actually applying the affinity.
> >
> > The driver does not really need to worry about spreading its IRQs across
> > CPUs. The core code already takes care of that.
> > On the contrary, when the driver applies affinities by itself, it breaks
> > the users' expectations:
> > 1. The user configures irqbalance with IRQBALANCE_BANNED_CPULIST in
> > order to prevent IRQs from being moved to certain CPUs that run a
> > real-time workload.
> > 2. ice reconfigures VSIs at runtime due to a MIB change
> > (ice_dcb_process_lldp_set_mib_change). Reopening a VSI resets the
> > affinity in ice_vsi_req_irq_msix().
> > 3. ice has no idea about irqbalance's config, so it may move an IRQ to
> > a banned CPU. The real-time workload suffers unacceptable latency.
> >
> > I am not sure if updating the affinity hints is at all useful, because
> > irqbalance ignores them since 2016 ([1]), but at least it's harmless.
> >
> > This ice change is similar to i40e commit d34c54d1739c ("i40e: Use
> > irq_update_affinity_hint()").
> >
> > [1] https://github.com/Irqbalance/irqbalance/commit/dcc411e7bfdd
> >
>
> This is sent tagged for next, but the commit description reads more like
> it fixes deployments running irqbalance. Would it make sense to mark
> this with Fixes and target net instead?
I chose to not add a Fixes tag here. The similar commits for i40e and
other drivers did not have them either. If I had to add Fixes, the
referenced commit could be cdedef59deb0 ("ice: Configure VSIs for
Tx/Rx"), from 2018. But that's not good, because
irq_update_affinity_hint was added in 2021 with commit 65c7cdedeb30
("genirq: Provide new interfaces for affinity hints"). I'm OK with
next.
Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint()
2024-05-22 23:12 [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint() Michal Schmidt
` (2 preceding siblings ...)
2024-05-24 21:04 ` Jacob Keller
@ 2024-05-28 16:00 ` Pucha, HimasekharX Reddy
3 siblings, 0 replies; 6+ messages in thread
From: Pucha, HimasekharX Reddy @ 2024-05-28 16:00 UTC (permalink / raw)
To: mschmidt, Brandeburg, Jesse, Nguyen, Anthony L,
intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org, Thomas Gleixner, Nitesh Narayan Lal
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal Schmidt
> Sent: Thursday, May 23, 2024 4:43 AM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Thomas Gleixner <tglx@linutronix.de>; Nitesh Narayan Lal <nitesh@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next] ice: use irq_update_affinity_hint()
>
> irq_set_affinity_hint() is deprecated. Use irq_update_affinity_hint() instead. This removes the side-effect of actually applying the affinity.
>
> The driver does not really need to worry about spreading its IRQs across CPUs. The core code already takes care of that.
> On the contrary, when the driver applies affinities by itself, it breaks the users' expectations:
> 1. The user configures irqbalance with IRQBALANCE_BANNED_CPULIST in
> order to prevent IRQs from being moved to certain CPUs that run a
> real-time workload.
> 2. ice reconfigures VSIs at runtime due to a MIB change
> (ice_dcb_process_lldp_set_mib_change). Reopening a VSI resets the
> affinity in ice_vsi_req_irq_msix().
> 3. ice has no idea about irqbalance's config, so it may move an IRQ to
> a banned CPU. The real-time workload suffers unacceptable latency.
>
> I am not sure if updating the affinity hints is at all useful, because irqbalance ignores them since 2016 ([1]), but at least it's harmless.
>
> This ice change is similar to i40e commit d34c54d1739c ("i40e: Use irq_update_affinity_hint()").
>
> [1] https://github.com/Irqbalance/irqbalance/commit/dcc411e7bfdd
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
> drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++-- drivers/net/ethernet/intel/ice/ice_main.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
^ permalink raw reply [flat|nested] 6+ messages in thread