lkml.org 
[lkml]   [2022]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1 net-next 1/2] net: mscc: ocelot: remove redundant stats_layout pointers
Date
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?

>
> 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?
\
 
 \ /
  Last update: 2022-11-15 17:10    [W:0.095 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site