mm/thp: Use conventional format for boolean attributes

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 <[email protected]>

Cc [email protected] [2.6.38]

---
 mm/huge_memory.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

mm/huge_memory.c +10 -6

index 113e35c..dc0b3f0 100644
@@ -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)

Quoted 6 lines · 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.

Quoted 3 lines

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.

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? :)

Quoted 7 lines · Andrew Morton

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.

Quoted 3 lines

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.

Quoted 9 lines · 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.

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.

Quoted 6 lines

Signed-off-by Ben Hutchings <[email protected]>

Cc [email protected] [2.6.38]

---
 mm/huge_memory.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

Quoted 5 lines

mm/huge_memory.c +0 -0

index 113e35c..dc0b3f0 100644

mm/huge_memory.c static ssize_t single_flag_show(struct kobject *kobj, · line 248

Quoted 9 lines

mm/huge_memory.c +2 -4

@@ -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));

Quoted 24 lines

mm/huge_memory.c +9 -6

 }
 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;
 

mm/huge_memory.c static ssize_t single_flag_show(struct kobject *kobj, · line 268

Quoted 2 lines

mm/huge_memory.c +0 -0

 	return count;
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. 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:"[email protected]"> [email protected] </a>

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

Quoted 2 lines · Neil Brown

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...

Quoted 4 lines

Quoted 4 lines

mm/huge_memory.c +0 -0

@@ -244,24 +244,25 @@ static ssize_t single_flag_show(struct kobject *kobj,
 				struct kobj_attribute *attr, char *buf,

Quoted 8 lines

mm/huge_memory.c static ssize_t single_flag_show(struct kobject *kobj, · line 248

Quoted 8 lines

mm/huge_memory.c +2 -4

 				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).

From: Ben Hutchings <[email protected]>

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.

[[email protected]: use kstrtoul()]

[[email protected]: test_bit() doesn't return 1/0, per Neil]

Signed-off-by Ben Hutchings <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Rik van Riel <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc David Rientjes <[email protected]>

Cc NeilBrown <[email protected]>

Cc <[email protected]> [2.6.38.x]

Signed-off-by Andrew Morton <[email protected]>