Re: [RFC PATCH net-next 6/6] mlxsw: spectrum_nve: Expose VXLAN counters via devlink-metric

[Andrew Lunn <andrew@lunn.ch>]

> +static int mlxsw_sp1_nve_vxlan_metrics_init(struct mlxsw_sp *mlxsw_sp)
> +{
> +	struct mlxsw_sp_nve_metrics *metrics = &mlxsw_sp->nve->metrics;
> +	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
> +	int err;
> +
> +	err = mlxsw_sp1_nve_vxlan_counters_clear(mlxsw_sp);
> +	if (err)
> +		return err;
> +
> +	metrics->counter_encap =
> +		devlink_metric_counter_create(devlink, "nve_vxlan_encap",
> +					      &mlxsw_sp1_nve_vxlan_encap_ops,
> +					      mlxsw_sp);
> +	if (IS_ERR(metrics->counter_encap))
> +		return PTR_ERR(metrics->counter_encap);
> +
> +	metrics->counter_decap =
> +		devlink_metric_counter_create(devlink, "nve_vxlan_decap",
> +					      &mlxsw_sp1_nve_vxlan_decap_ops,
> +					      mlxsw_sp);
> +	if (IS_ERR(metrics->counter_decap)) {
> +		err = PTR_ERR(metrics->counter_decap);
> +		goto err_counter_decap;
> +	}
> +
> +	metrics->counter_decap_errors =
> +		devlink_metric_counter_create(devlink, "nve_vxlan_decap_errors",
> +					      &mlxsw_sp1_nve_vxlan_decap_errors_ops,
> +					      mlxsw_sp);
> +	if (IS_ERR(metrics->counter_decap_errors)) {
> +		err = PTR_ERR(metrics->counter_decap_errors);
> +		goto err_counter_decap_errors;
> +	}
> +
> +	metrics->counter_decap_discards =
> +		devlink_metric_counter_create(devlink, "nve_vxlan_decap_discards",
> +					      &mlxsw_sp1_nve_vxlan_decap_discards_ops,
> +					      mlxsw_sp);
> +	if (IS_ERR(metrics->counter_decap_discards)) {
> +		err = PTR_ERR(metrics->counter_decap_discards);
> +		goto err_counter_decap_discards;
> +	}
> +
> +	return 0;

Looking at this, i wonder about the scalability of this API. With just
4 counters it looks pretty ugly. What about 50 counters?

Maybe move the name into the ops structure. Then add a call
devlink_metric_counters_create() where you can pass an array and array
size of op structures? There are plenty of other examples in the
kernel, e.g. sysfs groups, hwmon, etc. where you register a large
bunch of things with the core with a single call.

> +static void mlxsw_sp1_nve_vxlan_metrics_fini(struct mlxsw_sp *mlxsw_sp)
> +{
> +	struct mlxsw_sp_nve_metrics *metrics = &mlxsw_sp->nve->metrics;
> +	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
> +
> +	devlink_metric_destroy(devlink, metrics->counter_decap_discards);
> +	devlink_metric_destroy(devlink, metrics->counter_decap_errors);
> +	devlink_metric_destroy(devlink, metrics->counter_decap);
> +	devlink_metric_destroy(devlink, metrics->counter_encap);
> +}

I guess the most frequent use case is to remove all counters,
e.g. driver unload, or when probe fails. So maybe provide a
devlink_metric_destroy_all(devlink) ?

    Andrew