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

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: <20200817142952.GC2291654@lunn.ch>
Date:   Mon, 17 Aug 2020 16:29:52 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        jiri@...dia.com, amcohen@...dia.com, danieller@...dia.com,
        mlxsw@...dia.com, roopa@...dia.com, dsahern@...il.com,
        f.fainelli@...il.com, vivien.didelot@...il.com, saeedm@...dia.com,
        tariqt@...dia.com, ayal@...dia.com, eranbe@...dia.com,
        mkubecek@...e.cz, Ido Schimmel <idosch@...dia.com>
Subject: Re: [RFC PATCH net-next 6/6] mlxsw: spectrum_nve: Expose VXLAN
 counters via devlink-metric

> +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