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: <MW4PR11MB5776E4EFEA7FDE7E52C79E1EFD769@MW4PR11MB5776.namprd11.prod.outlook.com>
Date: Tue, 9 May 2023 12:52:26 +0000
From: "Drewek, Wojciech" <wojciech.drewek@...el.com>
To: "Lobakin, Aleksander" <aleksander.lobakin@...el.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Ertman, David M"
	<david.m.ertman@...el.com>, "michal.swiatkowski@...ux.intel.com"
	<michal.swiatkowski@...ux.intel.com>, "marcin.szycik@...ux.intel.com"
	<marcin.szycik@...ux.intel.com>, "Chmielewski, Pawel"
	<pawel.chmielewski@...el.com>, "Samudrala, Sridhar"
	<sridhar.samudrala@...el.com>
Subject: RE: [PATCH net-next 12/12] ice: Ethtool fdb_cnt stats



> -----Original Message-----
> From: Lobakin, Aleksander <aleksander.lobakin@...el.com>
> Sent: piątek, 21 kwietnia 2023 18:33
> To: Drewek, Wojciech <wojciech.drewek@...el.com>
> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Ertman, David M <david.m.ertman@...el.com>;
> michal.swiatkowski@...ux.intel.com; marcin.szycik@...ux.intel.com; Chmielewski, Pawel <pawel.chmielewski@...el.com>;
> Samudrala, Sridhar <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.

All the FDB operations are done either from notification context so they are protected by
rtnl_lock or explicitly protected by us (see ice_eswitch_br_fdb_event_work, we use rtnl_lock there).

> 
> >  }
> >
> >  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.

I'll do some research on alternatives

> 
> >  };
> >
> >  enum ice_ethtool_test_id {
> 
> Thanks,
> Olek