On Tue, Dec 22, 2015 at 03:15:27PM -0800, Andrew Morton wrote:
> On Tue, 22 Dec 2015 15:11:38 -0800 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> > What I have is
>
> And after a bit of reject resolution in
> mm-memcontrol-clean-up-alloc-online-offline-free-functions.patch we
> have
>
>
> static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
> {
> struct mem_cgroup *memcg = mem_cgroup_from_css(css);
>
> if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket)
> static_branch_dec(&memcg_sockets_enabled_key);
>
> vmpressure_cleanup(&memcg->vmpressure);
> cancel_work_sync(&memcg->high_work);
> mem_cgroup_remove_from_trees(memcg);
> memcg_free_kmem(memcg);
>
> if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_active)
> static_branch_dec(&memcg_sockets_enabled_key);
>
> mem_cgroup_free(memcg);
> }
>
> code looks a bit strange. Can we move the static_branch_dec's together
> and run cgroup_subsys_on_dfl just once?
Thanks for fixing it up. I think we can at least put the branches next
to each other. Here is what I have in my local tree:
static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
{
struct mem_cgroup *memcg = mem_cgroup_from_css(css);
if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket)
static_branch_dec(&memcg_sockets_enabled_key);
if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) && memcg->tcpmem_active)
static_branch_dec(&memcg_sockets_enabled_key);
vmpressure_cleanup(&memcg->vmpressure);
cancel_work_sync(&memcg->high_work);
mem_cgroup_remove_from_trees(memcg);
memcg_free_kmem(memcg);
mem_cgroup_free(memcg);
}
However, I don't think turning it into this would be an improvement:
if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) {
if (!cgroup_memory_nosocket)
static_branch_dec(&memcg_sockets_enabled_key);
} else if (memcg->tcpmem_active) {
static_branch_dec(&memcg_sockets_enabled_key);
}
Plus, I'm a little worried that conflating cgroup and cgroup2 blocks
will get us into trouble. Yeah, that code looks a little unusual, but
I can't help but think it's easier to follow the code flow for one
particular mode when the jump labels are always explicit. Then the
brain can easily pattern-match and ignore blocks of the other mode.
It doesn't work the same when we hide keywords in implicit else ifs.
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
