Re: [PATCH net v4 4/5] amt: make pre-query report drops visible

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The commit message says amt_dev_xmit() now uses
  DEV_STATS_INC() for tx_dropped and tx_errors "so concurrent increments…

--

amt: make pre-query report drops visible

When a gateway drops an IGMP or MLD report because the relay's Membership
Query has not arrived yet, this patch now logs a netdev_dbg() message. It
also stops counting successfully forwarded gateway reports as tx_dropped.
The tx_dropped and tx_errors updates in amt_dev_xmit() now use
DEV_STATS_INC().

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index e466cf815a358..884f27fd84a4a 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -1236,7 +1236,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  		group.ip6 = ipv6_hdr(skb)->daddr;
>  #endif
>  	} else {
> -		dev->stats.tx_errors++;
> +		DEV_STATS_INC(dev, tx_errors);
>  		goto free;
>  	}
>  

[ ... ]

> @@ -1283,8 +1292,7 @@ static netdev_tx_t amt_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  	return NETDEV_TX_OK;
>  free:
>  	dev_kfree_skb(skb);
> -unlock:
> -	dev->stats.tx_dropped++;
> +	DEV_STATS_INC(dev, tx_dropped);
>  	return NETDEV_TX_OK;
>  }

[Severity: Medium]
The commit message says DEV_STATS_INC() is used "so concurrent increments
are not lost". Is that still true while other code in amt.c updates the
same counters with a plain increment?

DEV_STATS_INC() expands to atomic_long_inc(&dev->stats.__tx_dropped).
NET_DEV_STAT() makes __tx_dropped a union alias of the same unsigned long
that stats.tx_dropped++ modifies. If one site uses the atomic increment and
another does a plain read-modify-write on that word, updates can still be
lost.

For tx_dropped, amt_send_igmp_gq() and amt_send_mld_gq() still use the
plain form:

drivers/net/amt.c:amt_send_igmp_gq() {
    ...
	if (amt_send_membership_query(amt, skb, tunnel, false)) {
		amt->dev->stats.tx_dropped++;
		kfree_skb(skb);
	}
}

In relay mode these functions run from RX softirq via
amt_rcv()->amt_request_handler(). A remote gateway's AMT Request is enough
to trigger them. Nothing serializes them with amt_dev_xmit(), which is
LLTX:

CPU1 amt_send_igmp_gq()          CPU0 amt_dev_xmit()
  load tx_dropped (N)
                                   atomic_long_inc() -> N+1
  store N+1

The counter ends up at N+1 instead of N+2.

For tx_errors, plain increments remain in amt_send_discovery() and
amt_send_request() (gateway event work). They also remain in
amt_build_mld_gq() and amt_send_advertisement() (relay RX softirq). For
example:

drivers/net/amt.c:amt_send_request() {
    ...
	if (IS_ERR(rt)) {
		amt->dev->stats.tx_errors++;
		goto out;
	}
    ...
}

Should every tx_dropped and tx_errors writer in amt.c be converted to
DEV_STATS_INC(), so that the statement in the commit message is accurate?
No later patch in the series seems to make this change.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013175407.947606-1-omar%40blockcast.net