linux-kernel - Re: [PATCH v1 net-next 1/2] net: mscc: ocelot: remove redundant stats

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: <Y3PIIQeGD9LUs2np@colin-ia-desktop>
Date:   Tue, 15 Nov 2022 09:10:57 -0800
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <olteanv@...il.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v1 net-next 1/2] net: mscc: ocelot: remove redundant
 stats_layout pointers

On Tue, Nov 15, 2022 at 04:08:40PM +0000, Vladimir Oltean wrote:
> On Mon, Nov 14, 2022 at 07:43:48PM -0800, Colin Foster wrote:
> > > The issue is that not all Ocelot family switches support the MAC merge
> > > layer. Namely, only vsc9959 does.
> > > 
> > > With your removal of the ability to have a custom per-switch stats layout,
> > > the only remaining thing for vsc9959 to do is to add a "bool mm_supported"
> > > to the common struct ocelot, and all the above extra stats will only be read
> > > from the common code in ocelot_stats.c only if mm_supported is set to true.
> > > 
> > > What do you think, is this acceptable?
> > 
> > That's an interesting solution. I don't really have any strong opinions
> > on this one. I remember we'd had the discussion about making sure the
> > stats are ordered (so that bulk stat reads don't get fragmented) and that
> > wasn't an issue here. So I'm happy to go any route, either:
> 
> Oops, I completely forgot about this patch, which I promised I'd submit
> to net-next and I never did:
> https://patchwork.kernel.org/project/netdevbpf/patch/20220816135352.1431497-7-vladimir.oltean@nxp.com/#24973682
> 
> Would you mind picking it up since you're dealing with stats ATM anyway?

I'll bring that patch into v2 of this set. I plan to get that out late
this week / end.

> 
> > 
> > 1. I fix up this patch and resubmit
> 
> Honestly, I don't quite remember today what I had in mind yesterday with
> "mm_supported" - I'm not sure how that would work. I guess it involves
> creating an extra struct ocelot_stat_layout array beyond ocelot_stats_layout[],
> which would be called ocelot_mm_stats_layout[].
> 
> What you mentioned just above with the stats ordering is going to be a
> problem with this approach, because we'd need to modify ocelot_prepare_stats_regions()
> to construct the regions based on 2 distinct struct ocelot_stat_layout
> arrays, depending on whether ocelot->mm_supported is set (at least that's
> what I believe I was saying yesterday). But if we merge the arrays if
> mm_supported is set, we need to merge them in a sorted way. Complicates
> a lot of things.
> 
> > 2. I wait until the 9959 code lands, and do some tweaks for mac merge stats
> 
> Hmm, waiting for me to do something sounds like a potentially long wait.
> Why do you need to make these changes exactly? To reduce the amount of
> stuff exposed for vsc7512, right?
> 
> > 3. Maybe we deem this patch set unnecessary and drop it, since 9959 will
> > start using custom stats again.
> > 
> > 
> > Or maybe a 4th route, where ocelot->stats_layout remains in tact and
> > felix->info->stats_layout defaults to the common stats. Only the 9959
> > would have to override it?
> 
> Something like that, maybe we could have a helper that is used in
> ocelot_stats.c like this:
> 
> static const struct ocelot_stat_layout *
> ocelot_get_stats_layout(struct ocelot *ocelot)
> {
> 	if (ocelot->stats_layout)
> 		return ocelot->stats_layout;
> 
> 	return ocelot_stats_layout; // common for everyone except VSC9959
> }
> 
> and we keep exposing to the world the OCELOT_COMMON_STATS macro and
> whatever else is needed for VSC9959 to construct its own vsc9959_stats_layout.
> 
> Or..... hmm (sorry, this is a single-pass email, not gonna delete
> anything previous), maybe we could implement the helper function like
> this:
> 
> static const struct ocelot_stat_layout ocelot_stats_layout[OCELOT_NUM_STATS] = {
> 	OCELOT_COMMON_STATS,
> };
> 
> static const struct ocelot_stat_layout ocelot_mm_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),
> };
> 
> static const struct ocelot_stat_layout *
> ocelot_get_stats_layout(struct ocelot *ocelot)
> {
> 	if (ocelot->mm_supported)
> 		return ocelot_mm_stats_layout; // common + MM stats
> 
> 	return ocelot_stats_layout; // just common stats
> }
> 
> Then, setting mm_supported = true from vsc9959 would be enough, no need
> to provide its own stats layout, no need to sort/merge anything.
> 
> How does this sound?

That should work. If there end up being 10 different struct
ocelot_stat_layout[]s, we might reconsider... but in the foreseeable
future there will only be two.

So this applies to patch 2 of my set, which means I'll pretty much keep
it as-is. The get_stats_layout and the ocelot_mm_stats_layout can be
added when the 9959 stuff gets applied.

Thanks for the feedback / suggestions as always!