From: Colin Foster <colin.foster@in-advantage.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: netdev@vger.kernel.org, Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Claudiu Manoil <claudiu.manoil@nxp.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
UNGLinuxDriver@microchip.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 1/3] net: mscc: ocelot: fix stats region batching
Date: Mon, 20 Mar 2023 19:14:18 -0700 [thread overview]
Message-ID: <ZBkS+oLNkFAjcDBn@euler> (raw)
In-Reply-To: <20230321010325.897817-2-vladimir.oltean@nxp.com>
On Tue, Mar 21, 2023 at 03:03:23AM +0200, Vladimir Oltean wrote:
> The blamed commit changed struct ocelot_stat_layout :: "u32 offset" to
> "u32 reg".
>
> However, "u32 reg" is not quite a register address, but an enum
> ocelot_reg, which in itself encodes an enum ocelot_target target in the
> upper bits, and an index into the ocelot->map[target][] array in the
> lower bits.
>
> So, whereas the previous code comparison between stats_layout[i].offset
> and last + 1 was correct (because those "offsets" at the time were
> 32-bit relative addresses), the new code, comparing layout[i].reg to
> last + 4 is not correct, because the "reg" here is an enum/index, not an
> actual register address.
>
> What we want to compare are indeed register addresses, but to do that,
> we need to actually go through the same motions as
> __ocelot_bulk_read_ix() itself.
>
> With this bug, all statistics counters are deemed by
> ocelot_prepare_stats_regions() as constituting their own region.
> (Truncated) log on VSC9959 (Felix) below (prints added by me):
>
> Before:
>
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x000]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x001]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x002]
> ...
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x041]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x042]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x080]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x081]
> ...
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x0ac]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x100]
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x101]
> ...
> region of 1 contiguous counters starting with SYS:STAT:CNT[0x111]
>
> After:
>
> region of 67 contiguous counters starting with SYS:STAT:CNT[0x000]
> region of 45 contiguous counters starting with SYS:STAT:CNT[0x080]
> region of 18 contiguous counters starting with SYS:STAT:CNT[0x100]
Yes, I verified this with:
`trace-cmd record -p function_graph -l ocelot_* sleep 3`
Before the patch series, on the VSC7512 a call to
ocelot_port_update_stats() takes about 14ms, with many calls to
ocelot_spi_regmap_bus_read().
After the patch series, the calls take about 2ms, with four calls to
ocelot_spi_regmap_bus_read().
Acked-by: Colin Foster <colin.foster@in-advantage.com>
Tested-by: Colin Foster <colin.foster@in-advantage.com>
next prev parent reply other threads:[~2023-03-21 2:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-21 1:03 [PATCH net 0/3] Fix trainwreck with Ocelot switch statistics counters Vladimir Oltean
2023-03-21 1:03 ` [PATCH net 1/3] net: mscc: ocelot: fix stats region batching Vladimir Oltean
2023-03-21 2:14 ` Colin Foster [this message]
2023-03-21 1:03 ` [PATCH net 2/3] net: mscc: ocelot: fix transfer from region->buf to ocelot->stats Vladimir Oltean
2023-03-21 1:03 ` [PATCH net 3/3] net: mscc: ocelot: add TX_MM_HOLD to ocelot_mm_stats_layout Vladimir Oltean
2023-03-22 4:40 ` [PATCH net 0/3] Fix trainwreck with Ocelot switch statistics counters patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZBkS+oLNkFAjcDBn@euler \
--to=colin.foster@in-advantage.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=claudiu.manoil@nxp.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.