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;
From:   Shenwei Wang <shenwei.wang@nxp.com>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
CC:     Andrew Lunn <andrew@lunn.ch>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        Wei Fang <wei.fang@nxp.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "imx@lists.linux.dev" <imx@lists.linux.dev>,
        kernel test robot <lkp@intel.com>
Subject: RE: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool
 statistics
Thread-Topic: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool
 statistics
Thread-Index: AQHY9eM2DcCGX+tMv0uN92sufUZdVq4+cwAAgAAGTYCAAA76UIAABgiAgABeOBA=
Date:   Mon, 14 Nov 2022 21:17:48 +0000
Message-ID: <PAXPR04MB918589D35F8B10307D4D430E89059@PAXPR04MB9185.eurprd04.prod.outlook.com>
References: <20221111153505.434398-1-shenwei.wang@nxp.com>
 <20221114134542.697174-1-alexandr.lobakin@intel.com>
 <Y3JLz1niXbdVbRH9@lunn.ch>
 <PAXPR04MB91853D935E363E8A7E3ED7BF89059@PAXPR04MB9185.eurprd04.prod.outlook.com>
 <20221114152327.702592-1-alexandr.lobakin@intel.com>
In-Reply-To: <20221114152327.702592-1-alexandr.lobakin@intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xBFHl+V1G4L9DZ9coIzi7w434oYUUNg3lfCQsrDu1DOklYp6qe8GAW+tKnzg?=
 =?us-ascii?Q?+acnuQlkdoXIYFUk7xvdW29t2h54lcFCI17A88Ns2ruUak0oQp/dCbR+MAuQ?=
 =?us-ascii?Q?y4EMHHmUHEbe1jgwWteerDGwqw+ygjXIJD5mTuSb608lNHocbXd4v3E87wFO?=
 =?us-ascii?Q?dWYEUAkyMdhG9eH1Nkwpg7/Lgo4F6MV3/LC0pQanGMbs9DWs+C7n+RH6u+8j?=
 =?us-ascii?Q?5Y/fiOAdkRNB0AS8p2GjuoaiZ9ByRmQfpBVrNtii3txupFE/os5HacDWpTYJ?=
 =?us-ascii?Q?Ozjjev77tVEiiMC0lwkVWk4i5Qya7uvXBAcvRPXZ5LJCZHPdD4GetaBCfx+I?=
 =?us-ascii?Q?ufKwOtx+nftAjrIQaxz+6+Sren3Xy2aCS+3wioWtc+s4WB/S0KW3uuhE4T6R?=
 =?us-ascii?Q?9qF7S8QQm1IYTGVTDoreAFT47vxnK8+ysH1K/DGo4iEiD+MObqPjS6Kv/8lp?=
 =?us-ascii?Q?8DwKKPIiy1SJeuP/KcLyc75cJ4cavOljhT5y9Va6EyPtDTh8hlPWe/BHV6ci?=
 =?us-ascii?Q?R5i0D0qjhFWTu9qzKmhLu8EA71A/2auAtrWvkeYTUeksI7ci7cib4fozyrGF?=
 =?us-ascii?Q?4HYg827Gx0kSm9sNFsMd4IgkLVVd1f5H5vWshH7gqB93BYOITwqMGSXMfFXD?=
 =?us-ascii?Q?EJIK1HK1uyzFlbsWneZgkMedQ78UT47vvlM6x7Dho9IthrBP6m4NZckbEa99?=
 =?us-ascii?Q?4zt69SGgYX04aJlLSac9UjfTGsRi15NofClfzwYcCUoQvRMCzQOoDR8ko+MW?=
 =?us-ascii?Q?sjce99NVJoCPcHk2WSBEMDzXSt9W7MKhxizEXg0qmxuRDlgFnpKIHLwyImeY?=
 =?us-ascii?Q?uVzwXabqgj7aViYIGRiFshDWn5aunYfPZIqbx84YMYFTLTKnniGK7O2loijQ?=
 =?us-ascii?Q?b2rbLQRcenbdomV4fO6gfMSIbYHel/WnrUKexb2uydI6lACFGV/doaxNzxep?=
 =?us-ascii?Q?m3aP8+BPbsEMx8eg9XyQkuIBNKL2jKAyrjflIRzKGO1VPO6cxYaU+N529Cop?=
 =?us-ascii?Q?s7nT1cq6luRV0FIvvdfuY6tH2G+OEhHQzs/X2jF5eAC1KNE/OWxOjtvdQAho?=
 =?us-ascii?Q?yoM7rcm2T8Jg/8v7XYTlB8S6v6SuvtkYRoIn9s7VtJWOVqR0EcVwmTZi9Hv0?=
 =?us-ascii?Q?3KfUGHSJcp3IgMp2Bm7Sh+Tl10LLEhhfyk8KZTQVnWIiVy1Xtu2eNJxrDi2Z?=
 =?us-ascii?Q?oxrFht0wypMuR+rZxRX/awp4DUYGJG/YjlaPneSKxPG8ysWz6AE677xxWgA6?=
 =?us-ascii?Q?bZgxzJlI5AXbGrR1odECbXCWBhpI2Pp1scJNosc3E02Y3BlezFLDC1180r8c?=
 =?us-ascii?Q?kdfe3TNAsUH8hXIqM9TQ67JTK3MPzqrZjIwMwbiEcvDgql7vwjma2BHMwzu/?=
 =?us-ascii?Q?MYy/uQsbVJpHR1xUwKYboXDoKH64+qR9gvv8CtJWEe23Pr44A8oif4SbsNyr?=
 =?us-ascii?Q?F8wIVWNJuWOUoZTzHF2be9jj59kqQQeqOO2dJ3zVyRtVqVSs1z9ICCd194Pn?=
 =?us-ascii?Q?t/Q+b7vtYQv+YaGVln5nt07ZJYlxZlD7Ga0Vdh1r9YUPZfkH8vgMQfF6gLM4?=
 =?us-ascii?Q?iK+sed8v1poJEN+N1QIoDETWA63mEoiV5yivHBK0?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9185.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ebc835c-9c10-4a21-5829-08dac685aed0
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Nov 2022 21:17:49.1092
 (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: eCaMk4etGe/ezQyQBvPmvaRfnN5jNPHwQCXyozec7OEASTtkgQvDm3JkDBBP2TyJ6XD54iB2ThENavVsbhOvsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB9296
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS 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


> -----Original Message-----
> From: Alexander Lobakin <alexandr.lobakin@intel.com>
> Sent: Monday, November 14, 2022 9:23 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Alexander Lobakin <alexandr.lobakin@intel.com>; Andrew Lunn
> <andrew@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann
> <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>; John
> Fastabend <john.fastabend@gmail.com>; Wei Fang <wei.fang@nxp.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev=
;
> kernel test robot <lkp@intel.com>
> Subject: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool statist=
ics
>=20
> Caution: EXT Email
>=20
> From: Shenwei Wang <shenwei.wang@nxp.com>
> Date: Mon, 14 Nov 2022 15:06:04 +0000
>=20
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Monday, November 14, 2022 8:08 AM
> > > To: Alexander Lobakin <alexandr.lobakin@intel.com>
> > > Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller
> > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Alexei
> > > Starovoitov <ast@kernel.org>; Daniel Borkmann
> > > <daniel@iogearbox.net>; Jesper Dangaard Brouer <hawk@kernel.org>;
> > > John Fastabend <john.fastabend@gmail.com>; Wei Fang
> > > <wei.fang@nxp.com>; netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; imx@lists.linux.dev; kernel test robot
> > > <lkp@intel.com>
> > > Subject: [EXT] Re: [PATCH v3 1/1] net: fec: add xdp and page pool
> > > statistics
> > >
> > > Caution: EXT Email
> > >
> > >> Drivers should never select PAGE_POOL_STATS. This Kconfig option
> > >> was made to allow user to choose whether he wants stats or better
> > >> performance on slower systems. It's pure user choice, if something
> > >> doesn't build or link, it must be guarded with
> > >> IS_ENABLED(CONFIG_PAGE_POOL_STATS).
> > >
> > > Given how simple the API is, and the stubs for when
> > > CONFIG_PAGE_POOL_STATS is disabled, i doubt there is any need for the
> driver to do anything.
> > >
> > >>>     struct page_pool *page_pool;
> > >>>     struct xdp_rxq_info xdp_rxq;
> > >>> +   u32 stats[XDP_STATS_TOTAL];
> > >>
> > >> Still not convinced it is okay to deliberately provoke overflows
> > >> here, maybe we need some more reviewers to help us agree on what is
> better?
> > >
> > > You will find that many embedded drivers only have 32 bit hardware
> > > stats and do wrap around. And the hardware does not have atomic read
> > > and clear so you can accumulate into a u64. The FEC is from the
> > > times of MIB 2 ifTable, which only requires 32 bit counters. ifXtable=
 is
> modern compared to the FEC.
> > >
> > > Software counters like this are a different matter. The overhead of
> > > a
> > > u64 on a 32 bit system is probably in the noise, so i think there is
> > > strong argument for using u64.
> >
> > If it is required to support u64 counters, the code logic need to
> > change to record the counter locally per packet, and then update the
> > counters for the fec instance when the napi receive loop is complete.
> > In this way we can reduce the performance overhead.
>=20
> That's how it is usually done in the drivers. You put u32 counters on the=
 stack,
> it's impossible to overflow them in just one NAPI poll cycle. Then, after=
 you're
> done with processing descriptors, you just increment the 64-bit on-ring c=
ounters
> at once.
>=20

Did some testing with the atomic64_t counter, with the following codes to u=
pdate
the u64 counter in the end of every NAPI poll cycle.

@@ -1764,7 +1768,13 @@ fec_enet_rx_queue(struct net_device *ndev, int budge=
t, u16 queue_id)
=20
        if (xdp_result & FEC_ENET_XDP_REDIR)
                xdp_do_flush_map();
+#if 1
+       if (xdp_prog) {
+               int i;
+               for(i =3D 0; i < XDP_STATS_TOTAL; i++)
+                       atomic64_add(xdp_stats[i], &rxq->stats[i]);
+       }
+#endif
        return pkt_received;
 }

With the codes above, the testing result is below:
root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0
 sock0@eth0:0 rxdrop xdp-drv
                   pps            pkts           1.00
rx                 349399         1035008
tx                 0              0

 sock0@eth0:0 rxdrop xdp-drv
                   pps            pkts           1.00
rx                 349407         1384640
tx                 0              0

Without  the atomic_add codes above, the testing result is below:
root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0
 sock0@eth0:0 rxdrop xdp-drv
                   pps            pkts           1.00
rx                 350109         1989130
tx                 0              0

 sock0@eth0:0 rxdrop xdp-drv
                   pps            pkts           1.00
rx                 350425         2339786
tx                 0              0

And regarding the u32 counter solution, the testing result is below:
   root@imx8qxpc0mek:~/bpf# ./xdpsock -i eth0
     sock0@eth0:0 rxdrop xdp-drv
                       pps            pkts           1.00
    rx                 361347         2637796
    tx                 0              0

There are about 10K pkts/s difference here. Do we really want the u64 count=
ers?

Regards,
Shenwei

> >
> > Thanks,
> > Shenwei
> >
> > >
> > >        Andrew
>=20
> Thanks,
> Olek