[PATCH] mm/thp: Use conventional format for boolean attributes

[PATCH] mm/thp: Use conventional format for boolean attributes

[Ben Hutchings]

The conventional format for boolean attributes in sysfs is numeric
("0" or "1" followed by new-line).  Any boolean attribute can then be
read and written using a generic function.  Using the strings
"yes [no]", "[yes] no" (read), "yes" and "no" (write) will frustrate
this.

Cc'd to stable in order to change this before many scripts depend on
the current strings.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: stable@kernel.org [2.6.38]
---
 mm/huge_memory.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 113e35c..dc0b3f0 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -244,24 +244,25 @@ static ssize_t single_flag_show(struct kobject *kobj,
 				struct kobj_attribute *attr, char *buf,
 				enum transparent_hugepage_flag flag)
 {
-	if (test_bit(flag, &transparent_hugepage_flags))
-		return sprintf(buf, "[yes] no\n");
-	else
-		return sprintf(buf, "yes [no]\n");
+	return sprintf(buf, "%d\n",
+		       test_bit(flag, &transparent_hugepage_flags));
 }
 static ssize_t single_flag_store(struct kobject *kobj,
 				 struct kobj_attribute *attr,
 				 const char *buf, size_t count,
 				 enum transparent_hugepage_flag flag)
 {
-	if (!memcmp("yes", buf,
-		    min(sizeof("yes")-1, count))) {
+	unsigned long value;
+	char *endp;
+
+	value = simple_strtoul(buf, &endp, 0);
+	if (endp == buf || value > 1)
+		return -EINVAL;
+
+	if (value)
 		set_bit(flag, &transparent_hugepage_flags);
-	} else if (!memcmp("no", buf,
-			   min(sizeof("no")-1, count))) {
+	else
 		clear_bit(flag, &transparent_hugepage_flags);
-	} else
-		return -EINVAL;
 
 	return count;
 }
-- 
1.7.4.1


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[David Rientjes]

On Tue, 22 Mar 2011, Ben Hutchings wrote:

> The conventional format for boolean attributes in sysfs is numeric
> ("0" or "1" followed by new-line).  Any boolean attribute can then be
> read and written using a generic function.  Using the strings
> "yes [no]", "[yes] no" (read), "yes" and "no" (write) will frustrate
> this.
> 
> Cc'd to stable in order to change this before many scripts depend on
> the current strings.
> 

I agree with this in general, it's certainly the standard way of altering 
a boolean tunable throughout the kernel so it would be nice to use the 
same userspace libraries with THP.

Let's cc Andrew on this since it will go through the -mm tree if it's 
merged.

> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: stable@kernel.org [2.6.38]
> ---
>  mm/huge_memory.c |   21 +++++++++++----------
>  1 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 113e35c..dc0b3f0 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -244,24 +244,25 @@ static ssize_t single_flag_show(struct kobject *kobj,
>  				struct kobj_attribute *attr, char *buf,
>  				enum transparent_hugepage_flag flag)
>  {
> -	if (test_bit(flag, &transparent_hugepage_flags))
> -		return sprintf(buf, "[yes] no\n");
> -	else
> -		return sprintf(buf, "yes [no]\n");
> +	return sprintf(buf, "%d\n",
> +		       test_bit(flag, &transparent_hugepage_flags));
>  }
>  static ssize_t single_flag_store(struct kobject *kobj,
>  				 struct kobj_attribute *attr,
>  				 const char *buf, size_t count,
>  				 enum transparent_hugepage_flag flag)
>  {
> -	if (!memcmp("yes", buf,
> -		    min(sizeof("yes")-1, count))) {
> +	unsigned long value;
> +	char *endp;
> +
> +	value = simple_strtoul(buf, &endp, 0);
> +	if (endp == buf || value > 1)
> +		return -EINVAL;
> +
> +	if (value)
>  		set_bit(flag, &transparent_hugepage_flags);
> -	} else if (!memcmp("no", buf,
> -			   min(sizeof("no")-1, count))) {
> +	else
>  		clear_bit(flag, &transparent_hugepage_flags);
> -	} else
> -		return -EINVAL;
>  
>  	return count;
>  }

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Andrew Morton]

