Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 8 Feb 2022 08:07:42 -0800
From:   Colin Foster <colin.foster@in-advantage.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Claudiu Manoil <claudiu.manoil@nxp.com>
Subject: Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for
 stats
Message-ID: <20220208160742.GB4785@euler>
References: <20220208044644.359951-1-colin.foster@in-advantage.com>
 <20220208044644.359951-4-colin.foster@in-advantage.com>
 <20220208150303.afoabx742j4ijry7@skbuf>
 <20220208153449.wyv7xrv4kotji7mb@skbuf>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220208153449.wyv7xrv4kotji7mb@skbuf>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?cp7y+YEbnjVLGf8Qbe2yvOYTwVRzwFPDUOySQfBsC6QYU2ZDe728ldbioxc7?=
 =?us-ascii?Q?bdNJr2xfOzoaodknPbcBWiNf9WhSJZ9L2rmLZOmmGXQHRw8jYYv4NtyEXV8g?=
 =?us-ascii?Q?n+URHkRp8cRfZmLnC+rN5pGiUJNdCT+/Stc6kO/bZ6jL+FGCuF419Rq5zyGM?=
 =?us-ascii?Q?+5cLjN25F5aTkWiRISDqHgKPPj0m11xmIb8MeaOf9Sv/CaOHBEPvJTZLO4Xz?=
 =?us-ascii?Q?mXfVhhvSlwcTHoXfZMNra3Z4slBLb0pT8AW9hPJ7gkPhjgglnmGGdfnv7uWq?=
 =?us-ascii?Q?E/wpwuJQx+bFtGJEr8Q7hXaXehOXWic2odhlYB33VVrh3hSKtHSJHRj9FXN4?=
 =?us-ascii?Q?GyfQ8FC8V/sZ9bGLEdMRyou7UABXLAUZfnGpI7FjGaFwz3KFDjKS5YFLh40a?=
 =?us-ascii?Q?vaJdgR0duDnGpWmhr+MWTrqb9WMtSsNemVoZnfqyULaZrE7r9Gm6C3IAM+IW?=
 =?us-ascii?Q?0Gf/SBj9DOjWKxPL1qlUtYehWZhGdXEnf8K6XpuvngdSbinvgLaPXrbW33Zu?=
 =?us-ascii?Q?w81hI9Q8idFOOK2PHM2JvaJqHdlw4oMFCXXqZFZgVP8iCSZGAHCwKUffDbcu?=
 =?us-ascii?Q?+cN68aZev5AfY5GLnkneas+CXfdz/J97JcK7cUO4vt7EVHozau4uVKPlKsda?=
 =?us-ascii?Q?F7boG3Ys38Tq3yd4GuB7e1MeZOOzK1UnFbN1Xq5T/csXLsRVQdOO7qZ3xc7J?=
 =?us-ascii?Q?n1/73F/vyYa5Enty6E19B0+h6VMCUQ8hKgbfRyJKui3+wU5DCVUkcExztoPU?=
 =?us-ascii?Q?J+377jbnnLY7JQ2vCNOf6i4jcjNDogeBEdf0z9rfAAIv1Zuk3UxjDF8VL8y9?=
 =?us-ascii?Q?Yrfyn6jCWO301CCzKuDpMjezJDdnkiwMYBvCZjT0x37EF5ysXME38h3jVe3m?=
 =?us-ascii?Q?CIfLVhfF6EjbLH89qexFUOeX/1H4YDpdN3Q2iGhCh3U+lzV95I95t4+k0n6g?=
 =?us-ascii?Q?ICYhDke+wzoebKQIPicS0Qzb7Sg0+V1+vVthfEqngybBAKvXPqI0JZ+R12wO?=
 =?us-ascii?Q?i/H+6142RhjNPLp1VGGifXnyxtEhWO3VsNKQUH4ONgwtSEAbjp2TjBSgwUHF?=
 =?us-ascii?Q?0E7vAFciIrsb9CRNzf7FwTvyF4IQ8F+LnVX0IUrRJzViZuCsf511hEpMFIK8?=
 =?us-ascii?Q?QBSTHukija9uYvG3ut6GPjyfwYkJmMZ/T5VCM3nuTxBNGFzkaG/dP+WpEJ1y?=
 =?us-ascii?Q?sQ+UqKyxQJwqg00yMHl7Q+gF/Pqhkt9q432lpmzJXv80hp+Me3BijSrm0YOE?=
 =?us-ascii?Q?ZIBziC99CNjoqdIdasNaJpjyzRayuL5vjIUHiAG2PE/ZLlUdN4a8wl9UdHqL?=
 =?us-ascii?Q?Kd6OEGyDRPH9DvqerhNkQxoDlNJ0KhrCbk23FscINjUhL8EOxApwprOPVIil?=
 =?us-ascii?Q?87WKrrhX1U9dTY81Vso66xcXS38WG9yetMZQozFF51HCZ10nVWkjjAqfBFxV?=
 =?us-ascii?Q?eXJ2aMur3ej2Ed2YBRWbmsMUI9lPd+xlAubWuxC0w4Z2jA97aDXHwZ+7vBZX?=
 =?us-ascii?Q?uvuWmMK37j7PEFBcD7XREj5oeL3DYfIQPXEPv+yrGsKFb8JcwA0flY9J2z3Z?=
 =?us-ascii?Q?WVhFq1TRYd8IRVNSn8suBn9nHyX2UANcvoRG4ZhcpLLO0Co2xRBz3spdGPrf?=
 =?us-ascii?Q?mqMUBYk9d9jctVuMDWG3Hctayr/6xApQFhGNu3fZSiMUx0H2LODm9WpcNxNV?=
 =?us-ascii?Q?/7C3n1grFmTPS/PyapPHDlRM4SA=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c89a6f81-6907-485a-c7bf-08d9eb1d244b
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2022 16:07:44.7348
 (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: ABN0eEOhwDGEOGG0PBSH1UtkGsUuHVdFo31OdU9ruNWJD6knO55Grt9dr57KaAB4u1s0AoNgTAcKhROYBuYR8xbMmd1uuh+eXm0o6W1WEg0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1616
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Feb 08, 2022 at 03:34:49PM +0000, Vladimir Oltean wrote:
> On Tue, Feb 08, 2022 at 05:03:03PM +0200, Vladimir Oltean wrote:
> > >  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> > > +		unsigned int idx = 0;
> > > +
> > 
> > This is a bug which causes ocelot->stats to be overwritten with the
> > statistics of port 0, for all ports. Either move the variable
> > declaration and initialization with 0 in the larger scope (outside the
> > "for" loop), or initialize idx with i * ocelot->num_stats.
> 
> My analysis was slightly incorrect. Somehow I managed to fool myself
> into thinking that you had tested this in a limited scenario, hence the
> reason you didn't notice it's not working. But apparently you didn't
> test with traffic at all.
> 
> So ocelot->stats isn't overwritten with the stats of port 0 for all
> ports. But rather, all ports write into the ocelot->stats space
> dedicated for port 0, effectively overwriting the stats of port 0 with
> the stats of the last port. And no one populates the ocelot->stats space
> for ports [1 .. last]. So no port has good statistics, I don't see a
> circumstance where testing could have misled you.

Both ethtool -S and debugfs show statistics that I'd expect in my test
setup. But yes, the port 0 stats would be especially curious.

EDIT: Just saw your message about these. Yes, port 0 is all messed up:
# ethtool -S eth0 | grep p00 | head
     p00_rx_octets: 13602161426432
     p00_rx_unicast: 4539780431872
     p00_rx_multicast: 9028021256192
     p00_rx_broadcast: 8980776615936
     p00_rx_shorts: 0
     p00_rx_fragments: 0
     p00_rx_jabbers: 0
     p00_rx_crc_align_errs: 0
     p00_rx_sym_errs: 0
     p00_rx_frames_below_65_octets: 4539780431872


(configuration - swp2 plugged into swp3, bridged with stp. swp1 into my
development machine, swp4-7 not up)

# pwd
/sys/class/net
# cat swp[123]/statistics/tx_bytes
44616
44668
52
# cat swp[123]/statistics/rx_bytes
34574
46
73272

# ethtool -S swp1 | head -n 5
NIC statistics:
     tx_packets: 942
     tx_bytes: 48984
     rx_packets: 764
     rx_bytes: 37808
# ethtool -S swp2 | head -n 5
NIC statistics:
     tx_packets: 946
     tx_bytes: 49192
     rx_packets: 1
     rx_bytes: 46
# ethtool -S swp3 | head -n 5
NIC statistics:
     tx_packets: 1
     tx_bytes: 52
     rx_packets: 1699
     rx_bytes: 80658
# ethtool -S swp4 | head -n 5
NIC statistics:
     tx_packets: 0
     tx_bytes: 0
     rx_packets: 0
     rx_bytes: 0
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1520 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
3: swp1@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
4: swp2@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
5: swp3@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
6: swp4@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
7: swp5@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
8: swp6@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
9: swp7@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
10: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff


Clearly my testing wasn't sufficient as there were still issues, but
there were reasonable signs of things working as expected on ports 1-4.