Re: [PATCH net 2/3] vxlan: vnifilter: account VNI node and per-CPU stats to memcg

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The commit message's quantitative model of the per-CPU charge is
  wrong/incomplete.
- [Low] The commit message states categorically that 'try_charge()
  reclaims and then invokes the memcg OOM killer rather than returning…

--

vxlan: vnifilter: account VNI node and per-CPU stats to memcg

This changes vxlan_vni_alloc() to allocate the struct vxlan_vni_node with
GFP_KERNEL_ACCOUNT and to switch the per-VNI stats block from
netdev_alloc_pcpu_stats() to __netdev_alloc_pcpu_stats() with
GFP_KERNEL_ACCOUNT, so both allocations are charged to the caller's memcg.

Both comments below are about the numbers and the failure behaviour
described in the changelog, not about the code change itself.

> diff --git a/drivers/net/vxlan/vxlan_vnifilter.c b/drivers/net/vxlan/vxlan_vnifilter.c
> index f18ce0e1e741..3d6718ec3f55 100644
> --- a/drivers/net/vxlan/vxlan_vnifilter.c
> +++ b/drivers/net/vxlan/vxlan_vnifilter.c
> @@ -703,10 +703,11 @@ static struct vxlan_vni_node *vxlan_vni_alloc(struct vxlan_dev *vxlan,
>  {
>  	struct vxlan_vni_node *vninode;
>  
> -	vninode = kzalloc_obj(*vninode);
> +	vninode = kzalloc_obj(*vninode, GFP_KERNEL_ACCOUNT);
>  	if (!vninode)
>  		return NULL;
> -	vninode->stats = netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu);
> +	vninode->stats = __netdev_alloc_pcpu_stats(struct vxlan_vni_stats_pcpu,
> +						   GFP_KERNEL_ACCOUNT);
>  	if (!vninode->stats) {
>  		kfree(vninode);
>  		return NULL;

[Severity: Low]
Is the stated cost model for this per-CPU allocation complete?  The
changelog says:

    "plus 64 bytes per possible CPU for the stats block. The per-CPU term
     is the one that grows: 256 bytes per VNI on a 2-CPU host, but 4.2 KB
     per VNI on a 64-CPU one."

Once __GFP_ACCOUNT is set, the amount charged comes from
pcpu_obj_full_size() in mm/percpu-internal.h:

	size_t extra_size = 0;

	#ifdef CONFIG_MEMCG
		if (!mem_cgroup_kmem_disabled())
			extra_size += size / PCPU_MIN_ALLOC_SIZE * sizeof(struct obj_cgroup *);
	#endif

		return size * num_possible_cpus() + extra_size;

For a 64-byte object on a 64-bit build that extra term is 64 / 4 * 8 =
128 bytes, which is what makes the quoted measurements add up
(64 * 2 + 128 = 256, 64 * 64 + 128 = 4224).  Would it be clearer to state
the obj_cgroup metadata term alongside the per-CPU term, since the stated
formula and the stated measurements otherwise disagree?

Also, is the 64-byte figure architecture independent?  struct
vxlan_vni_stats_pcpu in include/net/vxlan.h embeds a u64_stats_sync:

	struct vxlan_vni_stats_pcpu {
		struct vxlan_vni_stats stats;
		struct u64_stats_sync syncp;
	};

That member is empty on 64-bit but carries a seqcount on 32-bit, so the
object is larger than 64 bytes there.

[Severity: Low]
The changelog states:

    "try_charge() reclaims and then invokes the memcg OOM killer rather
     than returning -ENOMEM, so the request does not fail gracefully, the
     caller is killed"

Is that categorical?  In try_charge_memcg() in mm/memcontrol.c, when
mem_cgroup_oom() returns false (no killable task, oom_lock bypass, an
existing OOM victim with MMF_OOM_SKIP, or task_in_memcg_oom(current)),
control reaches:

	nomem:
		if (!(gfp_mask & (__GFP_NOFAIL | __GFP_HIGH)))
			return -ENOMEM;

GFP_KERNEL_ACCOUNT carries neither __GFP_NOFAIL nor __GFP_HIGH, so the
charge can return -ENOMEM, vxlan_vni_alloc() then returns NULL and the
netlink request does fail with -ENOMEM rather than the caller being
killed.

Separately, when the memcg OOM killer does run it picks a victim inside
the constrained cgroup, which need not be the allocating task, and
memory.oom.group can extend the kill to the whole cgroup.

Would it be more accurate to say the add may trigger memcg reclaim and
then either a memcg OOM kill of one or more tasks in the cgroup, or a
plain -ENOMEM?

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