* [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers @ 2019-10-23 13:13 Jani Nikula 2019-10-23 13:21 ` Rasmus Villemoes 2019-10-23 22:56 ` Andrew Morton 0 siblings, 2 replies; 8+ messages in thread From: Jani Nikula @ 2019-10-23 13:13 UTC (permalink / raw) To: linux-kernel Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Andrew Morton, Julia Lawall, Rasmus Villemoes The kernel has plenty of ternary operators to choose between constant strings, such as condition ? "yes" : "no", as well as value == 1 ? "" : "s": $ git grep '? "yes" : "no"' | wc -l 258 $ git grep '? "on" : "off"' | wc -l 204 $ git grep '? "enabled" : "disabled"' | wc -l 196 $ git grep '? "" : "s"' | wc -l 25 Additionally, there are some occurences of the same in reverse order, split to multiple lines, or otherwise not caught by the simple grep. Add helpers to return the constant strings. Remove existing equivalent and conflicting functions in i915, cxgb4, and USB core. Further conversion can be done incrementally. The main goal here is to abstract recurring patterns, and slightly clean up the code base by not open coding the ternary operators. Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: intel-gfx@lists.freedesktop.org Cc: Vishal Kulkarni <vishal@chelsio.com> Cc: netdev@vger.kernel.org Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-usb@vger.kernel.org Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-kernel@vger.kernel.org Cc: Julia Lawall <julia.lawall@lip6.fr> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- v2: add string-choice.[ch] to not clutter kernel.h and to actually save space on string constants. v3: back to static inlines based on Rasmus' feedback v4: Massaged commit message about space savings to make it less fluffy based on Rasmus' feedback. Example of further cleanup possibilities are at [1], to be done incrementally afterwards. [1] http://lore.kernel.org/r/20190903133731.2094-2-jani.nikula@intel.com --- drivers/gpu/drm/i915/i915_utils.h | 16 +--------- .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 12 +------ drivers/usb/core/config.c | 6 +--- drivers/usb/core/generic.c | 6 +--- include/linux/string-choice.h | 31 +++++++++++++++++++ 5 files changed, 35 insertions(+), 36 deletions(-) create mode 100644 include/linux/string-choice.h diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 562f756da421..794f02a90efe 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -28,6 +28,7 @@ #include <linux/list.h> #include <linux/overflow.h> #include <linux/sched.h> +#include <linux/string-choice.h> #include <linux/types.h> #include <linux/workqueue.h> @@ -395,21 +396,6 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) #define MBps(x) KBps(1000 * (x)) #define GBps(x) ((u64)1000 * MBps((x))) -static inline const char *yesno(bool v) -{ - return v ? "yes" : "no"; -} - -static inline const char *onoff(bool v) -{ - return v ? "on" : "off"; -} - -static inline const char *enableddisabled(bool v) -{ - return v ? "enabled" : "disabled"; -} - static inline void add_taint_for_CI(unsigned int taint) { /* diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c index ae6a47dd7dc9..d9123dae1d00 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c @@ -35,6 +35,7 @@ #include <linux/seq_file.h> #include <linux/debugfs.h> #include <linux/string_helpers.h> +#include <linux/string-choice.h> #include <linux/sort.h> #include <linux/ctype.h> @@ -2023,17 +2024,6 @@ static const struct file_operations rss_debugfs_fops = { /* RSS Configuration. */ -/* Small utility function to return the strings "yes" or "no" if the supplied - * argument is non-zero. - */ -static const char *yesno(int x) -{ - static const char *yes = "yes"; - static const char *no = "no"; - - return x ? yes : no; -} - static int rss_config_show(struct seq_file *seq, void *v) { struct adapter *adapter = seq->private; diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c index 151a74a54386..52cee9067eb4 100644 --- a/drivers/usb/core/config.c +++ b/drivers/usb/core/config.c @@ -10,6 +10,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/device.h> +#include <linux/string-choice.h> #include <asm/byteorder.h> #include "usb.h" @@ -19,11 +20,6 @@ #define USB_MAXCONFIG 8 /* Arbitrary limit */ -static inline const char *plural(int n) -{ - return (n == 1 ? "" : "s"); -} - static int find_next_descriptor(unsigned char *buffer, int size, int dt1, int dt2, int *num_skipped) { diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c index 38f8b3e31762..a784a09794d6 100644 --- a/drivers/usb/core/generic.c +++ b/drivers/usb/core/generic.c @@ -21,14 +21,10 @@ #include <linux/usb.h> #include <linux/usb/hcd.h> +#include <linux/string-choice.h> #include <uapi/linux/usb/audio.h> #include "usb.h" -static inline const char *plural(int n) -{ - return (n == 1 ? "" : "s"); -} - static int is_rndis(struct usb_interface_descriptor *desc) { return desc->bInterfaceClass == USB_CLASS_COMM diff --git a/include/linux/string-choice.h b/include/linux/string-choice.h new file mode 100644 index 000000000000..320b598bd8f0 --- /dev/null +++ b/include/linux/string-choice.h @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2019 Intel Corporation + */ + +#ifndef __STRING_CHOICE_H__ +#define __STRING_CHOICE_H__ + +#include <linux/types.h> + +static inline const char *yesno(bool v) +{ + return v ? "yes" : "no"; +} + +static inline const char *onoff(bool v) +{ + return v ? "on" : "off"; +} + +static inline const char *enableddisabled(bool v) +{ + return v ? "enabled" : "disabled"; +} + +static inline const char *plural(long v) +{ + return v == 1 ? "" : "s"; +} + +#endif /* __STRING_CHOICE_H__ */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 13:13 [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers Jani Nikula @ 2019-10-23 13:21 ` Rasmus Villemoes 2019-10-23 13:26 ` Jani Nikula 2019-10-23 22:56 ` Andrew Morton 1 sibling, 1 reply; 8+ messages in thread From: Rasmus Villemoes @ 2019-10-23 13:21 UTC (permalink / raw) To: Jani Nikula, linux-kernel Cc: Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Andrew Morton, Julia Lawall On 23/10/2019 15.13, Jani Nikula wrote: > The kernel has plenty of ternary operators to choose between constant > strings, such as condition ? "yes" : "no", as well as value == 1 ? "" : > "s": > > > v4: Massaged commit message about space savings to make it less fluffy > based on Rasmus' feedback. Thanks, it looks good to me. FWIW, Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 13:21 ` Rasmus Villemoes @ 2019-10-23 13:26 ` Jani Nikula 0 siblings, 0 replies; 8+ messages in thread From: Jani Nikula @ 2019-10-23 13:26 UTC (permalink / raw) To: Rasmus Villemoes, linux-kernel Cc: Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Andrew Morton, Julia Lawall On Wed, 23 Oct 2019, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 23/10/2019 15.13, Jani Nikula wrote: >> The kernel has plenty of ternary operators to choose between constant >> strings, such as condition ? "yes" : "no", as well as value == 1 ? "" : >> "s": >> >> >> v4: Massaged commit message about space savings to make it less fluffy >> based on Rasmus' feedback. > > Thanks, it looks good to me. FWIW, > > Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Thanks. I think the question is, which tree to apply this to, who's going to pick it up? I'm fine with any route. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 13:13 [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers Jani Nikula 2019-10-23 13:21 ` Rasmus Villemoes @ 2019-10-23 22:56 ` Andrew Morton 2019-10-23 23:46 ` Joe Perches ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Andrew Morton @ 2019-10-23 22:56 UTC (permalink / raw) To: Jani Nikula Cc: linux-kernel, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Julia Lawall, Rasmus Villemoes On Wed, 23 Oct 2019 16:13:08 +0300 Jani Nikula <jani.nikula@intel.com> wrote: > The kernel has plenty of ternary operators to choose between constant > strings, such as condition ? "yes" : "no", as well as value == 1 ? "" : > "s": > > $ git grep '? "yes" : "no"' | wc -l > 258 > $ git grep '? "on" : "off"' | wc -l > 204 > $ git grep '? "enabled" : "disabled"' | wc -l > 196 > $ git grep '? "" : "s"' | wc -l > 25 > > Additionally, there are some occurences of the same in reverse order, > split to multiple lines, or otherwise not caught by the simple grep. > > Add helpers to return the constant strings. Remove existing equivalent > and conflicting functions in i915, cxgb4, and USB core. Further > conversion can be done incrementally. > > The main goal here is to abstract recurring patterns, and slightly clean > up the code base by not open coding the ternary operators. Fair enough. > --- /dev/null > +++ b/include/linux/string-choice.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2019 Intel Corporation > + */ > + > +#ifndef __STRING_CHOICE_H__ > +#define __STRING_CHOICE_H__ > + > +#include <linux/types.h> > + > +static inline const char *yesno(bool v) > +{ > + return v ? "yes" : "no"; > +} > + > +static inline const char *onoff(bool v) > +{ > + return v ? "on" : "off"; > +} > + > +static inline const char *enableddisabled(bool v) > +{ > + return v ? "enabled" : "disabled"; > +} > + > +static inline const char *plural(long v) > +{ > + return v == 1 ? "" : "s"; > +} > + > +#endif /* __STRING_CHOICE_H__ */ These aren't very good function names. Better to create a kernel-style namespace such as "choice_" and then add the expected underscores: choice_yes_no() choice_enabled_disabled() choice_plural() (Example: note that slabinfo.c already has an "onoff()"). Also, I worry that making these functions inline means that each .o file will contain its own copy of the strings ("yes", "no", "enabled", etc) if the .c file calls the relevant helper. I'm not sure if the linker is smart enough (yet) to fix this up. If not, we will end up with a smaller kernel by uninlining these functions. lib/string-choice.c would suit. And doing this will cause additional savings: calling a single-arg out-of-line function generates less .text than calling yesno(). When I did this: --- a/include/linux/string-choice.h~string-choice-add-yesno-onoff-enableddisabled-plural-helpers-fix +++ a/include/linux/string-choice.h @@ -8,10 +8,7 @@ #include <linux/types.h> -static inline const char *yesno(bool v) -{ - return v ? "yes" : "no"; -} +const char *yesno(bool v); static inline const char *onoff(bool v) { The text segment of drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.o (78 callsites) shrunk by 118 bytes. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 22:56 ` Andrew Morton @ 2019-10-23 23:46 ` Joe Perches 2019-10-24 7:32 ` Jani Nikula 2019-10-24 7:40 ` Rasmus Villemoes 2 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2019-10-23 23:46 UTC (permalink / raw) To: Andrew Morton, Jani Nikula Cc: linux-kernel, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Julia Lawall, Rasmus Villemoes On Wed, 2019-10-23 at 15:56 -0700, Andrew Morton wrote: > And doing this will cause additional savings: calling a single-arg > out-of-line function generates less .text than calling yesno(). I get no change in size at all with any of extern static __always_inline with either of bool or int argument. gcc 8.3, defconfig with CONFIG_CHELSIO_T4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 22:56 ` Andrew Morton 2019-10-23 23:46 ` Joe Perches @ 2019-10-24 7:32 ` Jani Nikula 2019-10-24 7:40 ` Rasmus Villemoes 2 siblings, 0 replies; 8+ messages in thread From: Jani Nikula @ 2019-10-24 7:32 UTC (permalink / raw) To: Andrew Morton Cc: linux-kernel, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Julia Lawall, Rasmus Villemoes On Wed, 23 Oct 2019, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 23 Oct 2019 16:13:08 +0300 Jani Nikula <jani.nikula@intel.com> wrote: > >> The kernel has plenty of ternary operators to choose between constant >> strings, such as condition ? "yes" : "no", as well as value == 1 ? "" : >> "s": >> >> $ git grep '? "yes" : "no"' | wc -l >> 258 >> $ git grep '? "on" : "off"' | wc -l >> 204 >> $ git grep '? "enabled" : "disabled"' | wc -l >> 196 >> $ git grep '? "" : "s"' | wc -l >> 25 >> >> Additionally, there are some occurences of the same in reverse order, >> split to multiple lines, or otherwise not caught by the simple grep. >> >> Add helpers to return the constant strings. Remove existing equivalent >> and conflicting functions in i915, cxgb4, and USB core. Further >> conversion can be done incrementally. >> >> The main goal here is to abstract recurring patterns, and slightly clean >> up the code base by not open coding the ternary operators. > > Fair enough. > >> --- /dev/null >> +++ b/include/linux/string-choice.h >> @@ -0,0 +1,31 @@ >> +/* SPDX-License-Identifier: MIT */ >> +/* >> + * Copyright © 2019 Intel Corporation >> + */ >> + >> +#ifndef __STRING_CHOICE_H__ >> +#define __STRING_CHOICE_H__ >> + >> +#include <linux/types.h> >> + >> +static inline const char *yesno(bool v) >> +{ >> + return v ? "yes" : "no"; >> +} >> + >> +static inline const char *onoff(bool v) >> +{ >> + return v ? "on" : "off"; >> +} >> + >> +static inline const char *enableddisabled(bool v) >> +{ >> + return v ? "enabled" : "disabled"; >> +} >> + >> +static inline const char *plural(long v) >> +{ >> + return v == 1 ? "" : "s"; >> +} >> + >> +#endif /* __STRING_CHOICE_H__ */ > > These aren't very good function names. Better to create a kernel-style > namespace such as "choice_" and then add the expected underscores: > > choice_yes_no() > choice_enabled_disabled() > choice_plural() I was merely using existing function names used in several drivers in the kernel. But I can rename no problem. Are your suggestions the names we can settle on now, or should I expect to receive more opinions, but only after I send v5? > (Example: note that slabinfo.c already has an "onoff()"). Under tools/ though? I did mean to address all conflicts in this patch. > Also, I worry that making these functions inline means that each .o > file will contain its own copy of the strings ("yes", "no", "enabled", > etc) if the .c file calls the relevant helper. I'm not sure if the > linker is smart enough (yet) to fix this up. If not, we will end up > with a smaller kernel by uninlining these functions. > lib/string-choice.c would suit. > > And doing this will cause additional savings: calling a single-arg > out-of-line function generates less .text than calling yesno(). When I > did this: > > --- a/include/linux/string-choice.h~string-choice-add-yesno-onoff-enableddisabled-plural-helpers-fix > +++ a/include/linux/string-choice.h > @@ -8,10 +8,7 @@ > > #include <linux/types.h> > > -static inline const char *yesno(bool v) > -{ > - return v ? "yes" : "no"; > -} > +const char *yesno(bool v); > > static inline const char *onoff(bool v) > { > > The text segment of drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.o > (78 callsites) shrunk by 118 bytes. So we've already been back and forth on that particular topic in the history of this patch. v2 had lib/string-choice.c and no inlines [1]. In the end, starting to use functions, inline or not, will let us rework the implementation as we see fit, without touching the callers. Again, it's no problem to go back to lib/string-choice.c, *once* more, and the effort is trivial, but the ping-pong is getting old. BR, Jani. [1] http://lore.kernel.org/r/20190930141842.15075-1-jani.nikula@intel.com -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-23 22:56 ` Andrew Morton 2019-10-23 23:46 ` Joe Perches 2019-10-24 7:32 ` Jani Nikula @ 2019-10-24 7:40 ` Rasmus Villemoes 2019-10-24 7:57 ` Rasmus Villemoes 2 siblings, 1 reply; 8+ messages in thread From: Rasmus Villemoes @ 2019-10-24 7:40 UTC (permalink / raw) To: Andrew Morton, Jani Nikula Cc: linux-kernel, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Julia Lawall On 24/10/2019 00.56, Andrew Morton wrote: > On Wed, 23 Oct 2019 16:13:08 +0300 Jani Nikula <jani.nikula@intel.com> wrote: > >> + >> +static inline const char *yesno(bool v) >> +{ >> + return v ? "yes" : "no"; >> +} >> + >> +static inline const char *onoff(bool v) >> +{ >> + return v ? "on" : "off"; >> +} >> + >> +static inline const char *enableddisabled(bool v) >> +{ >> + return v ? "enabled" : "disabled"; >> +} >> + >> +static inline const char *plural(long v) >> +{ >> + return v == 1 ? "" : "s"; >> +} >> + >> +#endif /* __STRING_CHOICE_H__ */ > > These aren't very good function names. Better to create a kernel-style > namespace such as "choice_" and then add the expected underscores: > > choice_yes_no() > choice_enabled_disabled() > choice_plural() I think I prefer the short names (no choice_ prefix), it's rather obvious what they do. I also asked for underscores, especially for the enableddisabled case, but Jani didn't want to change existing callers. But I'll keep out of the naming discussion from now on. > Also, I worry that making these functions inline means that each .o > file will contain its own copy of the strings They will, in .rodata.str1.1 ("yes", "no", "enabled", > etc) if the .c file calls the relevant helper. I'm not sure if the > linker is smart enough (yet) to fix this up. AFAIK, that's an optimization the linker has done forever - the whole reason the SHF_MERGE | SHF_STRINGS (the MS in readelf -S output) flags exist (and AFAICT they have been part of the ELF spec since forever) is so the linker can do that trick. So no, do not make them ool. > And doing this will cause additional savings: calling a single-arg > out-of-line function generates less .text than calling yesno(). When I > did this: > > --- a/include/linux/string-choice.h~string-choice-add-yesno-onoff-enableddisabled-plural-helpers-fix > +++ a/include/linux/string-choice.h > @@ -8,10 +8,7 @@ > > #include <linux/types.h> > > -static inline const char *yesno(bool v) > -{ > - return v ? "yes" : "no"; > -} > +const char *yesno(bool v); > > static inline const char *onoff(bool v) > { > > The text segment of drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.o > (78 callsites) shrunk by 118 bytes. > Interesting, and not at all what I see. Mind dumping rss_config_show before/after? Even better, cxgb4_debugfs.s before/after. Here's what I get: static inline yesno: cbb: 49 c7 c4 00 00 00 00 mov $0x0,%r12 cbe: R_X86_64_32S .rodata.str1.1+0x197 cc2: 44 89 ea mov %r13d,%edx cc5: 48 c7 c6 00 00 00 00 mov $0x0,%rsi cc8: R_X86_64_32S .rodata.str1.1+0x1b7 ccc: 48 c7 c5 00 00 00 00 mov $0x0,%rbp ccf: R_X86_64_32S .rodata.str1.1+0x19b Load "yes" into %12 and "no" into %rbp (or vice versa). cd3: 4d 89 e7 mov %r12,%r15 cd6: e8 00 00 00 00 callq cdb <rss_config_show+0x3b> cd7: R_X86_64_PC32 seq_printf-0x4 cdb: 45 85 ed test %r13d,%r13d cde: 4c 89 e2 mov %r12,%rdx ce1: 48 89 df mov %rbx,%rdi ce4: 48 0f 49 d5 cmovns %rbp,%rdx ce8: 48 c7 c6 00 00 00 00 mov $0x0,%rsi ceb: R_X86_64_32S .rodata.str1.1+0x1cb cef: e8 00 00 00 00 callq cf4 <rss_config_show+0x54> cf0: R_X86_64_PC32 seq_printf-0x4 cf4: 41 f7 c5 00 00 00 40 test $0x40000000,%r13d cfb: 4c 89 e2 mov %r12,%rdx cfe: 48 89 df mov %rbx,%rdi d01: 48 0f 44 d5 cmove %rbp,%rdx d05: 48 c7 c6 00 00 00 00 mov $0x0,%rsi d08: R_X86_64_32S .rodata.str1.1+0x1e1 d0c: e8 00 00 00 00 callq d11 <rss_config_show+0x71> d0d: R_X86_64_PC32 seq_printf-0x4 Test a bit, move "yes" into %rdx, conditionally move "no" into %rdx instead, call seq_printf. d11: 41 f7 c5 00 00 00 20 test $0x20000000,%r13d d18: 4c 89 e2 mov %r12,%rdx d1b: 48 89 df mov %rbx,%rdi d1e: 48 0f 44 d5 cmove %rbp,%rdx d22: 48 c7 c6 00 00 00 00 mov $0x0,%rsi d25: R_X86_64_32S .rodata.str1.1+0x1f7 d29: e8 00 00 00 00 callq d2e <rss_config_show+0x8e> d2a: R_X86_64_PC32 seq_printf-0x4 etc. That's a marginal (i.e., after the preamble storing "yes" and "no" in callee-saved registers) cost of six instructions/29 bytes per seq_printf, three of which are to implement the yesno() call. extern const char *yesno(): 64e7: 48 c7 c6 00 00 00 00 mov $0x0,%rsi 64ea: R_X86_64_32S .rodata.str1.1+0x8e4 64ee: 89 ea mov %ebp,%edx 64f0: 41 89 ed mov %ebp,%r13d 64f3: e8 00 00 00 00 callq 64f8 <rss_config_show+0x28> 64f4: R_X86_64_PC32 seq_printf-0x4 64f8: 89 ef mov %ebp,%edi 64fa: c1 ef 1f shr $0x1f,%edi 64fd: e8 00 00 00 00 callq 6502 <rss_config_show+0x32> 64fe: R_X86_64_PC32 yesno-0x4 Three instructions to prepare the argument to yesno and call it. 6502: 48 c7 c6 00 00 00 00 mov $0x0,%rsi 6505: R_X86_64_32S .rodata.str1.1+0x8f8 6509: 48 89 c2 mov %rax,%rdx One more to put the return from yesno in the right register. 650c: 48 89 df mov %rbx,%rdi 650f: e8 00 00 00 00 callq 6514 <rss_config_show+0x44> 6510: R_X86_64_PC32 seq_printf-0x4 So not a lot, but still one more instruction, for a total of 31 bytes. bloat-o-meter says $ scripts/bloat-o-meter drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.o.{1,2} add/remove: 0/0 grow/shrink: 3/0 up/down: 301/0 (301) Function old new delta rss_config_show 2343 2482 +139 rss_vf_config_show 263 363 +100 rss_pf_config_show 342 404 +62 which is more then 2*78, but I haven't looked at the code patterns in the other functions. Did you use size(1) to compare when you say "text segment"? That would include .rodata (and, more importantly, .rodata.strX.Y) in its text column. Maybe your compiler doesn't do string literal merging (since the linker does it anyway), so your .rodata.str1.1 might contain several copies of "yes" and "no", but they shouldn't really be counted. Rasmus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers 2019-10-24 7:40 ` Rasmus Villemoes @ 2019-10-24 7:57 ` Rasmus Villemoes 0 siblings, 0 replies; 8+ messages in thread From: Rasmus Villemoes @ 2019-10-24 7:57 UTC (permalink / raw) To: Andrew Morton, Jani Nikula Cc: linux-kernel, Joonas Lahtinen, Rodrigo Vivi, intel-gfx, Vishal Kulkarni, netdev, Greg Kroah-Hartman, linux-usb, Julia Lawall On 24/10/2019 09.40, Rasmus Villemoes wrote: > column. Maybe your compiler doesn't do string literal merging (since the > linker does it anyway), so your .rodata.str1.1 might contain several > copies of "yes" and "no", but they shouldn't really be counted. Sorry, that's of course nonsense - the strings only appear once in the TU (inside the static inline function), so gcc must treat them all as the same object - as opposed to the case where the implementation was #define yesno(x) ((x) ? "yes" : "no") So that can't explain why you saw a smaller text segment using the OOL version. Rasmus ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-24 7:57 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-10-23 13:13 [PATCH v4] string-choice: add yesno(), onoff(), enableddisabled(), plural() helpers Jani Nikula 2019-10-23 13:21 ` Rasmus Villemoes 2019-10-23 13:26 ` Jani Nikula 2019-10-23 22:56 ` Andrew Morton 2019-10-23 23:46 ` Joe Perches 2019-10-24 7:32 ` Jani Nikula 2019-10-24 7:40 ` Rasmus Villemoes 2019-10-24 7:57 ` Rasmus Villemoes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).