On Wed, 13 Apr 2011 12:04:59 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Tue, 22 Mar 2011, Ben Hutchings wrote:
> 
> > The conventional format for boolean attributes in sysfs is numeric
> > ("0" or "1" followed by new-line).  Any boolean attribute can then be
> > read and written using a generic function.  Using the strings
> > "yes [no]", "[yes] no" (read), "yes" and "no" (write) will frustrate
> > this.
> > 
> > Cc'd to stable in order to change this before many scripts depend on
> > the current strings.
> > 
> 
> I agree with this in general, it's certainly the standard way of altering 
> a boolean tunable throughout the kernel so it would be nice to use the 
> same userspace libraries with THP.

yup.

It's a bit naughty to change the existing interface in 2.6.38.x but the time
window is small and few people will be affected and they were nuts to be
using 2.6.38.0 anyway ;)

I suppose we could support both the old and new formats for a while,
then retire the old format but I doubt if it's worth it.

Isn't there some user documentation which needs to be updated to
reflect this change?  If not, why not?  :)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[David Rientjes]

On Wed, 13 Apr 2011, Andrew Morton wrote:

> It's a bit naughty to change the existing interface in 2.6.38.x but the time
> window is small and few people will be affected and they were nuts to be
> using 2.6.38.0 anyway ;)
> 
> I suppose we could support both the old and new formats for a while,
> then retire the old format but I doubt if it's worth it.
> 
> Isn't there some user documentation which needs to be updated to
> reflect this change?  If not, why not?  :)
> 

Indeed there is, in Documentation/vm/transhuge.txt -- only for 
/sys/kernel/mm/transparent_hugepage/khugepaged/defrag, though, we lack 
documentation of debug_cow.

Ben, do you have time to update the patch?  It sounds like this is 2.6.39 
material.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Andrew Morton]

On Tue, 22 Mar 2011 05:45:11 +0000
Ben Hutchings <ben@decadent.org.uk> wrote:

>  static ssize_t single_flag_store(struct kobject *kobj,
>  				 struct kobj_attribute *attr,
>  				 const char *buf, size_t count,
>  				 enum transparent_hugepage_flag flag)
>  {
> -	if (!memcmp("yes", buf,
> -		    min(sizeof("yes")-1, count))) {
> +	unsigned long value;
> +	char *endp;
> +
> +	value = simple_strtoul(buf, &endp, 0);

What the heck is this doing using simple_strtoul()?  checkpatch has
been telling us to use strict_strtoul() for ages, and lately it tells us
to use kstrtoul().

Please review and test asap:

--- a/mm/huge_memory.c~mm-thp-use-conventional-format-for-boolean-attributes-fix
+++ a/mm/huge_memory.c
@@ -247,16 +247,19 @@ static ssize_t single_flag_show(struct k
 	return sprintf(buf, "%d\n",
 		       test_bit(flag, &transparent_hugepage_flags));
 }
+
 static ssize_t single_flag_store(struct kobject *kobj,
 				 struct kobj_attribute *attr,
 				 const char *buf, size_t count,
 				 enum transparent_hugepage_flag flag)
 {
 	unsigned long value;
-	char *endp;
+	int ret;
 
-	value = simple_strtoul(buf, &endp, 0);
-	if (endp == buf || value > 1)
+	ret = kstrtoul(buf, 10, &value);
+	if (ret < 0)
+		return ret;
+	if (value > 1)
 		return -EINVAL;
 
 	if (value)
_

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Andrea Arcangeli]

Hi,

On Wed, Apr 13, 2011 at 12:28:42PM -0700, David Rientjes wrote:
> On Wed, 13 Apr 2011, Andrew Morton wrote:
> 
> > It's a bit naughty to change the existing interface in 2.6.38.x but the time
> > window is small and few people will be affected and they were nuts to be
> > using 2.6.38.0 anyway ;)
> > 
> > I suppose we could support both the old and new formats for a while,
> > then retire the old format but I doubt if it's worth it.
> > 
> > Isn't there some user documentation which needs to be updated to
> > reflect this change?  If not, why not?  :)
> > 
> 
> Indeed there is, in Documentation/vm/transhuge.txt -- only for 
> /sys/kernel/mm/transparent_hugepage/khugepaged/defrag, though, we lack 
> documentation of debug_cow.

