Re: [PATCH net-next v18 1/4] hinic3: Add ethtool statistic ops part1

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 3 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] hinic3_send_one_skb()
  (drivers/net/ethernet/huawei/hinic3/hinic3_tx.c) reads skb->len for…
- [Medium] The commit is described as a feature (ethtool -S support).
- [Medium] The new TX byte (and packet) accounting in
  hinic3_send_one_skb() adds only skb->len and 1 per skb.
- [Medium] The new RX accounting in hinic3_rx_poll() adds the CQE pkt_len
  to rx_bytes and counts nr_pkts once per completion.

--

hinic3: Add ethtool statistic ops part1

This commit adds per-queue TX/RX software counters plus firmware-backed
vport and physical port statistics to hinic3. It implements the
.get_strings, .get_sset_count and .get_ethtool_stats callbacks for
ethtool -S, and adds packet/byte accounting to the RX and TX hot paths.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> index da73811641a9d..c71bb3dff39c3 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_netdev_ops.c
> @@ -802,14 +802,14 @@ static void hinic3_get_stats64(struct net_device *netdev,
>  		txq_stats = &txq->txq_stats;
>  		do {
>  			start = u64_stats_fetch_begin(&txq_stats->syncp);
> -			bytes += txq_stats->bytes;
> -			packets += txq_stats->packets;
> -			dropped += txq_stats->dropped;
> +			bytes = txq_stats->bytes;
> +			packets = txq_stats->packets;
> +			dropped = txq_stats->dropped;
>  		} while (u64_stats_fetch_retry(&txq_stats->syncp, start));
> +		stats->tx_packets += packets;
> +		stats->tx_bytes   += bytes;
> +		stats->tx_dropped += dropped;
>  	}
> -	stats->tx_packets = packets;
> -	stats->tx_bytes   = bytes;
> -	stats->tx_dropped = dropped;

[Severity: Medium]
The commit message says this patch adds ethtool -S support. This hunk
also changes hinic3_get_stats64() so it no longer accumulates with +=
inside the u64_stats_fetch_begin()/u64_stats_fetch_retry() loop. Before,
every retry added the values again.

The new increments in hinic3_rx_poll() and hinic3_send_one_skb() also
look like the first writes to rxq_stats/txq_stats packets and bytes,
apart from hinic3_rxq_clean_stats() and hinic3_txq_clean_stats().

At the baseline, did hinic3_get_stats64() then always report 0 for RX/TX
packets, bytes and dropped in ip -s link and /proc/net/dev?

If so, hinic3_net_dim() was only passing zeros to DIM:

hinic3_irq.c:hinic3_net_dim() {
    ...
	dim_update_sample(irq_cfg->total_events, rxq->rxq_stats.packets,
			  rxq->rxq_stats.bytes, &sample);
    ...
}

That would mean adaptive RX coalescing never saw any traffic.

The commit message doesn't mention any of this. Could these fixes go to
net as separate patches, with Fixes: tags for the commits that added
.ndo_get_stats64 and DIM, so stable kernels get them?

[ ... ]

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> index 309ab59013797..ae4ccff4a09a9 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c

[ ... ]

> @@ -575,6 +602,7 @@ int hinic3_rx_poll(struct hinic3_rxq *rxq, int budget)
>  			break;
>  
>  		nr_pkts++;
> +		rx_bytes += pkt_len;
>  		num_lro = RQ_CQE_STATUS_GET(status, NUM_LRO);
>  		if (num_lro)
>  			num_wqe += hinic3_get_sge_num(rxq, pkt_len);

[Severity: Medium]
When hardware LRO is on (ethtool -K <dev> lro on), one CQE stands for
num_lro coalesced wire packets. recv_one_pkt() already passes num_lro to
hinic3_lro_set_gso_params(), which sets gso_segs = num_lro.

Are rx_packets and rx_bytes undercounted for LRO aggregates? nr_pkts goes
up by one per completion. pkt_len holds only one copy of the headers, so
the headers of the other num_lro - 1 packets are not counted.

hinic3_get_stats64() reports these values as rx_packets/rx_bytes as they
are. Later in the series they are also reported as qstats
rx-packets/rx-bytes, which netdev.yaml defines as wire packets.

num_lro is already read here for the replenish threshold. Could it be
used for the counters too?

[ ... ]

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> index f92568f0f00b7..b0a0ffb1052b3 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_tx.c

[ ... ]

> @@ -599,11 +617,19 @@ static netdev_tx_t hinic3_send_one_skb(struct sk_buff *skb,
>  	hinic3_write_db(txq->sq, 0, DB_CFLAG_DP_SQ,
>  			hinic3_get_sq_local_pi(txq->sq));
>  
> +	u64_stats_update_begin(&txq->txq_stats.syncp);
> +	txq->txq_stats.bytes += skb->len;
> +	txq->txq_stats.packets += 1;
> +	u64_stats_update_end(&txq->txq_stats.syncp);
> +
>  	return NETDEV_TX_OK;

[Severity: High]
Can the skb already be freed when skb->len is read here?

At this point tx_info->skb is published and hinic3_write_db() has given
the WQE to the device. TX completion runs from NAPI on whichever CPU
handles the queue interrupt. It takes no lock that the xmit path also
takes:

hinic3_poll()
  hinic3_tx_poll()
    hinic3_tx_unmap_skb(netdev, tx_info->skb, tx_info->dma_info);
    napi_consume_skb(tx_info->skb, budget);

Suppose the device completes the packet and another CPU runs
hinic3_tx_poll() before this CPU reaches the stats update. That can
happen if this CPU takes a hardirq, if the vCPU is preempted, or under
PREEMPT_RT. The read of skb->len is then a use-after-free, and a garbage
value is added to tx_bytes.

Before this patch the skb was not touched after the doorbell, because
netif_subqueue_sent() reads skb->len earlier. The code is unchanged at
the end of the series ("hinic3: Add ethtool pause param ops").

Would saving skb->len in a local before the doorbell, next to
netif_subqueue_sent(), and using that local for the stats fix this?

[Severity: Medium]
How are TSO skbs counted here? hinic3_tso() passes gso_size to the
hardware via hinic3_set_tso_info(), so one GSO skb goes out as several
Ethernet frames. skb->len includes the L2/L3/L4 headers only once.

So tx_packets goes up by one per GSO skb rather than once per wire frame.
tx_bytes also misses the headers that are copied into each extra segment.

hinic3_get_stats64() reports these as rtnl_link_stats64
tx_packets/tx_bytes. The UAPI says tx_bytes should count the length of
Ethernet frames excluding the FCS.

Later in the series, "hinic3: Add Add per queue netdev-genl stats" also
reports these counters as qstats tx-packets/tx-bytes, which are defined
as wire packets.

Should this take gso_segs and the copied header length into account, for
example skb->len + (gso_segs - 1) * hdr_len?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792671176.git.gongfan1%40huawei.com