Added xdp and page pool statistics.
In order to make the implementation simple and compatible, the patch
uses the 32bit integer to record the XDP statistics.
Signed-off-by: Shenwei Wang <[email protected]>
---
Changes in v2:
- clean up and restructure the codes per Andrew Lunn's review comments
- clear the statistics when the adaptor is down
drivers/net/ethernet/freescale/fec.h | 14 ++++
drivers/net/ethernet/freescale/fec_main.c | 85 +++++++++++++++++++++--
2 files changed, 94 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 61e847b18343..5ba1e0d71c68 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -526,6 +526,19 @@ struct fec_enet_priv_txrx_info {
struct sk_buff *skb;
};
+enum {
+ RX_XDP_REDIRECT = 0,
+ RX_XDP_PASS,
+ RX_XDP_DROP,
+ RX_XDP_TX,
+ RX_XDP_TX_ERRORS,
+ TX_XDP_XMIT,
+ TX_XDP_XMIT_ERRORS,
+
+ /* The following must be the last one */
+ XDP_STATS_TOTAL,
+};
+
struct fec_enet_priv_tx_q {
struct bufdesc_prop bd;
unsigned char *tx_bounce[TX_RING_SIZE];
@@ -546,6 +559,7 @@ struct fec_enet_priv_rx_q {
/* page_pool */
struct page_pool *page_pool;
struct xdp_rxq_info xdp_rxq;
+ u32 stats[XDP_STATS_TOTAL];
/* rx queue number, in the range 0-7 */
u8 id;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3fb870340c22..d18e50871c42 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1523,10 +1523,12 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
switch (act) {
case XDP_PASS:
+ rxq->stats[RX_XDP_PASS]++;
ret = FEC_ENET_XDP_PASS;
break;
case XDP_REDIRECT:
+ rxq->stats[RX_XDP_REDIRECT]++;
err = xdp_do_redirect(fep->netdev, xdp, prog);
if (!err) {
ret = FEC_ENET_XDP_REDIR;
@@ -1549,6 +1551,7 @@ fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
fallthrough; /* handle aborts by dropping packet */
case XDP_DROP:
+ rxq->stats[RX_XDP_DROP]++;
ret = FEC_ENET_XDP_CONSUMED;
page = virt_to_head_page(xdp->data);
page_pool_put_page(rxq->page_pool, page, sync, true);
@@ -2659,6 +2662,16 @@ static const struct fec_stat {
#define FEC_STATS_SIZE (ARRAY_SIZE(fec_stats) * sizeof(u64))
+static const char *fec_xdp_stat_strs[XDP_STATS_TOTAL] = {
+ "rx_xdp_redirect", /* RX_XDP_REDIRECT = 0, */
+ "rx_xdp_pass", /* RX_XDP_PASS, */
+ "rx_xdp_drop", /* RX_XDP_DROP, */
+ "rx_xdp_tx", /* RX_XDP_TX, */
+ "rx_xdp_tx_errors", /* RX_XDP_TX_ERRORS, */
+ "tx_xdp_xmit", /* TX_XDP_XMIT, */
+ "tx_xdp_xmit_errors", /* TX_XDP_XMIT_ERRORS, */
+};
+
static void fec_enet_update_ethtool_stats(struct net_device *dev)
{
struct fec_enet_private *fep = netdev_priv(dev);
@@ -2668,6 +2681,40 @@ static void fec_enet_update_ethtool_stats(struct net_device *dev)
fep->ethtool_stats[i] = readl(fep->hwp + fec_stats[i].offset);
}
+static void fec_enet_get_xdp_stats(struct fec_enet_private *fep, u64 *data)
+{
+ u64 xdp_stats[XDP_STATS_TOTAL] = { 0 };
+ struct fec_enet_priv_rx_q *rxq;
+ int i, j;
+
+ for (i = fep->num_rx_queues - 1; i >= 0; i--) {
+ rxq = fep->rx_queue[i];
+
+ for (j = 0; j < XDP_STATS_TOTAL; j++)
+ xdp_stats[j] += rxq->stats[j];
+ }
+
+ memcpy(data, xdp_stats, sizeof(xdp_stats));
+}
+
+static void fec_enet_page_pool_stats(struct fec_enet_private *fep, u64 *data)
+{
+ struct page_pool_stats stats = {};
+ struct fec_enet_priv_rx_q *rxq;
+ int i;
+
+ for (i = fep->num_rx_queues - 1; i >= 0; i--) {
+ rxq = fep->rx_queue[i];
+
+ if (!rxq->page_pool)
+ continue;
+
+ page_pool_get_stats(rxq->page_pool, &stats);
+ }
+
+ page_pool_ethtool_stats_get(data, &stats);
+}
+
static void fec_enet_get_ethtool_stats(struct net_device *dev,
struct ethtool_stats *stats, u64 *data)
{
@@ -2677,6 +2724,12 @@ static void fec_enet_get_ethtool_stats(struct net_device *dev,
fec_enet_update_ethtool_stats(dev);
memcpy(data, fep->ethtool_stats, FEC_STATS_SIZE);
+ data += FEC_STATS_SIZE / sizeof(u64);
+
+ fec_enet_get_xdp_stats(fep, data);
+ data += XDP_STATS_TOTAL;
+
+ fec_enet_page_pool_stats(fep, data);
}
static void fec_enet_get_strings(struct net_device *netdev,
@@ -2685,9 +2738,16 @@ static void fec_enet_get_strings(struct net_device *netdev,
int i;
switch (stringset) {
case ETH_SS_STATS:
- for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
- memcpy(data + i * ETH_GSTRING_LEN,
- fec_stats[i].name, ETH_GSTRING_LEN);
+ for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
+ memcpy(data, fec_stats[i].name, ETH_GSTRING_LEN);
+ data += ETH_GSTRING_LEN;
+ }
+ for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
+ memcpy(data, fec_xdp_stat_strs[i], ETH_GSTRING_LEN);
+ data += ETH_GSTRING_LEN;
+ }
+ page_pool_ethtool_stats_get_strings(data);
+
break;
case ETH_SS_TEST:
net_selftest_get_strings(data);
@@ -2697,9 +2757,14 @@ static void fec_enet_get_strings(struct net_device *netdev,
static int fec_enet_get_sset_count(struct net_device *dev, int sset)
{
+ int count;
+
switch (sset) {
case ETH_SS_STATS:
- return ARRAY_SIZE(fec_stats);
+ count = ARRAY_SIZE(fec_stats) + XDP_STATS_TOTAL;
+ count += page_pool_ethtool_stats_get_count();
+ return count;
+
case ETH_SS_TEST:
return net_selftest_get_count();
default:
@@ -2710,7 +2775,8 @@ static int fec_enet_get_sset_count(struct net_device *dev, int sset)
static void fec_enet_clear_ethtool_stats(struct net_device *dev)
{
struct fec_enet_private *fep = netdev_priv(dev);
- int i;
+ struct fec_enet_priv_rx_q *rxq;
+ int i, j;
/* Disable MIB statistics counters */
writel(FEC_MIB_CTRLSTAT_DISABLE, fep->hwp + FEC_MIB_CTRLSTAT);
@@ -2718,6 +2784,12 @@ static void fec_enet_clear_ethtool_stats(struct net_device *dev)
for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
writel(0, fep->hwp + fec_stats[i].offset);
+ for (i = fep->num_rx_queues - 1; i >= 0; i--) {
+ rxq = fep->rx_queue[i];
+ for (j = 0; j < XDP_STATS_TOTAL; j++)
+ rxq->stats[j] = 0;
+ }
+
/* Don't disable MIB statistics counters */
writel(0, fep->hwp + FEC_MIB_CTRLSTAT);
}
@@ -3084,6 +3156,9 @@ static void fec_enet_free_buffers(struct net_device *ndev)
for (i = 0; i < rxq->bd.ring_size; i++)
page_pool_release_page(rxq->page_pool, rxq->rx_skb_info[i].page);
+ for (i = 0; i < XDP_STATS_TOTAL; i++)
+ rxq->stats[i] = 0;
+
if (xdp_rxq_info_is_reg(&rxq->xdp_rxq))
xdp_rxq_info_unreg(&rxq->xdp_rxq);
page_pool_destroy(rxq->page_pool);
--
2.34.1
Hello,
On Tue, 2022-11-08 at 20:31 -0600, Shenwei Wang wrote:
> @@ -2685,9 +2738,16 @@ static void fec_enet_get_strings(struct net_device *netdev,
> int i;
> switch (stringset) {
> case ETH_SS_STATS:
> - for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
> - memcpy(data + i * ETH_GSTRING_LEN,
> - fec_stats[i].name, ETH_GSTRING_LEN);
> + for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
> + memcpy(data, fec_stats[i].name, ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
> + }
> + for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
> + memcpy(data, fec_xdp_stat_strs[i], ETH_GSTRING_LEN);
> + data += ETH_GSTRING_LEN;
The above triggers a warning:
In function ‘fortify_memcpy_chk’,
inlined from ‘fec_enet_get_strings’ at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
../include/linux/fortify-string.h:413:25: warning: call to ‘__read_overflow2_field’ declared with attribute warning: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
413 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think you can address it changing fec_xdp_stat_strs definition to:
static const char fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] = { // ...
Cheers,
Paolo
> -----Original Message-----
> From: Paolo Abeni <[email protected]>
> Sent: Thursday, November 10, 2022 5:54 AM
> To: Shenwei Wang <[email protected]>; David S. Miller
> <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> Kicinski <[email protected]>
> > case ETH_SS_STATS:
> > - for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
> > - memcpy(data + i * ETH_GSTRING_LEN,
> > - fec_stats[i].name, ETH_GSTRING_LEN);
> > + for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
> > + memcpy(data, fec_stats[i].name, ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
> > + }
> > + for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
> > + memcpy(data, fec_xdp_stat_strs[i], ETH_GSTRING_LEN);
> > + data += ETH_GSTRING_LEN;
>
> The above triggers a warning:
>
> In function ‘fortify_memcpy_chk’,
> inlined from ‘fec_enet_get_strings’
> at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
> ../include/linux/fortify-string.h:413:25: warning: call to ‘__read_overflow2_field’
> declared with attribute warning: detected read beyond size of field (2nd
> parameter); maybe use struct_group()? [-Wattribute-warning]
> 413 | __read_overflow2_field(q_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> I think you can address it changing fec_xdp_stat_strs definition to:
>
> static const char fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] =
That does a problem. How about just change the memcpy to strncpy?
Regards,
Shenwei
> { // ...
>
> Cheers,
>
> Paolo
From: Shenwei Wang <[email protected]>
Date: Thu, 10 Nov 2022 13:29:56 +0000
> > -----Original Message-----
> > From: Paolo Abeni <[email protected]>
> > Sent: Thursday, November 10, 2022 5:54 AM
> > To: Shenwei Wang <[email protected]>; David S. Miller
> > <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> > Kicinski <[email protected]>
> > > case ETH_SS_STATS:
> > > - for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
> > > - memcpy(data + i * ETH_GSTRING_LEN,
> > > - fec_stats[i].name, ETH_GSTRING_LEN);
> > > + for (i = 0; i < ARRAY_SIZE(fec_stats); i++) {
> > > + memcpy(data, fec_stats[i].name, ETH_GSTRING_LEN);
> > > + data += ETH_GSTRING_LEN;
> > > + }
> > > + for (i = 0; i < ARRAY_SIZE(fec_xdp_stat_strs); i++) {
> > > + memcpy(data, fec_xdp_stat_strs[i], ETH_GSTRING_LEN);
> > > + data += ETH_GSTRING_LEN;
> >
> > The above triggers a warning:
> >
> > In function 'fortify_memcpy_chk',
> > inlined from 'fec_enet_get_strings'
> > at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
> > ../include/linux/fortify-string.h:413:25: warning: call to '__read_overflow2_field'
> > declared with attribute warning: detected read beyond size of field (2nd
> > parameter); maybe use struct_group()? [-Wattribute-warning]
> > 413 | __read_overflow2_field(q_size_field, size);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > I think you can address it changing fec_xdp_stat_strs definition to:
> >
> > static const char fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] =
>
> That does a problem. How about just change the memcpy to strncpy?
Don't use a static char array, it would consume more memory than the
current code. Just replace memcpy()s with strscpy().
Why u32 for the stats tho? It will overflow sooner or later. "To
keep it simple and compatible" you can use u64_stats API :)
>
> Regards,
> Shenwei
>
> > { // ...
> >
> > Cheers,
> >
> > Paolo
Thanks,
Olek
> -----Original Message-----
> From: Alexander Lobakin <[email protected]>
> Sent: Thursday, November 10, 2022 10:43 AM
> To: Shenwei Wang <[email protected]>
> Cc: Alexander Lobakin <[email protected]>; Paolo Abeni
> <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> <[email protected]>; Jakub Kicinski <[email protected]>; Alexei
> > > at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
> > > ../include/linux/fortify-string.h:413:25: warning: call to
> '__read_overflow2_field'
> > > declared with attribute warning: detected read beyond size of field
> > > (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> > > 413 | __read_overflow2_field(q_size_field, size);
> > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > I think you can address it changing fec_xdp_stat_strs definition to:
> > >
> > > static const char
> > > fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] =
> >
> > That does a problem. How about just change the memcpy to strncpy?
>
> Don't use a static char array, it would consume more memory than the current
> code. Just replace memcpy()s with strscpy().
>
> Why u32 for the stats tho? It will overflow sooner or later. "To keep it simple
> and compatible" you can use u64_stats API :)
The reason to use u32 here is : 1. It is simple to implement. 2. To follow the same
behavior as the other MAC hardware statistic counters which are all 32bit. 3. I did
investigate the u64_stats API, and think it is still a little expensive here.
Thanks,
Shenwei
>
> >
> > Regards,
> > Shenwei
> >
> > > { // ...
> > >
> > > Cheers,
> > >
> > > Paolo
>
> Thanks,
> Olek
Hi Shenwei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
[also build test ERROR on next-20221110]
[cannot apply to net/master linus/master v6.1-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shenwei-Wang/net-fec-add-xdp-and-page-pool-statistics/20221109-103338
patch link: https://lore.kernel.org/r/20221109023147.242904-1-shenwei.wang%40nxp.com
patch subject: [PATCH v2 RESEND 1/1] net: fec: add xdp and page pool statistics
config: arm64-randconfig-r016-20221110
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 463da45892e2d2a262277b91b96f5f8c05dc25d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/fd502c1d977612cf562038959936bbde7b331685
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Shenwei-Wang/net-fec-add-xdp-and-page-pool-statistics/20221109-103338
git checkout fd502c1d977612cf562038959936bbde7b331685
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/net/ethernet/freescale/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/net/ethernet/freescale/fec_main.c:2744:25: error: variable has incomplete type 'struct page_pool_stats'
struct page_pool_stats stats = {};
^
drivers/net/ethernet/freescale/fec_main.c:2744:9: note: forward declaration of 'struct page_pool_stats'
struct page_pool_stats stats = {};
^
>> drivers/net/ethernet/freescale/fec_main.c:2754:3: error: call to undeclared function 'page_pool_get_stats'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
page_pool_get_stats(rxq->page_pool, &stats);
^
2 errors generated.
vim +2744 drivers/net/ethernet/freescale/fec_main.c
2741
2742 static void fec_enet_page_pool_stats(struct fec_enet_private *fep, u64 *data)
2743 {
> 2744 struct page_pool_stats stats = {};
2745 struct fec_enet_priv_rx_q *rxq;
2746 int i;
2747
2748 for (i = fep->num_rx_queues - 1; i >= 0; i--) {
2749 rxq = fep->rx_queue[i];
2750
2751 if (!rxq->page_pool)
2752 continue;
2753
> 2754 page_pool_get_stats(rxq->page_pool, &stats);
2755 }
2756
2757 page_pool_ethtool_stats_get(data, &stats);
2758 }
2759
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Shenwei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
[also build test ERROR on next-20221110]
[cannot apply to net/master linus/master v6.1-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Shenwei-Wang/net-fec-add-xdp-and-page-pool-statistics/20221109-103338
patch link: https://lore.kernel.org/r/20221109023147.242904-1-shenwei.wang%40nxp.com
patch subject: [PATCH v2 RESEND 1/1] net: fec: add xdp and page pool statistics
config: powerpc-randconfig-r031-20221110
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fd502c1d977612cf562038959936bbde7b331685
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Shenwei-Wang/net-fec-add-xdp-and-page-pool-statistics/20221109-103338
git checkout fd502c1d977612cf562038959936bbde7b331685
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/net/ethernet/freescale/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/net/ethernet/freescale/fec_main.c: In function 'fec_enet_page_pool_stats':
>> drivers/net/ethernet/freescale/fec_main.c:2744:16: error: variable 'stats' has initializer but incomplete type
2744 | struct page_pool_stats stats = {};
| ^~~~~~~~~~~~~~~
>> drivers/net/ethernet/freescale/fec_main.c:2744:32: error: storage size of 'stats' isn't known
2744 | struct page_pool_stats stats = {};
| ^~~~~
>> drivers/net/ethernet/freescale/fec_main.c:2754:17: error: implicit declaration of function 'page_pool_get_stats'; did you mean 'phy_ethtool_get_stats'? [-Werror=implicit-function-declaration]
2754 | page_pool_get_stats(rxq->page_pool, &stats);
| ^~~~~~~~~~~~~~~~~~~
| phy_ethtool_get_stats
drivers/net/ethernet/freescale/fec_main.c:2744:32: warning: unused variable 'stats' [-Wunused-variable]
2744 | struct page_pool_stats stats = {};
| ^~~~~
cc1: some warnings being treated as errors
vim +/stats +2744 drivers/net/ethernet/freescale/fec_main.c
2741
2742 static void fec_enet_page_pool_stats(struct fec_enet_private *fep, u64 *data)
2743 {
> 2744 struct page_pool_stats stats = {};
2745 struct fec_enet_priv_rx_q *rxq;
2746 int i;
2747
2748 for (i = fep->num_rx_queues - 1; i >= 0; i--) {
2749 rxq = fep->rx_queue[i];
2750
2751 if (!rxq->page_pool)
2752 continue;
2753
> 2754 page_pool_get_stats(rxq->page_pool, &stats);
2755 }
2756
2757 page_pool_ethtool_stats_get(data, &stats);
2758 }
2759
--
0-DAY CI Kernel Test Service
https://01.org/lkp
From: Shenwei Wang <[email protected]>
Date: Thu, 10 Nov 2022 21:40:21 +0000
> > -----Original Message-----
> > From: Alexander Lobakin <[email protected]>
> > Sent: Thursday, November 10, 2022 10:43 AM
> > To: Shenwei Wang <[email protected]>
> > Cc: Alexander Lobakin <[email protected]>; Paolo Abeni
> > <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet
> > <[email protected]>; Jakub Kicinski <[email protected]>; Alexei
> > > > at ../drivers/net/ethernet/freescale/fec_main.c:2788:4:
> > > > ../include/linux/fortify-string.h:413:25: warning: call to
> > '__read_overflow2_field'
> > > > declared with attribute warning: detected read beyond size of field
> > > > (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> > > > 413 | __read_overflow2_field(q_size_field, size);
> > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > >
> > > > I think you can address it changing fec_xdp_stat_strs definition to:
> > > >
> > > > static const char
> > > > fec_xdp_stat_strs[XDP_STATS_TOTAL][ETH_GSTRING_LEN] =
> > >
> > > That does a problem. How about just change the memcpy to strncpy?
> >
> > Don't use a static char array, it would consume more memory than the current
> > code. Just replace memcpy()s with strscpy().
> >
> > Why u32 for the stats tho? It will overflow sooner or later. "To keep it simple
> > and compatible" you can use u64_stats API :)
>
> The reason to use u32 here is : 1. It is simple to implement. 2. To follow the same
> behavior as the other MAC hardware statistic counters which are all 32bit. 3. I did
> investigate the u64_stats API, and think it is still a little expensive here.
1) u64_stats_t is not much harder.
2) This only means your HW statistics handling in the driver is
wrong, as every driver which HW has 32-bit counter implements
64-bit containers and a periodic task to take fresh HW numbers
and clear them (so that the full stats are stored in the driver
only).
3) Page Pool stats currently give you much more overhead as they are
pure 64-bit, not u64_stats_t, with no synchronization.
What is your machine and how fast your link is? Just curious, I
never had any serious regressions using u64_stats_t on either
high-end x86_64 servers or low-end MIPS32.
>
> Thanks,
> Shenwei
>
> >
> > >
> > > Regards,
> > > Shenwei
> > >
> > > > { // ...
> > > >
> > > > Cheers,
> > > >
> > > > Paolo
> >
> > Thanks,
> > Olek
Thanks,
Olek
From: Andrew Lunn <[email protected]>
Date: Mon, 14 Nov 2022 14:50:54 +0100
> > What is your machine and how fast your link is?
>
> Some FEC implementations are Fast Ethernet. Others are 1G.
>
> I expect Shenwei is testing on a fast 64 bit machine with 1G, but
> there are slow 32bit machines with Fast ethernet or 1G.
Okay. I can say I have link speed on 1G on MIPS32, even on those
which are 1-core 600 MHz. And when I was adding more driver stats,
all based on u64_stats_t, I couldn't spot any visible regression.
100-150 Kbps maybe?
>
> Andrew
Thanks,
Olek
> What is your machine and how fast your link is?
Some FEC implementations are Fast Ethernet. Others are 1G.
I expect Shenwei is testing on a fast 64 bit machine with 1G, but
there are slow 32bit machines with Fast ethernet or 1G.
Andrew
> -----Original Message-----
> From: Alexander Lobakin <[email protected]>
> Sent: Monday, November 14, 2022 7:57 AM
> To: Andrew Lunn <[email protected]>
> Cc: Alexander Lobakin <[email protected]>; Shenwei Wang
> <[email protected]>; Paolo Abeni <[email protected]>; David S. Miller
> <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> Kicinski <[email protected]>; Alexei Starovoitov <[email protected]>; Daniel
> Borkmann <[email protected]>; Jesper Dangaard Brouer
> <[email protected]>; John Fastabend <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH v2 RESEND 1/1] net: fec: add xdp and page pool
> statistics
>
> Caution: EXT Email
>
> From: Andrew Lunn <[email protected]>
> Date: Mon, 14 Nov 2022 14:50:54 +0100
>
> > > What is your machine and how fast your link is?
> >
> > Some FEC implementations are Fast Ethernet. Others are 1G.
> >
> > I expect Shenwei is testing on a fast 64 bit machine with 1G, but
> > there are slow 32bit machines with Fast ethernet or 1G.
>
> Okay. I can say I have link speed on 1G on MIPS32, even on those which are 1-
> core 600 MHz. And when I was adding more driver stats, all based on
> u64_stats_t, I couldn't spot any visible regression.
> 100-150 Kbps maybe?
>
I did implement a quick version of u64_stats_t counters, and the performance impact
was about 1~3Mbps on the i.MX8QXP which is 1.2GHz ARM64 Dual Core platform, which
is about 1.5% performance decrease.
Regards,
Shenwei
> >
> > Andrew
>
> Thanks,
> Olek
> I did implement a quick version of u64_stats_t counters, and the performance impact
> was about 1~3Mbps on the i.MX8QXP which is 1.2GHz ARM64 Dual Core platform, which
> is about 1.5% performance decrease.
Please post your code.
Which driver did you copy? Maybe you picked a bad example?
Andrew