Well debug_cow only exists for CONFIG_DEBUG_VM so probably doesn't
need to be documented unless CONFIG_DEBUG_VM is documented in the
first place. It seems production kernels aren't using DEBUG_VM.

> Ben, do you have time to update the patch?  It sounds like this is 2.6.39 
> material.

I think it's fine for 2.6.39. Note that these tweaks are mostly for
debugging too, unless something's bad in compaction one wouldn't need
to tweak those. The only ones to tweak are the khugepaged parameters
and those are integers not booleans.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[NeilBrown]

On Wed, 13 Apr 2011 12:04:59 -0700 (PDT) David Rientjes <rientjes@google.com>
wrote:

> On Tue, 22 Mar 2011, Ben Hutchings wrote:
> 
> > The conventional format for boolean attributes in sysfs is numeric
> > ("0" or "1" followed by new-line).  Any boolean attribute can then be
> > read and written using a generic function.  Using the strings
> > "yes [no]", "[yes] no" (read), "yes" and "no" (write) will frustrate
> > this.
> > 
> > Cc'd to stable in order to change this before many scripts depend on
> > the current strings.
> > 
> 
> I agree with this in general, it's certainly the standard way of altering 
> a boolean tunable throughout the kernel so it would be nice to use the 
> same userspace libraries with THP.
> 
> Let's cc Andrew on this since it will go through the -mm tree if it's 
> merged.
> 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Cc: stable@kernel.org [2.6.38]
> > ---
> >  mm/huge_memory.c |   21 +++++++++++----------
> >  1 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 113e35c..dc0b3f0 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -244,24 +244,25 @@ static ssize_t single_flag_show(struct kobject *kobj,
> >  				struct kobj_attribute *attr, char *buf,
> >  				enum transparent_hugepage_flag flag)
> >  {
> > -	if (test_bit(flag, &transparent_hugepage_flags))
> > -		return sprintf(buf, "[yes] no\n");
> > -	else
> > -		return sprintf(buf, "yes [no]\n");
> > +	return sprintf(buf, "%d\n",
> > +		       test_bit(flag, &transparent_hugepage_flags));

It test bit guaranteed to return 0 or 1?

I think the x86 version returns 0 or -1 (that is from reading the code and
using google to check what 'sbb' does).

Maybe make it "!!test_bit" or even

     strcpy(buf, test_bit(...) ? "1\n" : "0\n");
     return 2;


NeilBrown


> >  }
> >  static ssize_t single_flag_store(struct kobject *kobj,
> >  				 struct kobj_attribute *attr,
> >  				 const char *buf, size_t count,
> >  				 enum transparent_hugepage_flag flag)
> >  {
> > -	if (!memcmp("yes", buf,
> > -		    min(sizeof("yes")-1, count))) {
> > +	unsigned long value;
> > +	char *endp;
> > +
> > +	value = simple_strtoul(buf, &endp, 0);
> > +	if (endp == buf || value > 1)
> > +		return -EINVAL;
> > +
> > +	if (value)
> >  		set_bit(flag, &transparent_hugepage_flags);
> > -	} else if (!memcmp("no", buf,
> > -			   min(sizeof("no")-1, count))) {
> > +	else
> >  		clear_bit(flag, &transparent_hugepage_flags);
> > -	} else
> > -		return -EINVAL;
> >  
> >  	return count;
> >  }
> 
> --
> 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/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Andrea Arcangeli]

On Thu, Apr 14, 2011 at 02:48:07PM +1000, Neil Brown wrote:
> I think the x86 version returns 0 or -1 (that is from reading the code and
> using google to check what 'sbb' does).

It really returns -1...

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Ben Hutchings]

On Thu, Apr 14, 2011 at 04:29:54PM +0200, Andrea Arcangeli wrote:
> On Thu, Apr 14, 2011 at 02:48:07PM +1000, Neil Brown wrote:
> > I think the x86 version returns 0 or -1 (that is from reading the code and
> > using google to check what 'sbb' does).
> 
> It really returns -1...
> 
Sorry, thought I checked this before sending the patch.

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[Andrew Morton]

