Messages in this thread | | | Date | Tue, 15 Nov 2022 09:10:57 -0800 | From | Colin Foster <> | 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!
| |