[PATCH 1/2] mm: remove unnecessary use of atomic

[PATCH 1/2] mm: remove unnecessary use of atomic

[Phil Carmody]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

The bottom 4 hunks are atomically changing memory to which there
are no aliases as it's freshly allocated, so there's no need to
use atomic operations.

The other hunks are just atomic_read and atomic_set, and do not
involve any read-modify-write. The use of atomic_{read,set}
doesn't prevent a read/write or write/write race, so if a race
were possible (I'm not saying one is), then it would still be
there even with atomic_set.

See:
http://digitalvampire.org/blog/index.php/2007/05/13/atomic-cargo-cults/

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
---
 mm/memcontrol.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6c755de..90e32b2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -151,7 +151,7 @@ struct mem_cgroup_threshold {
 
 struct mem_cgroup_threshold_ary {
 	/* An array index points to threshold just below usage. */
-	atomic_t current_threshold;
+	int current_threshold;
 	/* Size of entries[] */
 	unsigned int size;
 	/* Array of thresholds */
@@ -3327,7 +3327,7 @@ static void __mem_cgroup_threshold(struct mem_cgroup *memcg, bool swap)
 	 * If it's not true, a threshold was crossed after last
 	 * call of __mem_cgroup_threshold().
 	 */
-	i = atomic_read(&t->current_threshold);
+	i = t->current_threshold;
 
 	/*
 	 * Iterate backward over array of thresholds starting from
@@ -3351,7 +3351,7 @@ static void __mem_cgroup_threshold(struct mem_cgroup *memcg, bool swap)
 		eventfd_signal(t->entries[i].eventfd, 1);
 
 	/* Update current_threshold */
-	atomic_set(&t->current_threshold, i - 1);
+	t->current_threshold = i - 1;
 unlock:
 	rcu_read_unlock();
 }
@@ -3429,7 +3429,7 @@ static int mem_cgroup_register_event(struct cgroup *cgrp, struct cftype *cft,
 			compare_thresholds, NULL);
 
 	/* Find current threshold */
-	atomic_set(&thresholds_new->current_threshold, -1);
+	thresholds_new->current_threshold = -1;
 	for (i = 0; i < size; i++) {
 		if (thresholds_new->entries[i].threshold < usage) {
 			/*
@@ -3437,7 +3437,7 @@ static int mem_cgroup_register_event(struct cgroup *cgrp, struct cftype *cft,
 			 * until rcu_assign_pointer(), so it's safe to increment
 			 * it here.
 			 */
-			atomic_inc(&thresholds_new->current_threshold);
+			++thresholds_new->current_threshold;
 		}
 	}
 
@@ -3508,7 +3508,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
 	thresholds_new->size = size;
 
 	/* Copy thresholds and find current threshold */
-	atomic_set(&thresholds_new->current_threshold, -1);
+	thresholds_new->current_threshold = -1;
 	for (i = 0, j = 0; i < thresholds->size; i++) {
 		if (thresholds->entries[i].eventfd == eventfd)
 			continue;
@@ -3520,7 +3520,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
 			 * until rcu_assign_pointer(), so it's safe to increment
 			 * it here.
 			 */
-			atomic_inc(&thresholds_new->current_threshold);
+			++thresholds_new->current_threshold;
 		}
 		j++;
 	}
-- 
1.6.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] mm: memcontrol - uninitialised return value

[Phil Carmody]

From: Phil Carmody <ext-phil.2.carmody@nokia.com>

Only an out of memory error will cause ret to be set.

Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 mm/memcontrol.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 90e32b2..09af773 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3464,7 +3464,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
 	int type = MEMFILE_TYPE(cft->private);
 	u64 usage;
 	int size = 0;
-	int i, j, ret;
+	int i, j, ret = 0;
 
 	mutex_lock(&memcg->thresholds_lock);
 	if (type == _MEM)
-- 
1.6.0.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm: memcontrol - uninitialised return value

[KAMEZAWA Hiroyuki]

On Wed,  5 May 2010 14:21:49 +0300
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> Only an out of memory error will cause ret to be set.
> 
> Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm: memcontrol - uninitialised return value

[Andrew Morton]

