[PATCH -next v3 0/3] Add str_true_false()/str_false_true() helper

[PATCH -next v3 0/3] Add str_true_false()/str_false_true() helper

[Hongbo Li]

Add str_true_false()/str_false_true() helper to "true" or "false"
string literal. And we found more than 10 cases currently exist
in the tree. So these helpers can be used for these cases.

Patch 1: Introduce the str_true_false()/str_false_true() helper
Patch 2: Use case for str_true_false.
Patch 3: Use case for str_false_true.

v3:
 - Add use cases for these new helpers.

v2: https://lore.kernel.org/all/2024082407-shortlist-ultimate-2102@gregkh/T/#m0222c320c5395252b3f26f57d99b05509a58b74e
 - Squash into a single patch as Andy Shevchenko' suggesstion.

v1: https://lore.kernel.org/all/1deb2bc4-0cd1-41a0-9434-65c02eef77ed@huawei.com/T/

Hongbo Li (3):
  lib/string_choices: Add str_true_false()/str_false_true() helper
  mm: make use of str_true_false helper
  nfs make use of str_false_true helper

 fs/nfs/nfs4xdr.c               | 11 +++++------
 include/linux/string_choices.h |  6 ++++++
 mm/memory-tiers.c              |  3 +--
 3 files changed, 12 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Hongbo Li]

Add str_true_false()/str_false_true() helper to return "true" or
"false" string literal.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 include/linux/string_choices.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h
index 1320bcdcb89c..ebcc56b28ede 100644
--- a/include/linux/string_choices.h
+++ b/include/linux/string_choices.h
@@ -48,6 +48,12 @@ static inline const char *str_up_down(bool v)
 }
 #define str_down_up(v)		str_up_down(!(v))
 
