| Message ID | 20240827024517.914100-2-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6ff4cd1160afafc12ad1603e3d2f39256e4b708d |
| Headers | show |
| Series | Add str_true_false()/str_false_true() helper | expand |
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.
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
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?
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
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).
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";
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! >
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,
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
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(+)