linux-kernel - Re: [PATCH 2/4] cpuset: Remove unneeded NODEMASK_ALLOC() in cpuset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTimRH=LVRLnajbtL3a8FwKkbEfLspAHXXeQLUY8=@mail.gmail.com>
Date:	Thu, 17 Feb 2011 15:46:19 -0800
From:	Paul Menage <menage@...gle.com>
To:	Li Zefan <lizf@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	David Rientjes <rientjes@...gle.com>,
	缪 勰 <miaox@...fujitsu.com>,
	linux-mm@...ck.org
Subject: Re: [PATCH 2/4] cpuset: Remove unneeded NODEMASK_ALLOC() in cpuset_attch()

On Wed, Feb 16, 2011 at 5:49 PM, Li Zefan <lizf@...fujitsu.com> wrote:
> oldcs->mems_allowed is not modified during cpuset_attch(), so
> we don't have to copy it to a buffer allocated by NODEMASK_ALLOC().
> Just pass it to cpuset_migrate_mm().
>
> Signed-off-by: Li Zefan <lizf@...fujitsu.com>

I'd be inclined to skip this one - we're already allocating one
nodemask, so one more isn't really any extra complexity, and we're
doing horrendously complicated stuff in cpuset_migrate_mm() that's
much more likely to fail in low-memory situations.

It's true that mems_allowed can't change during the call to
cpuset_attach(), but that's due to the fact that both cgroup_attach()
and the cpuset.mems write paths take cgroup_mutex. I might prefer to
leave the allocated nodemask here and wrap callback_mutex around the
places in cpuset_attach() where we're reading from a cpuset's
mems_allowed - that would remove the implicit synchronization via
cgroup_mutex and leave the code a little more understandable.

> ---
>  kernel/cpuset.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
> index f13ff2e..70c9ca2 100644
> --- a/kernel/cpuset.c
> +++ b/kernel/cpuset.c
> @@ -1438,10 +1438,9 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>        struct mm_struct *mm;
>        struct cpuset *cs = cgroup_cs(cont);
>        struct cpuset *oldcs = cgroup_cs(oldcont);
> -       NODEMASK_ALLOC(nodemask_t, from, GFP_KERNEL);
>        NODEMASK_ALLOC(nodemask_t, to, GFP_KERNEL);
>
> -       if (from == NULL || to == NULL)
> +       if (to == NULL)
>                goto alloc_fail;
>
>        if (cs == &top_cpuset) {
> @@ -1463,18 +1462,16 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>        }
>
>        /* change mm; only needs to be done once even if threadgroup */
> -       *from = oldcs->mems_allowed;
>        *to = cs->mems_allowed;
>        mm = get_task_mm(tsk);
>        if (mm) {
>                mpol_rebind_mm(mm, to);
>                if (is_memory_migrate(cs))
> -                       cpuset_migrate_mm(mm, from, to);
> +                       cpuset_migrate_mm(mm, &oldcs->mems_allowed, to);
>                mmput(mm);
>        }
>
>  alloc_fail:
> -       NODEMASK_FREE(from);
>        NODEMASK_FREE(to);
>  }
>
> --
> 1.7.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/