Re: [Intel-wired-lan] [net-next, v6 2/2] ice: Accumulate ring statistics over reset

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

On 11/16/2022 3:26 PM, Benjamin Mikailenko wrote:
> Resets may occur with or without user interaction. For example, a TX hang
> or reconfiguration of parameters will result in a reset. During reset, the
> VSI is freed, freeing any statistics structures inside as well. This would
> create an issue for the user where a reset happens in the background,
> statistics set to zero, and the user checks ring statistics expecting them
> to be populated.
> 
> To ensure this doesn't happen, accumulate ring statistics over reset.
> 
> Define a new ring statistics structure, ice_ring_stats. The new structure
> lives in the VSI's parent, preserving ring statistics when VSI is freed.
> 
> 1. Define a new structure vsi_ring_stats in the PF scope
> 2. Allocate/free stats only during probe, unload, or change in ring size
> 3. Replace previous ring statistics functionality with new structure
> 
> Signed-off-by: Benjamin Mikailenko <benjamin.mikailenko@intel.com>
> ---

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index a5945319b62e..02392546c007 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -447,6 +447,52 @@ static irqreturn_t ice_eswitch_msix_clean_rings(int __always_unused irq, void *d
>   	return IRQ_HANDLED;
>   }
>   
> +/**
> + * ice_vsi_alloc_stat_arrays - Allocate statistics arrays
> + * @vsi: VSI pointer
> + */
> +static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
> +{
> +	struct ice_vsi_stats *vsi_stat;
> +	struct ice_pf *pf = vsi->back;
> +	struct device *dev;
> +
> +	dev = ice_pf_to_dev(pf);
> +
> +	if (vsi->type == ICE_VSI_CHNL)
> +		return 0;
> +	if (!pf->vsi_stats)
> +		return -ENOENT;
> +
> +	vsi_stat = kzalloc(sizeof(*vsi_stat), GFP_KERNEL);
> +	if (!vsi_stat)
> +		return -ENOMEM;
> +
> +	vsi_stat->tx_ring_stats =
> +		kcalloc(vsi->alloc_txq, sizeof(*vsi_stat->tx_ring_stats),
> +			GFP_KERNEL);
> +	if (!vsi_stat->tx_ring_stats)
> +		goto err_alloc_tx;
> +
> +	vsi_stat->rx_ring_stats =
> +		kcalloc(vsi->alloc_rxq, sizeof(*vsi_stat->rx_ring_stats),
> +			GFP_KERNEL);
> +	if (!vsi_stat->rx_ring_stats)
> +		goto err_alloc_rx;
> +
> +	pf->vsi_stats[vsi->idx] = vsi_stat;
> +
> +	return 0;
> +
> +err_alloc_rx:
> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
> +err_alloc_tx:
> +	devm_kfree(dev, vsi_stat->tx_ring_stats);
> +	devm_kfree(dev, vsi_stat);

These need to be updated as well. If you used non-managed allocs, you 
need to use non-managed frees.

> +	pf->vsi_stats[vsi->idx] = NULL;
> +	return -ENOMEM;
> +}
> +
>   /**
>    * ice_vsi_alloc - Allocates the next available struct VSI in the PF
>    * @pf: board private structure
> @@ -560,6 +606,11 @@ ice_vsi_alloc(struct ice_pf *pf, enum ice_vsi_type vsi_type,
>   
>   	if (vsi->type == ICE_VSI_CTRL && vf)
>   		vf->ctrl_vsi_idx = vsi->idx;
> +
> +	/* allocate memory for Tx/Rx ring stat pointers */
> +	if (ice_vsi_alloc_stat_arrays(vsi))
> +		goto err_rings;
> +
>   	goto unlock_pf;
>   
>   err_rings:
> @@ -1535,6 +1586,109 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
>   	return -ENOMEM;
>   }
>   
> +/**
> + * ice_vsi_free_stats - Free the ring statistics structures
> + * @vsi: VSI pointer
> + */
> +static void ice_vsi_free_stats(struct ice_vsi *vsi)
> +{
> +	struct ice_vsi_stats *vsi_stat;
> +	struct ice_pf *pf = vsi->back;
> +	struct device *dev;
> +	int i;
> +
> +	dev = ice_pf_to_dev(pf);
> +
> +	if (vsi->type == ICE_VSI_CHNL)
> +		return;
> +	if (!pf->vsi_stats)
> +		return;
> +
> +	vsi_stat = pf->vsi_stats[vsi->idx];
> +	if (!vsi_stat)
> +		return;
> +
> +	ice_for_each_alloc_txq(vsi, i) {
> +		if (vsi_stat->tx_ring_stats[i]) {
> +			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
> +			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
> +		}
> +	}
> +
> +	ice_for_each_alloc_rxq(vsi, i) {
> +		if (vsi_stat->rx_ring_stats[i]) {
> +			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
> +			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
> +		}
> +	}
> +
> +	devm_kfree(dev, vsi_stat->tx_ring_stats);
> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
> +	devm_kfree(dev, vsi_stat);

These as well.

> +	pf->vsi_stats[vsi->idx] = NULL;
> +}
> +

...

> @@ -2567,6 +2573,15 @@ static int ice_xdp_alloc_setup_rings(struct ice_vsi *vsi)
>   		if (!xdp_ring)
>   			goto free_xdp_rings;
>   
> +		ring_stats = (struct ice_ring_stats *)
> +			kzalloc(sizeof(*ring_stats), GFP_KERNEL);
> +
> +		if (!ring_stats) {

Cast not needed. No newline either between allocation and error check.

> +			ice_free_tx_ring(xdp_ring);
> +			goto free_xdp_rings;
> +		}
> +
> +		xdp_ring->ring_stats = ring_stats;
>   		xdp_ring->q_index = xdp_q_idx;
>   		xdp_ring->reg_idx = vsi->txq_map[xdp_q_idx];
>   		xdp_ring->vsi = vsi;
> @@ -2589,9 +2604,13 @@ static int ice_xdp_alloc_setup_rings(struct ice_vsi *vsi)
>   	return 0;
>   
>   free_xdp_rings:
> -	for (; i >= 0; i--)
> -		if (vsi->xdp_rings[i] && vsi->xdp_rings[i]->desc)
> +	for (; i >= 0; i--) {
> +		if (vsi->xdp_rings[i] && vsi->xdp_rings[i]->desc) {
> +			kfree_rcu(vsi->xdp_rings[i]->ring_stats, rcu);
> +			vsi->xdp_rings[i]->ring_stats = NULL;
>   			ice_free_tx_ring(vsi->xdp_rings[i]);
> +		}
> +	}
>   	return -ENOMEM;
>   }
>   
> @@ -2792,6 +2811,8 @@ int ice_destroy_xdp_rings(struct ice_vsi *vsi)
>   				synchronize_rcu();
>   				ice_free_tx_ring(vsi->xdp_rings[i]);
>   			}
> +			kfree_rcu(vsi->xdp_rings[i]->ring_stats, rcu);
> +			vsi->xdp_rings[i]->ring_stats = NULL;
>   			kfree_rcu(vsi->xdp_rings[i], rcu);
>   			vsi->xdp_rings[i] = NULL;
>   		}
> @@ -4771,11 +4792,19 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>   		goto err_init_pf_unroll;
>   	}
>   
> +	pf->vsi_stats = devm_kcalloc(dev, pf->num_alloc_vsi,
> +				     sizeof(*pf->vsi_stats), GFP_KERNEL);
> +
> +	if (!pf->vsi_stats) {

No newline here either.

> +		err = -ENOMEM;
> +		goto err_init_vsi_unroll;
> +	}
> +
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan