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:   Wed, 21 Jun 2023 17:23:35 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Jisheng Zhang <jszhang@kernel.org>
Cc:     Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>, netdev@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-sunxi@lists.linux.dev,
        Simon Horman <simon.horman@corigine.com>
Subject: Re: [PATCH net-next v3 2/2] net: stmmac: use pcpu 64 bit statistics
 where necessary
Message-ID: <20230621172335.33cf0dbb@kernel.org>
In-Reply-To: <20230619165220.2501-3-jszhang@kernel.org>
References: <20230619165220.2501-1-jszhang@kernel.org>
        <20230619165220.2501-3-jszhang@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 20 Jun 2023 00:52:20 +0800 Jisheng Zhang wrote:
> +struct stmmac_pcpu_stats {
> +	struct u64_stats_sync syncp;
> +	/* per queue statistics */
> +	struct stmmac_txq_stats txq_stats[MTL_MAX_TX_QUEUES];
> +	struct stmmac_rxq_stats rxq_stats[MTL_MAX_RX_QUEUES];
> +	/* device stats */
> +	u64 rx_packets;
> +	u64 rx_bytes;
> +	u64 tx_packets;
> +	u64 tx_bytes;
> +	/* Tx/Rx IRQ Events */
> +	u64 tx_pkt_n;
> +	u64 rx_pkt_n;
> +	u64 normal_irq_n;
> +	u64 rx_normal_irq_n;
> +	u64 napi_poll;
> +	u64 tx_normal_irq_n;
> +	u64 tx_clean;
> +	u64 tx_set_ic_bit;
> +	/* TSO */
> +	u64 tx_tso_frames;
> +	u64 tx_tso_nfrags;

AFAICT you're using the same structure and syncp for the stats updated
from within IRQ and from xmit and NAPI. That's not safe. The
documentation of u64_stats_sync suggests using _irqsave() variant for
that case but really, I think you should split these stats up.

The statistics which are counting packets / bytes should all go into
respective queue structs, like struct stmmac_tx_queue, and have their
own syncp per context (i.e. separate for xmit and completions if they
can run in parallel).

Having the counters in queue structs is much more common in drivers, 
it usually saves memory and allows reporting stats per queue.

You can keep the per-cpu stats for IRQs if there's no IRQ struct, 
if you prefer. 
-- 
pw-bot: cr