Re: [PATCH net-next 5/7] ice: Accumulate ring statistics over reset

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 14 Nov 2022 15:42:48 -0800 Tony Nguyen wrote:
> +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 = devm_kzalloc(dev, sizeof(*vsi_stat), GFP_KERNEL);

Don't use managed allocations if the structure's lifetime does not
match that of the driver instance. It's not generic garbage collection.

> +	if (!vsi_stat)
> +		return -ENOMEM;
> +
> +	vsi_stat->tx_ring_stats =
> +		devm_kcalloc(dev, vsi->alloc_txq,
> +			     sizeof(*vsi_stat->tx_ring_stats), GFP_KERNEL);
> +
> +	vsi_stat->rx_ring_stats =
> +		devm_kcalloc(dev, vsi->alloc_rxq,
> +			     sizeof(*vsi_stat->rx_ring_stats), GFP_KERNEL);
> +
> +	if (!vsi_stat->tx_ring_stats || !vsi_stat->rx_ring_stats)
> +		goto err_alloc;
> +
> +	pf->vsi_stats[vsi->idx] = vsi_stat;
> +
> +	return 0;
> +
> +err_alloc:

Don't do combined error checking, add appropriate labels for each case.

> +	devm_kfree(dev, vsi_stat->tx_ring_stats);
> +	vsi_stat->tx_ring_stats = NULL;

No need to clear the pointers, vsi_stats is freed few lines down.

> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
> +	vsi_stat->rx_ring_stats = NULL;
> +	devm_kfree(dev, vsi_stat);
> +	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,122 @@ 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);
> +	vsi_stat->tx_ring_stats = NULL;
> +	devm_kfree(dev, vsi_stat->rx_ring_stats);
> +	vsi_stat->rx_ring_stats = NULL;
> +	devm_kfree(dev, vsi_stat);
> +	pf->vsi_stats[vsi->idx] = NULL;
> +}
> +
> +/**
> + * ice_vsi_alloc_ring_stats - Allocates Tx and Rx ring stats for the VSI
> + * @vsi: VSI which is having stats allocated
> + */
> +static int ice_vsi_alloc_ring_stats(struct ice_vsi *vsi)
> +{
> +	struct ice_ring_stats **tx_ring_stats;
> +	struct ice_ring_stats **rx_ring_stats;
> +	struct ice_vsi_stats *vsi_stats;
> +	struct ice_pf *pf = vsi->back;
> +	u16 i;
> +
> +	if (!pf->vsi_stats)
> +		return -ENOENT;
> +
> +	vsi_stats = pf->vsi_stats[vsi->idx];
> +	if (!vsi_stats)
> +		return -ENOENT;
> +
> +	tx_ring_stats = vsi_stats->tx_ring_stats;
> +	if (!tx_ring_stats)
> +		return -ENOENT;
> +
> +	rx_ring_stats = vsi_stats->rx_ring_stats;
> +	if (!rx_ring_stats)
> +		return -ENOENT;
> +

Why all this NULL-checking? The init should have failed if any of these
are NULL. Please avoid defensive programming.