From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     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,
        Colin Foster <colin.foster@in-advantage.com>,
        linux-kernel@vger.kernel.org
Subject: [PATCH net 1/3] net: mscc: ocelot: fix stats region batching
Date:   Tue, 21 Mar 2023 03:03:23 +0200
Message-Id: <20230321010325.897817-2-vladimir.oltean@nxp.com>
In-Reply-To: <20230321010325.897817-1-vladimir.oltean@nxp.com>
References: <20230321010325.897817-1-vladimir.oltean@nxp.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?MzhNYJ8B+37rwF62F0ehJZouSujbuNK3Iq5INRtOWMQBIU86wfT6pX9v4hdY?=
 =?us-ascii?Q?zCViCPmhEo4YuAFfclgndfApTt49v8oBz23AhVtVlpCf4mSV4+R94rZgg6Cf?=
 =?us-ascii?Q?d0oa6AyLx021qqBt8lsuh7kajjvDSQbXuvJbarQu8mMke3B4+D2YiHFbCmXV?=
 =?us-ascii?Q?D5xdu2AQ/TCyTv6DzWiIjyvOyfPrYL/0OjGa2vZNHFKtAtVQqKbHjNvyeOuR?=
 =?us-ascii?Q?XcLYvqMvIyG7wlV3OLdT4r/RNzNJoVe/RDUi5rh0SNs7NseqkhurucbhmRlj?=
 =?us-ascii?Q?syz1wlfPz2rLsbMPlVRQsgKzeydH3QZaPjC7KciTKmdUFngrWqIAeutt9MzJ?=
 =?us-ascii?Q?Jt3s+Hp1WmloZyzkGHYmS5Yt0B8DvIUwqsOFpEqDHeQmjo0XEAFe9HmOpCQq?=
 =?us-ascii?Q?+RABvkoFGj3tzh+gAc5tBs77Qo0A50dfq/LjFRwT85aou4cvA2mZJirBU8Px?=
 =?us-ascii?Q?g2t+igx9uniNxl6GnZfDzUuCBsCqnHw9jz3WNFHy59iEM1Q+nfcDoEzVXBKt?=
 =?us-ascii?Q?sNb19ndVu5oEjj5fu/ReaJU4uFPgLafamUvYvLZhroEkFo/6tp/0BkmpWubh?=
 =?us-ascii?Q?4DkVb7LazPOSehkR+JQspvOH2JgBj9jfLTy+D70PTkIHJlPcHNYwkV8iJXT7?=
 =?us-ascii?Q?uzypKEQf3oFfV90qJNta+kudbqv4ZvaiiKPVFOlKH1FdzRQ31+/p6WVD6lVC?=
 =?us-ascii?Q?IuBzDThnHImrnGiycOOfBr3XJ5EmfrYzGWtvAchNJkPgmaKnwUgVFlvu71CN?=
 =?us-ascii?Q?EdNuOHpZTVdMruOlxvJMZkIYpo2z4N+afHvdsFG2dblgsynvL8UPu7fwvJdh?=
 =?us-ascii?Q?gpdVigcEc3qNt0Zy+Zz1TyhfEi7FOTFzB+vAoGCMaqBJmpQ3NA2Ow6Mt3Xr2?=
 =?us-ascii?Q?uWdWUd2il8JJcT5tBqMKhOzn0e1EInou9ORbRdUMfeSRehNguU13ALuFm/GD?=
 =?us-ascii?Q?QHNNfQbkzkdIUBPS/fD253K/wz55CJ08ChROJt89MF6VqTH2YxTJ4u7aTeKT?=
 =?us-ascii?Q?0MRCE/wDg8ewWcbNwd/XdqOmksrdajblWx3GK34qnNGuU+2B4fR8GtBHAO+G?=
 =?us-ascii?Q?PsVsCeD8Atw84gVbxYlLBH4SUBjxXi59X1fDF8KX30MuI9bXVXFfDmewbAJD?=
 =?us-ascii?Q?aopk3tlbxMyj+VTs6hbkMGP+5LEkJWnGLCnBAc8BaC/n+1KPz3x71y517NCp?=
 =?us-ascii?Q?KNpcB+E0NomQtD3ywdymiMn6nG1qVQ9UjIJUebojv2UThqcH7uTT17P3q9Lq?=
 =?us-ascii?Q?H9L1kZovF+axVKzYJsxGtxQ/JE9QIQGyUyerR4RYMoDO34zEp7+LKuWn6BB4?=
 =?us-ascii?Q?7ZQLsOGACOkJMuW+dJgSEfl9qe9haqF0O8Zr6FFBiePTtfutl1yN4wo4uJQ/?=
 =?us-ascii?Q?cXYYDozFIVxKs9ehj9p+at3EPSDRrj2fKTOtilwRmrX68Iu8IDOmFpBhmjCX?=
 =?us-ascii?Q?cvw+joy+ZRBbzSb3WrL8KTbaTbbKqkvUN25GHVuweQizRsqP7Tx1GVZDNKxl?=
 =?us-ascii?Q?VgkJfndlZCfsZ5+aVJhX1da9t/zTRx22SHGAYwAQbTm/N+xReFKguxVl/79a?=
 =?us-ascii?Q?z4V2pQwlFMz660Z9BoXUkiKbulla117bRsnkgqwSx2i/2LgpykxglvtJKmr9?=
 =?us-ascii?Q?IA=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e9765d7b-5e18-43c3-1927-08db29a81b68
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6452.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2023 01:03:39.6296
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: cfWLYgNm/KSxP+v7RNJeSuKedCCT+NPwWesSJUAV4EbTui+oa/eqG+1MtJPX3AGZdMZUSwXVfFU1RTItuXAuTA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB7911
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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]

Since commit d87b1c08f38a ("net: mscc: ocelot: use bulk reads for
stats") intended bulking as a performance improvement, and since now,
with trivial-sized regions, performance is even worse than without
bulking at all, this could easily qualify as a performance regression.

Fixes: d4c367650704 ("net: mscc: ocelot: keep ocelot_stat_layout by reg address, not offset")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot_stats.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index bdb893476832..096c81ec9dd6 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -899,7 +899,8 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
 		if (!layout[i].reg)
 			continue;
 
-		if (region && layout[i].reg == last + 4) {
+		if (region && ocelot->map[SYS][layout[i].reg & REG_MASK] ==
+		    ocelot->map[SYS][last & REG_MASK] + 4) {
 			region->count++;
 		} else {
 			region = devm_kzalloc(ocelot->dev, sizeof(*region),
-- 
2.34.1