On Wed,  5 May 2010 14:21:49 +0300
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> Only an out of memory error will cause ret to be set.
> 
> Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> ---
>  mm/memcontrol.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 90e32b2..09af773 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3464,7 +3464,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
>  	int type = MEMFILE_TYPE(cft->private);
>  	u64 usage;
>  	int size = 0;
> -	int i, j, ret;
> +	int i, j, ret = 0;
>  
>  	mutex_lock(&memcg->thresholds_lock);
>  	if (type == _MEM)

afacit the return value of cftype.unregister_event() is always ignored
anyway.  Perhaps it should be changed to void-returning, or fixed.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm: memcontrol - uninitialised return value

[KAMEZAWA Hiroyuki]

On Thu, 6 May 2010 14:24:17 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed,  5 May 2010 14:21:49 +0300
> Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
> 
> > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > 
> > Only an out of memory error will cause ret to be set.
> > 
> > Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> > ---
> >  mm/memcontrol.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 90e32b2..09af773 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3464,7 +3464,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
> >  	int type = MEMFILE_TYPE(cft->private);
> >  	u64 usage;
> >  	int size = 0;
> > -	int i, j, ret;
> > +	int i, j, ret = 0;
> >  
> >  	mutex_lock(&memcg->thresholds_lock);
> >  	if (type == _MEM)
> 
> afacit the return value of cftype.unregister_event() is always ignored
> anyway.  Perhaps it should be changed to void-returning, or fixed.
> 
> 
Ah, it's now "TODO". But hmm...."unregister_event()" is called by workqueue.
(for avoiding race?)

I think unregister_event should be "void" and mem_cgroup_unregister_event()
should be implemented as "never fail" function.

I'll try by myself....but if someone knows this event notifier implementation well,
please.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] mm: memcontrol - uninitialised return value

[Kirill A. Shutemov]

On Fri, May 7, 2010 at 4:07 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Thu, 6 May 2010 14:24:17 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> On Wed,  5 May 2010 14:21:49 +0300
>> Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:
>>
>> > From: Phil Carmody <ext-phil.2.carmody@nokia.com>
>> >
>> > Only an out of memory error will cause ret to be set.
>> >
>> > Acked-by: Kirill A. Shutemov <kirill@shutemov.name>
>> > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
>> > ---
>> >  mm/memcontrol.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> > index 90e32b2..09af773 100644
>> > --- a/mm/memcontrol.c
>> > +++ b/mm/memcontrol.c
>> > @@ -3464,7 +3464,7 @@ static int mem_cgroup_unregister_event(struct cgroup *cgrp, struct cftype *cft,
>> >     int type = MEMFILE_TYPE(cft->private);
>> >     u64 usage;
>> >     int size = 0;
>> > -   int i, j, ret;
>> > +   int i, j, ret = 0;
>> >
>> >     mutex_lock(&memcg->thresholds_lock);
>> >     if (type == _MEM)
>>
>> afacit the return value of cftype.unregister_event() is always ignored
>> anyway.  Perhaps it should be changed to void-returning, or fixed.
>>
>>
> Ah, it's now "TODO". But hmm...."unregister_event()" is called by workqueue.
> (for avoiding race?)

Because it can be called from atomic context.

> I think unregister_event should be "void" and mem_cgroup_unregister_event()
> should be implemented as "never fail" function.
>
> I'll try by myself....but if someone knows this event notifier implementation well,
> please.

Ok, better if I'll do it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] mm: remove unnecessary use of atomic

[KAMEZAWA Hiroyuki]

On Wed,  5 May 2010 14:21:48 +0300
Phil Carmody <ext-phil.2.carmody@nokia.com> wrote:

> From: Phil Carmody <ext-phil.2.carmody@nokia.com>
> 
> The bottom 4 hunks are atomically changing memory to which there
> are no aliases as it's freshly allocated, so there's no need to
> use atomic operations.
> 
> The other hunks are just atomic_read and atomic_set, and do not
> involve any read-modify-write. The use of atomic_{read,set}
> doesn't prevent a read/write or write/write race, so if a race
> were possible (I'm not saying one is), then it would still be
> there even with atomic_set.
> 
> See:
> http://digitalvampire.org/blog/index.php/2007/05/13/atomic-cargo-cults/
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
> Acked-by: Kirill A. Shutemov <kirill@shutemov.name>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>