netdev - Re: [PATCH iwl-next] ice: use irq_update_affinity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c4d4c8f0-5088-430d-a076-dc4bd8cfbc55@intel.com>
Date: Thu, 23 May 2024 10:53:37 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Michal Schmidt <mschmidt@...hat.com>, Jesse Brandeburg
	<jesse.brandeburg@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, Nitesh Narayan Lal <nitesh@...hat.com>, "Thomas
 Gleixner" <tglx@...utronix.de>
Subject: Re: [PATCH iwl-next] ice: use irq_update_affinity_hint()



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@...hat.com>
> ---

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@...el.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;