+static inline const char *str_true_false(bool v)
+{
+	return v ? "true" : "false";
+}
+#define str_false_true(v)		str_true_false(!(v))
+
 /**
  * str_plural - Return the simple pluralization based on English counts
  * @num: Number used for deciding pluralization
-- 
2.34.1

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Andrew Morton]

On Tue, 27 Aug 2024 10:45:15 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:

> Add str_true_false()/str_false_true() helper to return "true" or
> "false" string literal.
> 
> ...
>
> --- a/include/linux/string_choices.h
> +++ b/include/linux/string_choices.h
> @@ -48,6 +48,12 @@ static inline const char *str_up_down(bool v)
>  }
>  #define str_down_up(v)		str_up_down(!(v))
>  
> +static inline const char *str_true_false(bool v)
> +{
> +	return v ? "true" : "false";
> +}
> +#define str_false_true(v)		str_true_false(!(v))
> +
>  /**
>   * str_plural - Return the simple pluralization based on English counts
>   * @num: Number used for deciding pluralization

This might result in copies of the strings "true" and "false" being
generated for every .c file which uses this function, resulting in
unnecessary bloat.

It's possible that the compiler/linker can eliminate this duplication. 
If not, I suggest that every function in string_choices.h be uninlined.

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Hongbo Li]

On 2024/8/28 7:42, Andrew Morton wrote:
> On Tue, 27 Aug 2024 10:45:15 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:
> 
>> Add str_true_false()/str_false_true() helper to return "true" or
>> "false" string literal.
>>
>> ...
>>
>> --- a/include/linux/string_choices.h
>> +++ b/include/linux/string_choices.h
>> @@ -48,6 +48,12 @@ static inline const char *str_up_down(bool v)
>>   }
>>   #define str_down_up(v)		str_up_down(!(v))
>>   
>> +static inline const char *str_true_false(bool v)
>> +{
>> +	return v ? "true" : "false";
>> +}
>> +#define str_false_true(v)		str_true_false(!(v))
>> +
>>   /**
>>    * str_plural - Return the simple pluralization based on English counts
>>    * @num: Number used for deciding pluralization
> 
> This might result in copies of the strings "true" and "false" being
> generated for every .c file which uses this function, resulting in
> unnecessary bloat.
> 
> It's possible that the compiler/linker can eliminate this duplication.
> If not, I suggest that every function in string_choices.h be uninlined.
The inline function is in header file, it will cause code expansion. It 
should avoid the the copies of the strings.

Thanks,
Hongbo

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Andrew Morton]

On Wed, 28 Aug 2024 09:48:21 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:

> > This might result in copies of the strings "true" and "false" being
> > generated for every .c file which uses this function, resulting in
> > unnecessary bloat.
> > 
> > It's possible that the compiler/linker can eliminate this duplication.
> > If not, I suggest that every function in string_choices.h be uninlined.
> The inline function is in header file, it will cause code expansion. It 
> should avoid the the copies of the strings.

Sorry, I don't understand your reply.

Anything which is calling these functions is not performance-sensitive,
so optimizing for space is preferred.  An out-of-line function which
returns a const char * will achieve this?

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Hongbo Li]

On 2024/8/28 11:22, Andrew Morton wrote:
> On Wed, 28 Aug 2024 09:48:21 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:
> 
>>> This might result in copies of the strings "true" and "false" being
>>> generated for every .c file which uses this function, resulting in
>>> unnecessary bloat.
>>>
>>> It's possible that the compiler/linker can eliminate this duplication.
>>> If not, I suggest that every function in string_choices.h be uninlined.
>> The inline function is in header file, it will cause code expansion. It
>> should avoid the the copies of the strings.
> 
> Sorry, I don't understand your reply.
> 
I mean this is a inline function (and tiny enough), the compiler will do 
the code expansion and some optimizations.
> Anything which is calling these functions is not performance-sensitive,
> so optimizing for space is preferred.  An out-of-line function which
> returns a const char * will achieve this?
I think this helper can achieve this. Because it is tiny enough, the 
compiler will handle this like #define macro (do the replacement) 
without allocating extra functional stack. On the contrary, if it is 
implemented as a non-inline function, it will cause extra functional 
stack when it was called every time. And it also should be implemented 
in a source file (.c file), not in header file(.h file).

Thanks,
Hongbo

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Andrew Morton]

On Wed, 28 Aug 2024 11:49:51 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:

> > Anything which is calling these functions is not performance-sensitive,
> > so optimizing for space is preferred.  An out-of-line function which
> > returns a const char * will achieve this?
> I think this helper can achieve this. Because it is tiny enough, the 
> compiler will handle this like #define macro (do the replacement) 
> without allocating extra functional stack. On the contrary, if it is 
> implemented as a non-inline function, it will cause extra functional 
> stack when it was called every time. And it also should be implemented 
> in a source file (.c file), not in header file(.h file).

No, my concern is that if, for example, str_high_low() gets used in 100
.c files then we get 100 copies of the strings "high" and "low" in
vmlinux.  Making str_high_low() uninlined would fix this.

However a quick experiment tells me that the compiler and linker are
indeed able to perform this cross-object-file optimization:

--- a/fs/open.c~a
+++ a/fs/open.c
@@ -34,6 +34,8 @@
 #include <linux/mnt_idmapping.h>
 #include <linux/filelock.h>
 
+#include <linux/string_choices.h>
+
 #include "internal.h"
 
 int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
@@ -42,6 +44,8 @@ int do_truncate(struct mnt_idmap *idmap,
 	int ret;
 	struct iattr newattrs;
 
+	printk("%s\n", frozzle(dentry == NULL));
+
 	/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
 	if (length < 0)
 		return -EINVAL;
--- a/fs/inode.c~a
+++ a/fs/inode.c
@@ -22,6 +22,9 @@
 #include <linux/iversion.h>
 #include <linux/rw_hint.h>
 #include <trace/events/writeback.h>
+
+#include <linux/string_helpers.h>
+
 #include "internal.h"
 
 /*
@@ -110,6 +113,8 @@ static struct inodes_stat_t inodes_stat;
 static int proc_nr_inodes(const struct ctl_table *table, int write, void *buffer,
 			  size_t *lenp, loff_t *ppos)
 {
+	printk("%s\n", frozzle(table == NULL));
+
 	inodes_stat.nr_inodes = get_nr_inodes();
 	inodes_stat.nr_unused = get_nr_inodes_unused();
 	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
--- a/include/linux/string_choices.h~a
+++ a/include/linux/string_choices.h
@@ -4,6 +4,11 @@
 
 #include <linux/types.h>
 
+static inline const char *frozzle(bool v)
+{
+	return v ? "frizzle" : "frazzle";
+}
+
 static inline const char *str_enable_disable(bool v)
 {
 	return v ? "enable" : "disable";
_


x1:/usr/src/25> strings vmlinux|grep frazzle
frazzle
x1:/usr/src/25> 

See, only one copy!

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Hongbo Li]

On 2024/8/29 4:25, Andrew Morton wrote:
> On Wed, 28 Aug 2024 11:49:51 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:
> 
>>> Anything which is calling these functions is not performance-sensitive,
>>> so optimizing for space is preferred.  An out-of-line function which
>>> returns a const char * will achieve this?
>> I think this helper can achieve this. Because it is tiny enough, the
>> compiler will handle this like #define macro (do the replacement)
>> without allocating extra functional stack. On the contrary, if it is
>> implemented as a non-inline function, it will cause extra functional
>> stack when it was called every time. And it also should be implemented
>> in a source file (.c file), not in header file(.h file).
> 
> No, my concern is that if, for example, str_high_low() gets used in 100
> .c files then we get 100 copies of the strings "high" and "low" in
> vmlinux.  Making str_high_low() uninlined would fix this.

At first, I didn't consider these aspects.

> 
> However a quick experiment tells me that the compiler and linker are
> indeed able to perform this cross-object-file optimization:
> 
That's very good!

Thanks,
Hongbo

> --- a/fs/open.c~a
> +++ a/fs/open.c
> @@ -34,6 +34,8 @@
>   #include <linux/mnt_idmapping.h>
>   #include <linux/filelock.h>
>   
> +#include <linux/string_choices.h>
> +
>   #include "internal.h"
>   
>   int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
> @@ -42,6 +44,8 @@ int do_truncate(struct mnt_idmap *idmap,
>   	int ret;
>   	struct iattr newattrs;
>   
> +	printk("%s\n", frozzle(dentry == NULL));
> +
>   	/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
>   	if (length < 0)
>   		return -EINVAL;
> --- a/fs/inode.c~a
> +++ a/fs/inode.c
> @@ -22,6 +22,9 @@
>   #include <linux/iversion.h>
>   #include <linux/rw_hint.h>
>   #include <trace/events/writeback.h>
> +
> +#include <linux/string_helpers.h>
> +
>   #include "internal.h"
>   
>   /*
> @@ -110,6 +113,8 @@ static struct inodes_stat_t inodes_stat;
>   static int proc_nr_inodes(const struct ctl_table *table, int write, void *buffer,
>   			  size_t *lenp, loff_t *ppos)
>   {
> +	printk("%s\n", frozzle(table == NULL));
> +
>   	inodes_stat.nr_inodes = get_nr_inodes();
>   	inodes_stat.nr_unused = get_nr_inodes_unused();
>   	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
> --- a/include/linux/string_choices.h~a
> +++ a/include/linux/string_choices.h
> @@ -4,6 +4,11 @@
>   
>   #include <linux/types.h>
>   
> +static inline const char *frozzle(bool v)
> +{
> +	return v ? "frizzle" : "frazzle";
> +}
> +
>   static inline const char *str_enable_disable(bool v)
>   {
>   	return v ? "enable" : "disable";
> _
> 
> 
> x1:/usr/src/25> strings vmlinux|grep frazzle
> frazzle
> x1:/usr/src/25>
> 
> See, only one copy!
> 

Re: (subset) [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Kees Cook]

On Wed, 28 Aug 2024 13:25:09 -0700, Andrew Morton wrote:
> On Wed, 28 Aug 2024 11:49:51 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:
> 
> > > Anything which is calling these functions is not performance-sensitive,
> > > so optimizing for space is preferred.  An out-of-line function which
> > > returns a const char * will achieve this?
> > I think this helper can achieve this. Because it is tiny enough, the
> > compiler will handle this like #define macro (do the replacement)
> > without allocating extra functional stack. On the contrary, if it is
> > implemented as a non-inline function, it will cause extra functional
> > stack when it was called every time. And it also should be implemented
> > in a source file (.c file), not in header file(.h file).
> 
> [...]

Since I've taken over string maintainership, I've applied this to my
tree (where other similar changes are appearing). This should reduce
conflicts here...

Applied to for-next/hardening, thanks!

[1/3] lib/string_choices: Add str_true_false()/str_false_true() helper
      https://git.kernel.org/kees/c/6ff4cd1160af

Take care,

-- 
Kees Cook

Re: [PATCH -next v3 1/3] lib/string_choices: Add str_true_false()/str_false_true() helper

[Andy Shevchenko]

On Tue, Aug 27, 2024 at 04:42:18PM -0700, Andrew Morton wrote:
> On Tue, 27 Aug 2024 10:45:15 +0800 Hongbo Li <lihongbo22@huawei.com> wrote:

...

> > +#define str_false_true(v)		str_true_false(!(v))

> This might result in copies of the strings "true" and "false" being
> generated for every .c file which uses this function, resulting in
> unnecessary bloat.
> 
> It's possible that the compiler/linker can eliminate this duplication. 
> If not, I suggest that every function in string_choices.h be uninlined.

From this perspective this patch doesn't change anything.
The function is inline and in the same compilation module the linker will
optimise away the duplicates (note as well that this is kinda new feature,
some relatively old GCC might not have this feature, but I'm not an expert
in the area).

-- 
With Best Regards,
Andy Shevchenko

[PATCH -next v3 2/3] mm: make use of str_true_false helper

[Hongbo Li]

The helper str_true_false is introduced to reback "true/false"
string literal. We can simplify this format by str_true_false.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 mm/memory-tiers.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c
index 2a642ea86cb2..66ea9099dd63 100644
--- a/mm/memory-tiers.c
+++ b/mm/memory-tiers.c
@@ -940,8 +940,7 @@ bool numa_demotion_enabled = false;
 static ssize_t demotion_enabled_show(struct kobject *kobj,
 				     struct kobj_attribute *attr, char *buf)
 {
-	return sysfs_emit(buf, "%s\n",
-			  numa_demotion_enabled ? "true" : "false");
+	return sysfs_emit(buf, "%s\n", str_true_false(numa_demotion_enabled));
 }
 
 static ssize_t demotion_enabled_store(struct kobject *kobj,
-- 
2.34.1

[PATCH -next v3 3/3] nfs make use of str_false_true helper

[Hongbo Li]

The helper str_false_true is introduced to reback "false/true"
string literal. We can simplify this format by str_false_true.

Signed-off-by: Hongbo Li <lihongbo22@huawei.com>
---
 fs/nfs/nfs4xdr.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 7704a4509676..61190d6a5a77 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3447,7 +3447,7 @@ static int decode_attr_link_support(struct xdr_stream *xdr, uint32_t *bitmap, ui
 		*res = be32_to_cpup(p);
 		bitmap[0] &= ~FATTR4_WORD0_LINK_SUPPORT;
 	}
-	dprintk("%s: link support=%s\n", __func__, *res == 0 ? "false" : "true");
+	dprintk("%s: link support=%s\n", __func__, str_false_true(*res == 0));
 	return 0;
 }
 
@@ -3465,7 +3465,7 @@ static int decode_attr_symlink_support(struct xdr_stream *xdr, uint32_t *bitmap,
 		*res = be32_to_cpup(p);
 		bitmap[0] &= ~FATTR4_WORD0_SYMLINK_SUPPORT;
 	}
-	dprintk("%s: symlink support=%s\n", __func__, *res == 0 ? "false" : "true");
+	dprintk("%s: symlink support=%s\n", __func__, str_false_true(*res == 0));
 	return 0;
 }
 
@@ -3607,7 +3607,7 @@ static int decode_attr_case_insensitive(struct xdr_stream *xdr, uint32_t *bitmap
 		*res = be32_to_cpup(p);
 		bitmap[0] &= ~FATTR4_WORD0_CASE_INSENSITIVE;
 	}
-	dprintk("%s: case_insensitive=%s\n", __func__, *res == 0 ? "false" : "true");
+	dprintk("%s: case_insensitive=%s\n", __func__, str_false_true(*res == 0));
 	return 0;
 }
 
@@ -3625,7 +3625,7 @@ static int decode_attr_case_preserving(struct xdr_stream *xdr, uint32_t *bitmap,
 		*res = be32_to_cpup(p);
 		bitmap[0] &= ~FATTR4_WORD0_CASE_PRESERVING;
 	}
-	dprintk("%s: case_preserving=%s\n", __func__, *res == 0 ? "false" : "true");
+	dprintk("%s: case_preserving=%s\n", __func__, str_false_true(*res == 0));
 	return 0;
 }
 
@@ -4333,8 +4333,7 @@ static int decode_attr_xattrsupport(struct xdr_stream *xdr, uint32_t *bitmap,
 		*res = be32_to_cpup(p);
 		bitmap[2] &= ~FATTR4_WORD2_XATTR_SUPPORT;
 	}
-	dprintk("%s: XATTR support=%s\n", __func__,
-		*res == 0 ? "false" : "true");
+	dprintk("%s: XATTR support=%s\n", __func__, str_false_true(*res == 0));
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH -next v3 3/3] nfs make use of str_false_true helper

[Matthew Wilcox]

On Tue, Aug 27, 2024 at 10:45:17AM +0800, Hongbo Li wrote:
> The helper str_false_true is introduced to reback "false/true"
> string literal. We can simplify this format by str_false_true.

This seems unnecessarily verbose & complex.  How about:

	dprintk("%s: link support=%s\n", __func__, strbool(*res != 0));

> -	dprintk("%s: link support=%s\n", __func__, *res == 0 ? "false" : "true");
> +	dprintk("%s: link support=%s\n", __func__, str_false_true(*res == 0));

(do we have a convention for the antonym of kstrtoX?)

Re: [PATCH -next v3 3/3] nfs make use of str_false_true helper

[Hongbo Li]

On 2024/8/27 11:33, Matthew Wilcox wrote:
> On Tue, Aug 27, 2024 at 10:45:17AM +0800, Hongbo Li wrote:
>> The helper str_false_true is introduced to reback "false/true"
>> string literal. We can simplify this format by str_false_true.
> 
> This seems unnecessarily verbose & complex.  How about:
> 
> 	dprintk("%s: link support=%s\n", __func__, strbool(*res != 0));

This just keeps consistency with other string literal helpers such as 
"str_up_down", "str_yes_no" defined in include/linux/string_choices.h.

> 
>> -	dprintk("%s: link support=%s\n", __func__, *res == 0 ? "false" : "true");
>> +	dprintk("%s: link support=%s\n", __func__, str_false_true(*res == 0));
> 
> (do we have a convention for the antonym of kstrtoX?)

No, I haven't found this kind of convention helpers.

Thanks,
Hongbo

>