Date:   Mon, 20 Mar 2023 19:14:18 -0700
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
Message-ID: <ZBkS+oLNkFAjcDBn@euler>
References: <20230321010325.897817-1-vladimir.oltean@nxp.com>
 <20230321010325.897817-2-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230321010325.897817-2-vladimir.oltean@nxp.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?fr8akqCjz8J358JRtmVFkf4AzGGwYFlPOuSN3a6KAyg8otjbVmla3vDlJlwQ?=
 =?us-ascii?Q?AmOuNzGvS0MlXOtsFiEynx+e/tUWoUbRML7KRDjaOJp/En0FgHmS9pLIu75w?=
 =?us-ascii?Q?tqKf7qzp6/eRQWOCWC/OEUNcKZqHYkBxAehi3GlWTMrUZdw6JWbfX6ujJB8b?=
 =?us-ascii?Q?XGYx5Fbo4Hu44z1mVnHGO1f/pH5PApl+IgloAn9Yc1cCyR3BGJ49d4I+zoJR?=
 =?us-ascii?Q?59H8IKxpqN/4kCQLWX34jFu8/EebfriK8kTQ2IBdf0VlakUIgEb1ahTx+Djo?=
 =?us-ascii?Q?Tf115xHX2DsbO8y4dWkyPftkVJLOGNo+9s8KOnCzZUf+coN7KeZBZuw8WVBN?=
 =?us-ascii?Q?DD5XIXx1kNmlBGnsXGLAtONTRPzRkT91l/o/rNkc8Y5PgCn714qeipwA7xUk?=
 =?us-ascii?Q?VaSaZHeP7HdTaeKgCd3wjm71dcbsXfiQMhsGDpuv4n4qmsGWPV5JlgVoh4yC?=
 =?us-ascii?Q?xBCLwaMVf4EaGS+1HabuEidy4pn6JPduoKJwQMBmbHYwBUW7b6hFAP1hBE7Q?=
 =?us-ascii?Q?GA4kYvDvHncW0hIUxSDC2Ww8jSbINRn7SmH9iHVIxJsF4KtQNfz6MWxsf294?=
 =?us-ascii?Q?7SurXxMAh1D4JF7Eq8gB/2vKsHu2s0KW96xVwXi4NIN4L69wfKXj5mWdYdYY?=
 =?us-ascii?Q?7C9UhQFc8Nwha7m7qDbI/bEgaxmaTn6ho+RT9mc/XfpG7e9/jc3Uhd0rv4Mz?=
 =?us-ascii?Q?fl8sNEvxhLkf04wAzQq0yx+efqwMe1shgr2Ug5xl2jasV70y3x5MsYkoZEbm?=
 =?us-ascii?Q?HLiyvJL9TX/0mOyAodhidBFIIBgnnxKp63lRlOEN+TMCL9R9+4IF0/tMCt8L?=
 =?us-ascii?Q?v40GTN9A9C5NOi5dIFLlpq8TN+EVc9LjqPD6vNLliLznvac8SKBl7xISPxSA?=
 =?us-ascii?Q?qO7pFYP347tGr44cFt/0Q/+vZ4YhIPZHdmPYDi+p8g7fUZY61MBwcU18vlJA?=
 =?us-ascii?Q?A6hateTt7VmyXtZqmEXKIRBYcN2/WI1Qe+BlGdD7BcWGuqq+Z7wsdnA7DzFK?=
 =?us-ascii?Q?r6DCgWAAHj0tQ2x95do4+rsXmZxiGxv1XgxOUfYD7qr8YJ7v5WwsHh1GsVVW?=
 =?us-ascii?Q?ce5ah06QtEpzvXRLNC44PQPQ/bGjJ783O3izlgX1K+rrY9KrdY30N0jl4QkB?=
 =?us-ascii?Q?AAwu2w57EoBz7ocsNXWdWfqhaIu/It3fEeO47z4aQy3Lh9tZdQa3xuaBamCV?=
 =?us-ascii?Q?Pe7vjOo0j/XuA6ZP5Hn+DY6gspmDYQdUT8EBYczrSpgjtFQNgX2iczHCHphA?=
 =?us-ascii?Q?Xh75oLkweusupRWm/q3FdPL7nBKKRa66zvb7MOtqnDOUNcYKw56VCOz0ATxx?=
 =?us-ascii?Q?6TVnOa/ByJrpUrYgkhpSCS1EYDjy/fX7Uu6hwI0y3kAXmABKKcQ//oRmlRhB?=
 =?us-ascii?Q?VUIqRIj/dxYLA41lKVuwOjg/MznC9jCAirddGx+Iiu4P/4rp6XFCbij3aVM8?=
 =?us-ascii?Q?VL2xFmD+PyaylGCZtKgao4Z0YOV2X9U3UyDPZjc+TYA0pQTpPo5STOH5ALxt?=
 =?us-ascii?Q?FIV9CkA1I04izJFqsRV1AktO1dIKzN3QLebMikD/UhSQwcI/oCzDDLkhKfB6?=
 =?us-ascii?Q?JZrrnU8AQBdGmaqk5FpBeB1I4Dl7MpW/tBqW9bC4xWvr3uZxZ6IG7DCYKfag?=
 =?us-ascii?Q?ImAJEEu6njnsTT5jYD1rttc=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6a1f24a6-5aa5-4ad5-2eda-08db29b1fc94
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2023 02:14:22.8608
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qr3euK3eJq691aNUKfKhbziuSNDX0qLxVGA3xHIQWhv7A54i8K3g5H6xdiC3Xs1Q6yejdZODBYohXBiPcKffTtqpxLv6QmTEwaRJoM2miY0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4772
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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>