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:   Alexander Lobakin <alexandr.lobakin@intel.com>
To:     Shenwei Wang <shenwei.wang@nxp.com>
Cc:     Alexander Lobakin <alexandr.lobakin@intel.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: Re: [PATCH v3 1/1] net: fec: add xdp and page pool statistics
Date:   Mon, 14 Nov 2022 14:45:42 +0100
Message-Id: <20221114134542.697174-1-alexandr.lobakin@intel.com>
In-Reply-To: <20221111153505.434398-1-shenwei.wang@nxp.com>
References: <20221111153505.434398-1-shenwei.wang@nxp.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Shenwei Wang <shenwei.wang@nxp.com>
Date: Fri, 11 Nov 2022 09:35:05 -0600

> 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 <shenwei.wang@nxp.com>
> Reported-by: kernel test robot <lkp@intel.com>

I was this went upstream[0], I think it was quite premature.
First of all, there was a non-acked reply to me in the v2 thread,
but okay, I can live with that. More serious issues in the inline
comments below.

> ---
>  Changes in v3:
>  - change memcpy to strncpy to fix the warning reported by Paolo Abeni
>  - fix the compile errors on powerpc
> 
>  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/Kconfig    |  1 +
>  drivers/net/ethernet/freescale/fec.h      | 14 ++++
>  drivers/net/ethernet/freescale/fec_main.c | 85 +++++++++++++++++++++--
>  3 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
> index ce866ae3df03..f1e80d6996ef 100644
> --- a/drivers/net/ethernet/freescale/Kconfig
> +++ b/drivers/net/ethernet/freescale/Kconfig
> @@ -29,6 +29,7 @@ config FEC
>  	select CRC32
>  	select PHYLIB
>  	select PAGE_POOL
> +	select PAGE_POOL_STATS

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).

>  	imply NET_SELFTESTS
>  	help
>  	  Say Y here if you want to use the built-in 10/100 Fast ethernet
> 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];

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?

> 
>  	/* rx queue number, in the range 0-7 */
>  	u8 id;

[...]

>  	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++) {
> +			strncpy(data, fec_xdp_stat_strs[i], ETH_GSTRING_LEN);

strncpy() is deprecated in favor of strscpy(), there were tons of
commits which replace the former with the latter across the whole
tree.

> +			data += ETH_GSTRING_LEN;
> +		}
> +		page_pool_ethtool_stats_get_strings(data);
> +
>  		break;
>  	case ETH_SS_TEST:
>  		net_selftest_get_strings(data);

[...]

> +	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;

(not critical) Just memset(&rxq->stats)?

> +	}
> +
>  	/* 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

Could you please send a follow-up maybe, fixing at least that
PAGE_POOL_STATS select and strncpy()?

[0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?h=main&id=6970ef27ff7fd1ce3455b2c696081503d0c0f8ac

Thanks,
Olek