Re: [RFC PATCH net-next 0/7] 802.1Q Frame Preemption and 802.3 MAC Merge support via ethtool

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Wed, Aug 17, 2022 at 11:46:42AM -0700, Jakub Kicinski wrote:
> > > When we have separate set of stats for pMAC the normal stats are sum of
> > > all traffic, right? So normal - pMAC == eMAC, everything that's not
> > > preemptible is express?  
> > 
> > Actually not quite, or at least not for the LS1028A ENETC and Felix switch.
> > The normal counters report just what the eMAC sees, and the pMAC counters
> > just what the pMAC sees. After all, only the eMAC was enabled up until now.
> > Nobody does the addition currently.
> 
> I see. And the netdev stats are the total?

dev->stats reports the aggregate of express and preemptable packets seen
by software, yes. I got the hint though, I should also report the
aggregate. The summation seems like a generic problem which ethtool
should be able to do internally, yet a generic implementation is riddled
with problems that must be dealt with (RMON histograms reported by eMAC
and pMAC can be different; some counters could be implemented by the
eMAC but not the pMAC or vice versa, and that needs to be handled, etc).
Additionally, the summation of counters must also be done for ndo_get_stats64(),
when those come from hardware as well. So I'd incline to do it in the
driver rn.

> > > Did you consider adding an attribute for switching between MAC and pMAC
> > > for stats rather than duplicating things?  
> > 
> > No. Could you expand on that idea a little? Add a netlink attribute
> > where, and this helps reduce duplication where, and how?
> 
> Add a attribute to ETHTOOL_MSG_STATS_GET, let's call it
> ETHTOOL_A_STATS_EXPRESS, a flag.

I'll add this to the UAPI and to internal data structures, ok?

enum ethtool_stats_src {
	ETHTOOL_STATS_SRC_AGGREGATE = 0,
	ETHTOOL_STATS_SRC_EMAC,
	ETHTOOL_STATS_SRC_PMAC,
};

> Plumb thru to all the stats callback an extra argument 
> (a structure for future extensibility) with a bool pMAC;
> 
> Add a capability field to ethtool_ops to announce that
> driver will pay attention to the bool pMAC / has support.

You mean capability field as in ethtool_ops::supported_coalesce_params,
right? (we discussed about this separately).
This won't fit the enetc driver very well. Some enetc ports on the NXP
LS1028A support the MM layer (port 0, port 2) and some don't (port 1,
port 3). Yet they share the same PF driver. So populating mm_supported =
true in the const struct enetc_pf_ethtool_ops isn't going to cover both.
I can, however, key on my ethtool_ops :: get_mm_state() function which
lets the driver report a "bool supported". Is this ok?

> We can then use the existing callbacks.
> 
> Am I making sense?

Yes, thanks.