Message ID | 20200221014604.126118-1-shakeelb@google.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Series | [v3] cgroup: memcg: net: do not associate sock with unrelated cgroup | expand |
From: Shakeel Butt <shakeelb@google.com> Date: Thu, 20 Feb 2020 17:46:04 -0800 > We are testing network memory accounting in our setup and noticed > inconsistent network memory usage and often unrelated cgroups network > usage correlates with testing workload. On further inspection, it > seems like mem_cgroup_sk_alloc() and cgroup_sk_alloc() are broken in > IRQ context specially for cgroup v1. > > mem_cgroup_sk_alloc() and cgroup_sk_alloc() can be called in IRQ context > and kind of assumes that this can only happen from sk_clone_lock() > and the source sock object has already associated cgroup. However in > cgroup v1, where network memory accounting is opt-in, the source sock > can be unassociated with any cgroup and the new cloned sock can get > associated with unrelated interrupted cgroup. > > Cgroup v2 can also suffer if the source sock object was created by > process in the root cgroup or if sk_alloc() is called in IRQ context. > The fix is to just do nothing in interrupt. > > WARNING: Please note that about half of the TCP sockets are allocated > from the IRQ context, so, memory used by such sockets will not be > accouted by the memcg. Then if we do this then we have to have some kind of subsequent change to attach these sockets to the correct cgroup, right?
On Wed, Feb 26, 2020 at 11:07 AM David Miller <davem@davemloft.net> wrote: > > From: Shakeel Butt <shakeelb@google.com> > Date: Thu, 20 Feb 2020 17:46:04 -0800 > > > We are testing network memory accounting in our setup and noticed > > inconsistent network memory usage and often unrelated cgroups network > > usage correlates with testing workload. On further inspection, it > > seems like mem_cgroup_sk_alloc() and cgroup_sk_alloc() are broken in > > IRQ context specially for cgroup v1. > > > > mem_cgroup_sk_alloc() and cgroup_sk_alloc() can be called in IRQ context > > and kind of assumes that this can only happen from sk_clone_lock() > > and the source sock object has already associated cgroup. However in > > cgroup v1, where network memory accounting is opt-in, the source sock > > can be unassociated with any cgroup and the new cloned sock can get > > associated with unrelated interrupted cgroup. > > > > Cgroup v2 can also suffer if the source sock object was created by > > process in the root cgroup or if sk_alloc() is called in IRQ context. > > The fix is to just do nothing in interrupt. > > > > WARNING: Please note that about half of the TCP sockets are allocated > > from the IRQ context, so, memory used by such sockets will not be > > accouted by the memcg. > > Then if we do this then we have to have some kind of subsequent change > to attach these sockets to the correct cgroup, right? Currently we can potentially charge wrong cgroup. With this patch that will be fixed but potentially half of sockets remain unaccounted. I have a followup (incomplete) patch [1] to fix that. I will send the next version soon. [1] https://lore.kernel.org/linux-mm/20200222010456.40635-1-shakeelb@google.com/
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 9a8a5ded3c48..ef7630cb9749 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -6449,6 +6449,10 @@ void cgroup_sk_alloc(struct sock_cgroup_data *skcd) return; } + /* Don't associate the sock with unrelated interrupted task's cgroup. */ + if (in_interrupt()) + return; + rcu_read_lock(); while (true) { diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 63bb6a2aab81..f500da82bfe8 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -6697,6 +6697,10 @@ void mem_cgroup_sk_alloc(struct sock *sk) return; } + /* Do not associate the sock with unrelated interrupted task's memcg. */ + if (in_interrupt()) + return; + rcu_read_lock(); memcg = mem_cgroup_from_task(current); if (memcg == root_mem_cgroup)