On Thu, 14 Apr 2011 14:48:07 +1000
NeilBrown <neilb@suse.de> wrote:

> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -244,24 +244,25 @@ static ssize_t single_flag_show(struct kobject *kobj,
> > >  				struct kobj_attribute *attr, char *buf,
> > >  				enum transparent_hugepage_flag flag)
> > >  {
> > > -	if (test_bit(flag, &transparent_hugepage_flags))
> > > -		return sprintf(buf, "[yes] no\n");
> > > -	else
> > > -		return sprintf(buf, "yes [no]\n");
> > > +	return sprintf(buf, "%d\n",
> > > +		       test_bit(flag, &transparent_hugepage_flags));
> 
> It test bit guaranteed to return 0 or 1?
> 
> I think the x86 version returns 0 or -1 (that is from reading the code and
> using google to check what 'sbb' does).

Thanks for catching that.  One wonders how well-tested the patch was!

Speaking of which...

Here's the current status.  Ben, can you please test this soon?

From: Ben Hutchings <ben@decadent.org.uk>

The conventional format for boolean attributes in sysfs is numeric ("0" or
"1" followed by new-line).  Any boolean attribute can then be read and
written using a generic function.  Using the strings "yes [no]", "[yes]
no" (read), "yes" and "no" (write) will frustrate this.

[akpm@linux-foundation.org: use kstrtoul()]
[akpm@linux-foundation.org: test_bit() doesn't return 1/0, per Neil]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Johannes Weiner <jweiner@redhat.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: David Rientjes <rientjes@google.com>
Cc: NeilBrown <neilb@suse.de>
Cc: <stable@kernel.org> 	[2.6.38.x]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/huge_memory.c |   24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff -puN mm/huge_memory.c~mm-thp-use-conventional-format-for-boolean-attributes mm/huge_memory.c
--- a/mm/huge_memory.c~mm-thp-use-conventional-format-for-boolean-attributes
+++ a/mm/huge_memory.c
@@ -244,24 +244,28 @@ static ssize_t single_flag_show(struct k
 				struct kobj_attribute *attr, char *buf,
 				enum transparent_hugepage_flag flag)
 {
-	if (test_bit(flag, &transparent_hugepage_flags))
-		return sprintf(buf, "[yes] no\n");
-	else
-		return sprintf(buf, "yes [no]\n");
+	return sprintf(buf, "%d\n",
+		       !!test_bit(flag, &transparent_hugepage_flags));
 }
+
 static ssize_t single_flag_store(struct kobject *kobj,
 				 struct kobj_attribute *attr,
 				 const char *buf, size_t count,
 				 enum transparent_hugepage_flag flag)
 {
-	if (!memcmp("yes", buf,
-		    min(sizeof("yes")-1, count))) {
+	unsigned long value;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &value);
+	if (ret < 0)
+		return ret;
+	if (value > 1)
+		return -EINVAL;
+
+	if (value)
 		set_bit(flag, &transparent_hugepage_flags);
-	} else if (!memcmp("no", buf,
-			   min(sizeof("no")-1, count))) {
+	else
 		clear_bit(flag, &transparent_hugepage_flags);
-	} else
-		return -EINVAL;
 
 	return count;
 }
_

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/thp: Use conventional format for boolean attributes

[David Rientjes]

On Thu, 14 Apr 2011, Andrew Morton wrote:

> From: Ben Hutchings <ben@decadent.org.uk>
> 
> The conventional format for boolean attributes in sysfs is numeric ("0" or
> "1" followed by new-line).  Any boolean attribute can then be read and
> written using a generic function.  Using the strings "yes [no]", "[yes]
> no" (read), "yes" and "no" (write) will frustrate this.
> 
> [akpm@linux-foundation.org: use kstrtoul()]
> [akpm@linux-foundation.org: test_bit() doesn't return 1/0, per Neil]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Mel Gorman <mel@csn.ul.ie>
> Cc: Johannes Weiner <jweiner@redhat.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: NeilBrown <neilb@suse.de>
> Cc: <stable@kernel.org> 	[2.6.38.x]
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Tested-by: David Rientjes <rientjes@google.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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>