Remove duplicated reference to the 'current' task using a local
variable. Since refering the current can be a burden, it'd better
cache the reference, IMHO. At least this saves some bytes on x86_64.
$ size mempolicy-{old,new}.o
text data bss dec hex filename
25203 2448 9176 36827 8fdb mempolicy-old.o
25136 2448 9184 36768 8fa0 mempolicy-new.o
Signed-off-by: Namhyung Kim <[email protected]>
---
mm/mempolicy.c | 58 +++++++++++++++++++++++++++++--------------------------
1 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 959a8b8c7350..37cc80ce5054 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -304,6 +304,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
enum mpol_rebind_step step)
{
nodemask_t tmp;
+ struct task_struct *curr = current;
if (pol->flags & MPOL_F_STATIC_NODES)
nodes_and(tmp, pol->w.user_nodemask, *nodes);
@@ -335,12 +336,12 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
else
BUG();
- if (!node_isset(current->il_next, tmp)) {
- current->il_next = next_node(current->il_next, tmp);
- if (current->il_next >= MAX_NUMNODES)
- current->il_next = first_node(tmp);
- if (current->il_next >= MAX_NUMNODES)
- current->il_next = numa_node_id();
+ if (!node_isset(curr->il_next, tmp)) {
+ curr->il_next = next_node(curr->il_next, tmp);
+ if (curr->il_next >= MAX_NUMNODES)
+ curr->il_next = first_node(tmp);
+ if (curr->il_next >= MAX_NUMNODES)
+ curr->il_next = numa_node_id();
}
}
@@ -714,7 +715,8 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
nodemask_t *nodes)
{
struct mempolicy *new, *old;
- struct mm_struct *mm = current->mm;
+ struct task_struct *curr = current;
+ struct mm_struct *mm = curr->mm;
NODEMASK_SCRATCH(scratch);
int ret;
@@ -734,22 +736,22 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
*/
if (mm)
down_write(&mm->mmap_sem);
- task_lock(current);
+ task_lock(curr);
ret = mpol_set_nodemask(new, nodes, scratch);
if (ret) {
- task_unlock(current);
+ task_unlock(curr);
if (mm)
up_write(&mm->mmap_sem);
mpol_put(new);
goto out;
}
- old = current->mempolicy;
- current->mempolicy = new;
+ old = curr->mempolicy;
+ curr->mempolicy = new;
mpol_set_task_struct_flag();
if (new && new->mode == MPOL_INTERLEAVE &&
nodes_weight(new->v.nodes))
- current->il_next = first_node(new->v.nodes);
- task_unlock(current);
+ curr->il_next = first_node(new->v.nodes);
+ task_unlock(curr);
if (mm)
up_write(&mm->mmap_sem);
@@ -805,9 +807,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
unsigned long addr, unsigned long flags)
{
int err;
- struct mm_struct *mm = current->mm;
+ struct task_struct *curr = current;
+ struct mm_struct *mm = curr->mm;
struct vm_area_struct *vma = NULL;
- struct mempolicy *pol = current->mempolicy;
+ struct mempolicy *pol = curr->mempolicy;
if (flags &
~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR|MPOL_F_MEMS_ALLOWED))
@@ -817,9 +820,9 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (flags & (MPOL_F_NODE|MPOL_F_ADDR))
return -EINVAL;
*policy = 0; /* just so it's initialized */
- task_lock(current);
+ task_lock(curr);
*nmask = cpuset_current_mems_allowed;
- task_unlock(current);
+ task_unlock(curr);
return 0;
}
@@ -851,9 +854,9 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (err < 0)
goto out;
*policy = err;
- } else if (pol == current->mempolicy &&
+ } else if (pol == curr->mempolicy &&
pol->mode == MPOL_INTERLEAVE) {
- *policy = current->il_next;
+ *policy = curr->il_next;
} else {
err = -EINVAL;
goto out;
@@ -869,7 +872,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
}
if (vma) {
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
vma = NULL;
}
@@ -878,16 +881,16 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (mpol_store_user_nodemask(pol)) {
*nmask = pol->w.user_nodemask;
} else {
- task_lock(current);
+ task_lock(curr);
get_policy_nodemask(pol, nmask);
- task_unlock(current);
+ task_unlock(curr);
}
}
out:
mpol_cond_put(pol);
if (vma)
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
return err;
}
@@ -1912,22 +1915,23 @@ EXPORT_SYMBOL(alloc_pages_current);
/* Slow path of a mempolicy duplicate */
struct mempolicy *__mpol_dup(struct mempolicy *old)
{
+ struct task_struct *curr = current;
struct mempolicy *new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
if (!new)
return ERR_PTR(-ENOMEM);
/* task's mempolicy is protected by alloc_lock */
- if (old == current->mempolicy) {
- task_lock(current);
+ if (old == curr->mempolicy) {
+ task_lock(curr);
*new = *old;
- task_unlock(current);
+ task_unlock(curr);
} else
*new = *old;
rcu_read_lock();
if (current_cpuset_is_being_rebound()) {
- nodemask_t mems = cpuset_mems_allowed(current);
+ nodemask_t mems = cpuset_mems_allowed(curr);
if (new->flags & MPOL_F_REBINDING)
mpol_rebind_policy(new, &mems, MPOL_REBIND_STEP2);
else
--
1.7.4
On Fri, Apr 15, 2011 at 1:33 PM, Namhyung Kim <[email protected]> wrote:
> Remove duplicated reference to the 'current' task using a local
> variable. Since refering the current can be a burden, it'd better
> cache the reference, IMHO. At least this saves some bytes on x86_64.
>
> $ size mempolicy-{old,new}.o
> text data bss dec hex filename
> 25203 2448 9176 36827 8fdb mempolicy-old.o
> 25136 2448 9184 36768 8fa0 mempolicy-new.o
>
> Signed-off-by: Namhyung Kim <[email protected]>
Hi Namhyung,
The patch looks good to me. :)
But I have a nitpick. "curr" is rather awkward to me.
We have been used "tsk" and "p" instead of "curr" for task_struct.
But I don't like "p" since it has no meaning. So for consistency,
could you change it with "tsk"?
Thanks.
--
Kind regards,
Minchan Kim
Remove duplicated reference to the 'current' task using a local
variable. Since refering the current can be a burden, it'd better
cache the reference, IMHO. At least this saves some bytes on x86_64.
$ size mempolicy-{old,new}.o
text data bss dec hex filename
25203 2448 9176 36827 8fdb mempolicy-old.o
25136 2448 9184 36768 8fa0 mempolicy-new.o
Signed-off-by: Namhyung Kim <[email protected]>
---
mm/mempolicy.c | 58 +++++++++++++++++++++++++++++--------------------------
1 files changed, 31 insertions(+), 27 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 959a8b8c7350..5a30065590aa 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -304,6 +304,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
enum mpol_rebind_step step)
{
nodemask_t tmp;
+ struct task_struct *tsk = current;
if (pol->flags & MPOL_F_STATIC_NODES)
nodes_and(tmp, pol->w.user_nodemask, *nodes);
@@ -335,12 +336,12 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
else
BUG();
- if (!node_isset(current->il_next, tmp)) {
- current->il_next = next_node(current->il_next, tmp);
- if (current->il_next >= MAX_NUMNODES)
- current->il_next = first_node(tmp);
- if (current->il_next >= MAX_NUMNODES)
- current->il_next = numa_node_id();
+ if (!node_isset(tsk->il_next, tmp)) {
+ tsk->il_next = next_node(tsk->il_next, tmp);
+ if (tsk->il_next >= MAX_NUMNODES)
+ tsk->il_next = first_node(tmp);
+ if (tsk->il_next >= MAX_NUMNODES)
+ tsk->il_next = numa_node_id();
}
}
@@ -714,7 +715,8 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
nodemask_t *nodes)
{
struct mempolicy *new, *old;
- struct mm_struct *mm = current->mm;
+ struct task_struct *tsk = current;
+ struct mm_struct *mm = tsk->mm;
NODEMASK_SCRATCH(scratch);
int ret;
@@ -734,22 +736,22 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
*/
if (mm)
down_write(&mm->mmap_sem);
- task_lock(current);
+ task_lock(tsk);
ret = mpol_set_nodemask(new, nodes, scratch);
if (ret) {
- task_unlock(current);
+ task_unlock(tsk);
if (mm)
up_write(&mm->mmap_sem);
mpol_put(new);
goto out;
}
- old = current->mempolicy;
- current->mempolicy = new;
+ old = tsk->mempolicy;
+ tsk->mempolicy = new;
mpol_set_task_struct_flag();
if (new && new->mode == MPOL_INTERLEAVE &&
nodes_weight(new->v.nodes))
- current->il_next = first_node(new->v.nodes);
- task_unlock(current);
+ tsk->il_next = first_node(new->v.nodes);
+ task_unlock(tsk);
if (mm)
up_write(&mm->mmap_sem);
@@ -805,9 +807,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
unsigned long addr, unsigned long flags)
{
int err;
- struct mm_struct *mm = current->mm;
+ struct task_struct *tsk = current;
+ struct mm_struct *mm = tsk->mm;
struct vm_area_struct *vma = NULL;
- struct mempolicy *pol = current->mempolicy;
+ struct mempolicy *pol = tsk->mempolicy;
if (flags &
~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR|MPOL_F_MEMS_ALLOWED))
@@ -817,9 +820,9 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (flags & (MPOL_F_NODE|MPOL_F_ADDR))
return -EINVAL;
*policy = 0; /* just so it's initialized */
- task_lock(current);
+ task_lock(tsk);
*nmask = cpuset_current_mems_allowed;
- task_unlock(current);
+ task_unlock(tsk);
return 0;
}
@@ -851,9 +854,9 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (err < 0)
goto out;
*policy = err;
- } else if (pol == current->mempolicy &&
+ } else if (pol == tsk->mempolicy &&
pol->mode == MPOL_INTERLEAVE) {
- *policy = current->il_next;
+ *policy = tsk->il_next;
} else {
err = -EINVAL;
goto out;
@@ -869,7 +872,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
}
if (vma) {
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
vma = NULL;
}
@@ -878,16 +881,16 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
if (mpol_store_user_nodemask(pol)) {
*nmask = pol->w.user_nodemask;
} else {
- task_lock(current);
+ task_lock(tsk);
get_policy_nodemask(pol, nmask);
- task_unlock(current);
+ task_unlock(tsk);
}
}
out:
mpol_cond_put(pol);
if (vma)
- up_read(¤t->mm->mmap_sem);
+ up_read(&mm->mmap_sem);
return err;
}
@@ -1912,22 +1915,23 @@ EXPORT_SYMBOL(alloc_pages_current);
/* Slow path of a mempolicy duplicate */
struct mempolicy *__mpol_dup(struct mempolicy *old)
{
+ struct task_struct *tsk = current;
struct mempolicy *new = kmem_cache_alloc(policy_cache, GFP_KERNEL);
if (!new)
return ERR_PTR(-ENOMEM);
/* task's mempolicy is protected by alloc_lock */
- if (old == current->mempolicy) {
- task_lock(current);
+ if (old == tsk->mempolicy) {
+ task_lock(tsk);
*new = *old;
- task_unlock(current);
+ task_unlock(tsk);
} else
*new = *old;
rcu_read_lock();
if (current_cpuset_is_being_rebound()) {
- nodemask_t mems = cpuset_mems_allowed(current);
+ nodemask_t mems = cpuset_mems_allowed(tsk);
if (new->flags & MPOL_F_REBINDING)
mpol_rebind_policy(new, &mems, MPOL_REBIND_STEP2);
else
--
1.7.4
On Fri, Apr 15, 2011 at 3:08 PM, Namhyung Kim <[email protected]> wrote:
> Remove duplicated reference to the 'current' task using a local
> variable. Since refering the current can be a burden, it'd better
> cache the reference, IMHO. At least this saves some bytes on x86_64.
>
> $ size mempolicy-{old,new}.o
> text data bss dec hex filename
> 25203 2448 9176 36827 8fdb mempolicy-old.o
> 25136 2448 9184 36768 8fa0 mempolicy-new.o
>
> Signed-off-by: Namhyung Kim <[email protected]>
Reviewed-by: Minchan Kim <[email protected]>
--
Kind regards,
Minchan Kim
On Fri, 15 Apr 2011, Namhyung Kim wrote:
> Remove duplicated reference to the 'current' task using a local
> variable. Since refering the current can be a burden, it'd better
> cache the reference, IMHO. At least this saves some bytes on x86_64.
>
> $ size mempolicy-{old,new}.o
> text data bss dec hex filename
> 25203 2448 9176 36827 8fdb mempolicy-old.o
> 25136 2448 9184 36768 8fa0 mempolicy-new.o
>
So this is the opposite of what Andrew did in c06b1fca18c3
(mm/page_alloc.c: don't cache `current' in a local) for the page
allocator?
On Fri, 15 Apr 2011 15:08:08 +0900
Namhyung Kim <[email protected]> wrote:
> Remove duplicated reference to the 'current' task using a local
> variable. Since refering the current can be a burden, it'd better
> cache the reference, IMHO. At least this saves some bytes on x86_64.
>
> $ size mempolicy-{old,new}.o
> text data bss dec hex filename
> 25203 2448 9176 36827 8fdb mempolicy-old.o
> 25136 2448 9184 36768 8fa0 mempolicy-new.o
>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> mm/mempolicy.c | 58 +++++++++++++++++++++++++++++--------------------------
> 1 files changed, 31 insertions(+), 27 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 959a8b8c7350..5a30065590aa 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -304,6 +304,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
> enum mpol_rebind_step step)
> {
> nodemask_t tmp;
> + struct task_struct *tsk = current;
>
> if (pol->flags & MPOL_F_STATIC_NODES)
> nodes_and(tmp, pol->w.user_nodemask, *nodes);
> @@ -335,12 +336,12 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
> else
> BUG();
>
> - if (!node_isset(current->il_next, tmp)) {
> - current->il_next = next_node(current->il_next, tmp);
> - if (current->il_next >= MAX_NUMNODES)
> - current->il_next = first_node(tmp);
> - if (current->il_next >= MAX_NUMNODES)
> - current->il_next = numa_node_id();
> + if (!node_isset(tsk->il_next, tmp)) {
> + tsk->il_next = next_node(tsk->il_next, tmp);
> + if (tsk->il_next >= MAX_NUMNODES)
> + tsk->il_next = first_node(tmp);
> + if (tsk->il_next >= MAX_NUMNODES)
> + tsk->il_next = numa_node_id();
> }
> }
Odd. The new(ish) percpu_read_stable() stuff produces very efficient
code for `current' and usually means that caching `current' in a local
is unneeded, often an overall loss.
So... what is going wrong in mempolicy.c?
> Remove duplicated reference to the 'current' task using a local
> variable. Since refering the current can be a burden, it'd better
> cache the reference, IMHO. At least this saves some bytes on x86_64.
>
> $ size mempolicy-{old,new}.o
> text data bss dec hex filename
> 25203 2448 9176 36827 8fdb mempolicy-old.o
> 25136 2448 9184 36768 8fa0 mempolicy-new.o
>
> Signed-off-by: Namhyung Kim <[email protected]>
But, dense stack usage is also performance good thing. Therefore your
patch benefit is not obvious. I have two request.
1) Please don't increase mess into no hot path. It's no worth.
2) Please mesure performance your box instead size command.
thanks.