linux-kernel - Re: [PATCH v5 2/3] net: fec: add xdp statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Nov 2022 17:02:15 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Shenwei Wang <shenwei.wang@....com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        imx@...ts.linux.dev, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v5 2/3] net: fec: add xdp statistics

From: Shenwei Wang <shenwei.wang@....com>
Date: Tue, 15 Nov 2022 14:49:50 -0600

> Add xdp statistics for ethtool stats and using u64 to record the xdp counters.
> 
> Signed-off-by: Shenwei Wang <shenwei.wang@....com>
> Reported-by: kernel test robot <lkp@...el.com>

Nit: would be nice if you Cc me for the next submissions as I was
commenting on the previous ones. Just to make sure reviewers won't
miss anything.

> ---
>  drivers/net/ethernet/freescale/fec.h      | 15 +++++
>  drivers/net/ethernet/freescale/fec_main.c | 74 +++++++++++++++++++++--
>  2 files changed, 83 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 61e847b18343..adbe661552be 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,8 @@ struct fec_enet_priv_rx_q {
>  	/* page_pool */
>  	struct page_pool *page_pool;
>  	struct xdp_rxq_info xdp_rxq;
> +	struct u64_stats_sync syncp;
> +	u64 stats[XDP_STATS_TOTAL];

Why `u64`? u64_stats infra declares `u64_stat_t` type and a bunch of
helpers like u64_stats_add(), u64_stats_read() and so on, they will
be solved then by the compiler to the most appropriate ops for the
architecture. So they're more "generic" if you prefer.
Sure, if you show some numbers where `u64_stat_t` is slower than
`u64` on your machine, then okay, but otherwise...

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

[...]

> -- 
> 2.34.1

Thanks,
Olek