Re: [PATCH net-next v2 3/5] net: account per-CPU netdev stats to memcg

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

- [Medium] The accounting change only covers the plain
  `netdev_alloc_pcpu_stats()` form, so per-CPU stats belonging to one…

Pre-existing issues:
- [Low] Concern that the change is a memcg-isolation bug fix and
  therefore lacks a required `Fixes:` tag identifying the commit that…

--

net: account per-CPU netdev stats to memcg

This changes the plain netdev_alloc_pcpu_stats() macro to allocate with
GFP_KERNEL_ACCOUNT instead of GFP_KERNEL, so the per-CPU stats block is
charged to the memcg of the task that created the object.  The devm_ and
explicit-gfp variants of the macro are unchanged.

v1: https://lore.kernel.org/netdev/20260909092645.3105263-1-alishmery18@gmail.com/ (local)

[Severity: Medium]
Does this move the accounting asymmetry rather than remove it?  Several
callers allocate more than one per-CPU stats block for the same object,
and only the ones going through this macro become charged.

net/sched/sch_generic.c:qdisc_alloc() does:

	sch->cpu_bstats =
		netdev_alloc_pcpu_stats(struct gnet_stats_basic_sync);
	if (!sch->cpu_bstats)
		goto errout1;

	sch->cpu_qstats = alloc_percpu(struct gnet_stats_queue);

so cpu_bstats is now charged while cpu_qstats stays uncharged for the same
qdisc created by tc qdisc add.

net/sched/act_api.c:tcf_idr_create() has the same split:

	p->cpu_bstats_hw = netdev_alloc_pcpu_stats(struct gnet_stats_basic_sync);
	if (!p->cpu_bstats_hw)
		goto err2;
	p->cpu_qstats = alloc_percpu(struct gnet_stats_queue);

Two of the three per-CPU blocks of one tc action get charged, the third
does not.

The commit message says:

    The devm_ and explicit-gfp forms of the macro are left alone.

but it does not mention these adjacent alloc_percpu() companions inside the
same functions.  Were they deliberately left out, or just missed?

Related, the same net_device field ends up charged or not depending on which
macro a driver picks: dev->tstats is charged when it comes from
netdev_do_alloc_pcpu_stats() in net/core/dev.c, and uncharged when a driver
uses devm_netdev_alloc_pcpu_stats(), as in liteeth_probe()
(drivers/net/ethernet/litex/litex_liteeth.c), stmmac_dvr_probe()
(drivers/net/ethernet/stmicro/stmmac/stmmac_main.c) and
ax88796c_probe() (drivers/net/ethernet/asix/ax88796c_main.c).

And the user-triggered netlink path in net/ipv6/seg6_local.c still builds
per-CPU SRv6 counters with plain GFP_KERNEL:

	#define seg6_local_alloc_pcpu_counters(__gfp)			\
		__netdev_alloc_pcpu_stats(struct pcpu_seg6_local_counters,	\
					  ((__gfp) | __GFP_ZERO))

	...
	pcounters = seg6_local_alloc_pcpu_counters(GFP_KERNEL);

reached from parse_nla_counters() during route parsing.  Should that wrapper
add __GFP_ACCOUNT too, or is leaving it out intentional?

[Severity: Low]
This isn't a bug introduced by this patch, but the missing accounting in
netdev_alloc_pcpu_stats() is a pre-existing condition, and one review pass
asked whether a Fixes: tag pointing at 3c7be18ac9a0 ("mm: memcg/percpu:
account percpu memory to memory cgroups") is needed here.

Checking the closest precedent argues against it: commit c948f51c1654
("memcg: enable accounting for net_device and Tx/Rx queues"), which
introduced the GFP_KERNEL_ACCOUNT usage in alloc_netdev_mqs() and
netif_alloc_rx_queues() that this patch cites as its parity target, carries
no Fixes: tag and went in through net-next as an enhancement.  3c7be18ac9a0
also never claimed to charge allocations that omit __GFP_ACCOUNT, so it does
not look like a valid Fixes: target.

Is treating this as an enhancement without a Fixes: tag the intent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916193449.2552039-1-alishmery18%40gmail.com