netdev - Re: [PATCH net-next 12/12] ice: Ethtool fdb

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: <6f23fe3c-c23e-7a37-f22b-21a59281715c@intel.com>
Date:   Fri, 21 Apr 2023 18:32:41 +0200
From:   Alexander Lobakin <aleksander.lobakin@...el.com>
To:     Wojciech Drewek <wojciech.drewek@...el.com>
CC:     <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
        <david.m.ertman@...el.com>, <michal.swiatkowski@...ux.intel.com>,
        <marcin.szycik@...ux.intel.com>, <pawel.chmielewski@...el.com>,
        <sridhar.samudrala@...el.com>
Subject: Re: [PATCH net-next 12/12] ice: Ethtool fdb_cnt stats

From: Wojciech Drewek <wojciech.drewek@...el.com>
Date: Mon, 17 Apr 2023 11:34:12 +0200

> Introduce new ethtool statistic which is 'fdb_cnt'. It
> provides information about how many bridge fdbs are created on
> a given netdev.

[...]

> @@ -339,6 +340,7 @@ ice_eswitch_br_fdb_entry_delete(struct ice_esw_br *bridge,
>  	ice_eswitch_br_flow_delete(pf, fdb_entry->flow);
>  
>  	kfree(fdb_entry);
> +	vsi->fdb_cnt--;

Are FDB operations always serialized within one netdev? Because if it's
not, this probably needs to be atomic_t.

>  }
>  
>  static void

[...]

> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 8407c7175cf6..d06b2a688323 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -64,6 +64,7 @@ static const struct ice_stats ice_gstrings_vsi_stats[] = {
>  	ICE_VSI_STAT("tx_linearize", tx_linearize),
>  	ICE_VSI_STAT("tx_busy", tx_busy),
>  	ICE_VSI_STAT("tx_restart", tx_restart),
> +	ICE_VSI_STAT("fdb_cnt", fdb_cnt),

It's confusing to me to see it in the Ethtool stats. They're usually
counters, ice is no an exception. But this one is not, so it might give
wrong impression.
Have you considered alternatives? rtnl (iproute) or maybe even Devlink
(but I believe the former fits better)? This might be a good candidate
to become a generic stat, who knows.

>  };
>  
>  enum ice_ethtool_test_id {

Thanks,
Olek