Re: [Intel-wired-lan] [PATCH net v1] i40e: Fix not setting xps_cpus after reset

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 9/9/2022 2:36 AM, Kamil Maziarz wrote:
> From: Michal Jaron <michalx.jaron@intel.com>
> 
> During tx rings configuration default xps queue config is set and
> __I40E_TX_XPS_INIT_DONE is locked. Xps cpus maps are cleared in

nit: XPS and CPU are acronyms so their use should be capitalized in the 
commit and comments.

> every reset by netdev_set_num_tc() call regardless it was set by
> user or driver. If reset with reinit occurs __I40E_TX_XPS_INIT_DONE
> flag is removed and xps mapping is set to default again but after
> reset without reinit this flag is still set and xps cpus to queues
> mapping stays cleared.
> 
> Add code to preserve xps_cpus mapping as cpumask for every queue
> and restore those mapping at the end of reset.
> 
> Fixes: 6f853d4f8e93 ("i40e: allow XPS with QoS enabled")
> Signed-off-by: Michal Jaron <michalx.jaron@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> ---
>   drivers/net/ethernet/intel/i40e/i40e.h      |   6 +
>   drivers/net/ethernet/intel/i40e/i40e_main.c | 117 ++++++++++++++++++++
>   2 files changed, 123 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index d86b6d349ea9..e01af5943bfe 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -1306,4 +1306,10 @@ static inline u32 i40e_is_tc_mqprio_enabled(struct i40e_pf *pf)
>   	return pf->flags & I40E_FLAG_TC_MQPRIO;
>   }
>   
> +/* reverse xps cpus to tx queues map */
> +struct i40e_qmap_rev {
> +	struct cpumask cpus;
> +	int vsi_id;
> +};
> +
>   #endif /* _I40E_H_ */
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 9f1d5de7bf16..19a1cd0aa4f0 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -10770,6 +10770,89 @@ static int i40e_reset(struct i40e_pf *pf)
>   	return ret;
>   }
>   
> +#ifdef CONFIG_XPS
> +/**
> + * i40e_preserve_xps_settings - preserve xps maps before reset
> + * @vsi: pointer to the targeted VSI
> + * @qr: pointer to the structure with xps mapping
> + *
> + * Read queues mapping from every cpu and save it as a cpu mask for every
> + * queue.
> + **/
> +static void
> +i40e_preserve_xps_settings(struct i40e_vsi *vsi, struct i40e_qmap_rev *qr)
> +{
> +	int cpu, q_idx, cpu_idx, cpus = num_online_cpus();
> +	struct net_device *netdev = vsi->netdev;
> +	struct xps_dev_maps *dev_maps;
> +	struct xps_map *map;
> +	u64 bitmap_arr;
> +
> +	if (!IS_ENABLED(CONFIG_XPS))
> +		return;

This seems unnecessary as the while function is wrapped in ifdef CONFIG_XPS

> +
> +	if (!netdev || vsi->type != I40E_VSI_MAIN)
> +		return;
> +
> +	if (cpus < vsi->num_queue_pairs) {
> +		dev_warn(&vsi->back->pdev->dev,
> +			 "There are more queues than cpus. To set xps maps properly reinitialize queues.\n");
> +		return;
> +	}
> +
> +	rcu_read_lock();
> +
> +	if (!netdev->xps_maps[XPS_CPUS])
> +		goto out;
> +
> +	dev_maps = rcu_dereference(netdev->xps_maps[XPS_CPUS]);
> +
> +	for (cpu = 0; cpu < cpus; cpu++) {
> +		cpu_idx = cpumask_local_spread(cpu, -1);
> +		if (!dev_maps->attr_map[cpu_idx])
> +			continue;
> +
> +		map = rcu_dereference(dev_maps->attr_map[cpu_idx]);
> +		bitmap_arr = cpu_idx;
> +		do_div(bitmap_arr, BITS_PER_LONG);
> +		for (q_idx = 0; q_idx < map->len; q_idx++) {
> +			qr[map->queues[q_idx]].vsi_id = vsi->id;
> +			qr[map->queues[q_idx]].cpus.bits[bitmap_arr] |=
> +				BIT(cpu_idx);
> +		}
> +	}
> +
> +out:
> +	rcu_read_unlock();
> +}
> +
> +/**
> + * i40e_restore_xps_settings - restore xps maps after reset
> + * @vsi: pointer to the targeted VSI
> + * @qr: pointer to the structure with xps mapping
> + *
> + * Set previously preserved xps cpus to queues mapping.
> + **/
> +static void
> +i40e_restore_xps_settings(struct i40e_vsi *vsi, struct i40e_qmap_rev *qr)
> +{
> +	struct net_device *netdev = vsi->netdev;
> +	int q_count, q;
> +
> +	if (!IS_ENABLED(CONFIG_XPS))
> +		return;

This check as well.

> +
> +	q_count = min_t(unsigned int, num_online_cpus(), vsi->num_queue_pairs);
> +
> +	if (!qr || vsi->type != I40E_VSI_MAIN)

qr is already being checked in the caller so no need to check it again here.

> +		return;
> +
> +	for (q = 0; q < q_count; q++)
> +		if (qr[q].vsi_id == vsi->id)
> +			netif_set_xps_queue(netdev, &qr[q].cpus, q);
> +}
> +
> +#endif /* CONFIG_XPS */
>   /**
>    * i40e_rebuild - rebuild using a saved config
>    * @pf: board private structure
> @@ -10781,6 +10864,9 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>   {
>   	const bool is_recovery_mode_reported = i40e_check_recovery_mode(pf);
>   	struct i40e_vsi *vsi = pf->vsi[pf->lan_vsi];
> +#ifdef CONFIG_XPS
> +	struct i40e_qmap_rev *qr = NULL;
> +#endif /* CONFIG_XPS */
>   	struct i40e_hw *hw = &pf->hw;
>   	i40e_status ret;
>   	u32 val;
> @@ -10896,6 +10982,23 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>   	}
>   
>   #endif /* CONFIG_I40E_DCB */
> +#ifdef CONFIG_XPS
> +	if (!reinit) {
> +		int cpus = num_possible_cpus();
> +
> +		qr = kcalloc(cpus, sizeof(struct i40e_qmap_rev), GFP_KERNEL);
> +
> +		if (!qr) {

No newline between the call and the error check.

> +			ret = -ENOMEM;
> +			goto end_unlock;
> +		}
> +
> +		for (v = 0; v < pf->num_alloc_vsi; v++)
> +			if (pf->vsi[v])
> +				i40e_preserve_xps_settings(pf->vsi[v], qr);
> +	}
> +
> +#endif /* CONFIG_XPS */
>   	if (!lock_acquired)
>   		rtnl_lock();
>   	ret = i40e_setup_pf_switch(pf, reinit, true);
> @@ -11050,6 +11153,16 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>   
>   	i40e_reset_all_vfs(pf, true);
>   
> +#ifdef CONFIG_XPS
> +	if (!reinit) {
> +		for (v = 0; v < pf->num_alloc_vsi; v++)
> +			if (pf->vsi[v])
> +				i40e_restore_xps_settings(pf->vsi[v], qr);
> +	} else {
> +		dev_info(&pf->pdev->dev, "XPS maps were reset to default after queue re-initialization");
> +	}
> +
> +#endif /* CONFIG_XPS */
>   	/* tell the firmware that we're starting */
>   	i40e_send_version(pf);
>   
> @@ -11059,6 +11172,10 @@ static void i40e_rebuild(struct i40e_pf *pf, bool reinit, bool lock_acquired)
>   end_unlock:
>   	if (!lock_acquired)
>   		rtnl_unlock();
> +#ifdef CONFIG_XPS
> +	if (!reinit)
> +		kfree(qr)

kfree is null safe so no need for the check.

> +#endif /* CONFIG_XPS */
>   end_core_reset:
>   	clear_bit(__I40E_RESET_FAILED, pf->state);
>   clear_recovery:
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan