Message-ID: <20220817110644.bhvzl7fslq2l6g23@skbuf>
Date:   Wed, 17 Aug 2022 11:06:45 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxim Kochetkov <fido_max@...ox.ru>
Subject: Re: [PATCH net 6/8] net: mscc: ocelot: make struct ocelot_stat_layout
 array indexable

On Tue, Aug 16, 2022 at 11:46:46PM -0700, Colin Foster wrote:
> Hi Vladimir,
> 
> I'm not sure if this is an issue here, and I'm not sure it will ever
> be... ocelot_stat_layout as you know relies on consecutive register
> addresses to be most efficient. This was indirectly enforced by
> *_stats_layout[] always being laid out in ascending order.
> 
> If the order of ocelot_stat doesn't match each device's register
> offset order, there'll be unnecessary overhead. I tried to test
> this just now, but I'll have to deal with a few conflicts that I won't
> be able to get to immediately.
> 
> Do you see this as a potential issue in the future? Or do you expect all
> hardware is simliar enough that they'll all be ordered the same?
> 
> Or, because I'm the lucky one running on a slow SPI bus, this will be my
> problem to monitor and fix if need be :)

No, you're right and the bulk reads complicate things; in fact I'm not
sure that this patch even works by anything other than pure chance.

In some future changes I'm making the first N elements of the
struct ocelot_stat_layout array be common across all drivers. Only Felix
VSC9959 has some extra TSN-related counters which the others don't have.
It would look like this:

/* 32-bit counter checked for wraparound by ocelot_port_update_stats()
 * and copied to ocelot->stats.
 */
#define OCELOT_STAT(kind) \
	[OCELOT_STAT_ ## kind] = { .reg = SYS_COUNT_ ## kind }
/* Same as above, except also exported to ethtool -S. Standard counters should
 * only be exposed to more specific interfaces rather than by their string name.
 */
#define OCELOT_STAT_ETHTOOL(kind, ethtool_name) \
	[OCELOT_STAT_ ## kind] = { .reg = SYS_COUNT_ ## kind, .name = ethtool_name }

#define OCELOT_COMMON_STATS \
	OCELOT_STAT_ETHTOOL(RX_OCTETS, "rx_octets"), \
	OCELOT_STAT_ETHTOOL(RX_UNICAST, "rx_unicast"), \
	OCELOT_STAT_ETHTOOL(RX_MULTICAST, "rx_multicast"), \
	OCELOT_STAT_ETHTOOL(RX_BROADCAST, "rx_broadcast"), \
	OCELOT_STAT_ETHTOOL(RX_SHORTS, "rx_shorts"), \
	OCELOT_STAT_ETHTOOL(RX_FRAGMENTS, "rx_fragments"), \
	OCELOT_STAT_ETHTOOL(RX_JABBERS, "rx_jabbers"), \
	OCELOT_STAT_ETHTOOL(RX_CRC_ALIGN_ERRS, "rx_crc_align_errs"), \
	OCELOT_STAT_ETHTOOL(RX_SYM_ERRS, "rx_sym_errs"), \
	OCELOT_STAT_ETHTOOL(RX_64, "rx_frames_below_65_octets"), \
	OCELOT_STAT_ETHTOOL(RX_65_127, "rx_frames_65_to_127_octets"), \
	OCELOT_STAT_ETHTOOL(RX_128_255, "rx_frames_128_to_255_octets"), \
	OCELOT_STAT_ETHTOOL(RX_256_511, "rx_frames_256_to_511_octets"), \
	OCELOT_STAT_ETHTOOL(RX_512_1023, "rx_frames_512_to_1023_octets"), \
	OCELOT_STAT_ETHTOOL(RX_1024_1526, "rx_frames_1024_to_1526_octets"), \
	OCELOT_STAT_ETHTOOL(RX_1527_MAX, "rx_frames_over_1526_octets"), \
	OCELOT_STAT_ETHTOOL(RX_PAUSE, "rx_pause"), \
	OCELOT_STAT_ETHTOOL(RX_CONTROL, "rx_control"), \
	OCELOT_STAT_ETHTOOL(RX_LONGS, "rx_longs"), \
	OCELOT_STAT_ETHTOOL(RX_CLASSIFIED_DROPS, "rx_classified_drops"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_0, "rx_red_prio_0"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_1, "rx_red_prio_1"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_2, "rx_red_prio_2"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_3, "rx_red_prio_3"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_4, "rx_red_prio_4"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_5, "rx_red_prio_5"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_6, "rx_red_prio_6"), \
	OCELOT_STAT_ETHTOOL(RX_RED_PRIO_7, "rx_red_prio_7"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_0, "rx_yellow_prio_0"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_1, "rx_yellow_prio_1"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_2, "rx_yellow_prio_2"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_3, "rx_yellow_prio_3"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_4, "rx_yellow_prio_4"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_5, "rx_yellow_prio_5"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_6, "rx_yellow_prio_6"), \
	OCELOT_STAT_ETHTOOL(RX_YELLOW_PRIO_7, "rx_yellow_prio_7"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_0, "rx_green_prio_0"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_1, "rx_green_prio_1"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_2, "rx_green_prio_2"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_3, "rx_green_prio_3"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_4, "rx_green_prio_4"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_5, "rx_green_prio_5"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_6, "rx_green_prio_6"), \
	OCELOT_STAT_ETHTOOL(RX_GREEN_PRIO_7, "rx_green_prio_7"), \
	OCELOT_STAT_ETHTOOL(TX_OCTETS, "tx_octets"), \
	OCELOT_STAT_ETHTOOL(TX_UNICAST, "tx_unicast"), \
	OCELOT_STAT_ETHTOOL(TX_MULTICAST, "tx_multicast"), \
	OCELOT_STAT_ETHTOOL(TX_BROADCAST, "tx_broadcast"), \
	OCELOT_STAT_ETHTOOL(TX_COLLISION, "tx_collision"), \
	OCELOT_STAT_ETHTOOL(TX_DROPS, "tx_drops"), \
	OCELOT_STAT_ETHTOOL(TX_PAUSE, "tx_pause"), \
	OCELOT_STAT_ETHTOOL(TX_64, "tx_frames_below_65_octets"), \
	OCELOT_STAT_ETHTOOL(TX_65_127, "tx_frames_65_to_127_octets"), \
	OCELOT_STAT_ETHTOOL(TX_128_255, "tx_frames_128_255_octets"), \
	OCELOT_STAT_ETHTOOL(TX_256_511, "tx_frames_256_511_octets"), \
	OCELOT_STAT_ETHTOOL(TX_512_1023, "tx_frames_512_1023_octets"), \
	OCELOT_STAT_ETHTOOL(TX_1024_1526, "tx_frames_1024_1526_octets"), \
	OCELOT_STAT_ETHTOOL(TX_1527_MAX, "tx_frames_over_1526_octets"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_0, "tx_yellow_prio_0"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_1, "tx_yellow_prio_1"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_2, "tx_yellow_prio_2"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_3, "tx_yellow_prio_3"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_4, "tx_yellow_prio_4"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_5, "tx_yellow_prio_5"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_6, "tx_yellow_prio_6"), \
	OCELOT_STAT_ETHTOOL(TX_YELLOW_PRIO_7, "tx_yellow_prio_7"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_0, "tx_green_prio_0"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_1, "tx_green_prio_1"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_2, "tx_green_prio_2"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_3, "tx_green_prio_3"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_4, "tx_green_prio_4"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_5, "tx_green_prio_5"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_6, "tx_green_prio_6"), \
	OCELOT_STAT_ETHTOOL(TX_GREEN_PRIO_7, "tx_green_prio_7"), \
	OCELOT_STAT_ETHTOOL(TX_AGED, "tx_aged"), \
	OCELOT_STAT_ETHTOOL(DROP_LOCAL, "drop_local"), \
	OCELOT_STAT_ETHTOOL(DROP_TAIL, "drop_tail"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_0, "drop_yellow_prio_0"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_1, "drop_yellow_prio_1"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_2, "drop_yellow_prio_2"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_3, "drop_yellow_prio_3"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_4, "drop_yellow_prio_4"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_5, "drop_yellow_prio_5"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_6, "drop_yellow_prio_6"), \
	OCELOT_STAT_ETHTOOL(DROP_YELLOW_PRIO_7, "drop_yellow_prio_7"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_0, "drop_green_prio_0"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_1, "drop_green_prio_1"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_2, "drop_green_prio_2"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_3, "drop_green_prio_3"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_4, "drop_green_prio_4"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_5, "drop_green_prio_5"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_6, "drop_green_prio_6"), \
	OCELOT_STAT_ETHTOOL(DROP_GREEN_PRIO_7, "drop_green_prio_7")

static const struct ocelot_stat_layout ocelot_stats_layout[OCELOT_NUM_STATS] = {
	OCELOT_COMMON_STATS,
};

static const struct ocelot_stat_layout vsc9953_stats_layout[OCELOT_NUM_STATS] = {
	OCELOT_COMMON_STATS,
};

static const struct ocelot_stat_layout vsc9959_stats_layout[OCELOT_NUM_STATS] = {
	OCELOT_COMMON_STATS,
	OCELOT_STAT(RX_ASSEMBLY_ERRS),
	OCELOT_STAT(RX_SMD_ERRS),
	OCELOT_STAT(RX_ASSEMBLY_OK),
	OCELOT_STAT(RX_MERGE_FRAGMENTS),
	OCELOT_STAT(TX_MERGE_FRAGMENTS),
	OCELOT_STAT(RX_PMAC_OCTETS),
	OCELOT_STAT(RX_PMAC_UNICAST),
	OCELOT_STAT(RX_PMAC_MULTICAST),
	OCELOT_STAT(RX_PMAC_BROADCAST),
	OCELOT_STAT(RX_PMAC_SHORTS),
	OCELOT_STAT(RX_PMAC_FRAGMENTS),
	OCELOT_STAT(RX_PMAC_JABBERS),
	OCELOT_STAT(RX_PMAC_CRC_ALIGN_ERRS),
	OCELOT_STAT(RX_PMAC_SYM_ERRS),
	OCELOT_STAT(RX_PMAC_64),
	OCELOT_STAT(RX_PMAC_65_127),
	OCELOT_STAT(RX_PMAC_128_255),
	OCELOT_STAT(RX_PMAC_256_511),
	OCELOT_STAT(RX_PMAC_512_1023),
	OCELOT_STAT(RX_PMAC_1024_1526),
	OCELOT_STAT(RX_PMAC_1527_MAX),
	OCELOT_STAT(RX_PMAC_PAUSE),
	OCELOT_STAT(RX_PMAC_CONTROL),
	OCELOT_STAT(RX_PMAC_LONGS),
	OCELOT_STAT(TX_PMAC_OCTETS),
	OCELOT_STAT(TX_PMAC_UNICAST),
	OCELOT_STAT(TX_PMAC_MULTICAST),
	OCELOT_STAT(TX_PMAC_BROADCAST),
	OCELOT_STAT(TX_PMAC_PAUSE),
	OCELOT_STAT(TX_PMAC_64),
	OCELOT_STAT(TX_PMAC_65_127),
	OCELOT_STAT(TX_PMAC_128_255),
	OCELOT_STAT(TX_PMAC_256_511),
	OCELOT_STAT(TX_PMAC_512_1023),
	OCELOT_STAT(TX_PMAC_1024_1526),
	OCELOT_STAT(TX_PMAC_1527_MAX),
};

I would like to keep the flexibility of defining the counters in this
way without too much regard for:
- the order in which they are listed in enum ocelot_stat
- the order in which they are listed in the struct ocelot_stat_layout array
- the value of the SYS_COUNT_* of their underlying register address

However I'd still like to have a way in which I can index ocelot->stats
when I know specifically what I am looking for.

I think in practice this means that ocelot_prepare_stats_regions() would
need to be modified to first sort the ocelot_stat_layout array by "reg"
value (to keep bulking efficient), and then, I think I'd have to keep to
introduce another array of u32 *ocelot->stat_indices (to keep specific
indexing possible). Then I'd have to go through one extra layer of
indirection; RX_OCTETS would be available at

ocelot->stats[port * OCELOT_NUM_STATS + ocelot->stat_indices[OCELOT_STAT_RX_OCTETS]].

(I can wrap this behind a helper, of course)

This is a bit complicated, but I'm not aware of something simpler that
would do what you want and what I want. What are your thoughts?

Powered by blists - more mailing lists