| Message ID | 20190903133731.2094-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] linux/kernel.h: add yesno(), onoff(), enableddisabled(), plural() helpers | expand |
On Tue, Sep 03, 2019 at 04:37:30PM +0300, 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": > > $ 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. > > While the main goal here is to abstract recurring patterns, and slightly > clean up the code base by not open coding the ternary operators, there > are also some space savings to be had via better string constant > pooling. > > 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> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_utils.h | 15 ------------- > .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ---------- > drivers/usb/core/config.c | 5 ----- > drivers/usb/core/generic.c | 5 ----- > include/linux/kernel.h | 21 +++++++++++++++++++ > 5 files changed, 21 insertions(+), 36 deletions(-) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On 03/09/2019 15.37, Jani Nikula wrote: > While the main goal here is to abstract recurring patterns, and slightly > clean up the code base by not open coding the ternary operators, there > are also some space savings to be had via better string constant > pooling. Eh, no? The linker does that across translation units anyway - moreover, given that you make them static inlines, "yes" and "no" will still live in .rodata.strX.Y in each individual TU that uses the yesno() helper. The enableddisabled() is a mouthful, perhaps the helpers should have an underscore between the choices yes_no() enabled_disabled() on_off() ? > drivers/gpu/drm/i915/i915_utils.h | 15 ------------- > .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ---------- > drivers/usb/core/config.c | 5 ----- > drivers/usb/core/generic.c | 5 ----- > include/linux/kernel.h | 21 +++++++++++++++++++ Pet peeve: Can we please stop using linux/kernel.h as a dumping ground for every little utility/helper? That makes each and every translation unit in the kernel slightly larger, hence slower to compile. Please make a linux/string-choice.h and put them there. Rasmus
On Wed, 04 Sep 2019, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > On 03/09/2019 15.37, Jani Nikula wrote: > >> While the main goal here is to abstract recurring patterns, and slightly >> clean up the code base by not open coding the ternary operators, there >> are also some space savings to be had via better string constant >> pooling. > > Eh, no? The linker does that across translation units anyway - moreover, > given that you make them static inlines, "yes" and "no" will still live > in .rodata.strX.Y in each individual TU that uses the yesno() helper. I should've been more careful there; this allows us to do better constant pooling but does not actually deliver on that here. You'd need to return pointers to strings in the kernel image. The linker can't constant pool across modules by itself. Anyway, that was not the main point here. > The enableddisabled() is a mouthful, perhaps the helpers should have an > underscore between the choices > > yes_no() > enabled_disabled() > on_off() > > ? I'm replacing existing functions that are being used in the kernel already. $ git grep "[^a-zA-Z0-9_]\(yesno\|onoff\|enableddisabled\)(" | wc -l 241 Not keen on renaming all of them. >> drivers/gpu/drm/i915/i915_utils.h | 15 ------------- >> .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ---------- >> drivers/usb/core/config.c | 5 ----- >> drivers/usb/core/generic.c | 5 ----- >> include/linux/kernel.h | 21 +++++++++++++++++++ > > Pet peeve: Can we please stop using linux/kernel.h as a dumping ground > for every little utility/helper? That makes each and every translation > unit in the kernel slightly larger, hence slower to compile. Please make > a linux/string-choice.h and put them there. *grin* In the absense of a natural candidate in include/linux/*.h, I thought shoving them to kernel.h would provoke the best feedback on where to put them. A new string-choice.h works for me, thanks. :) BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_utils.h b/drivers/gpu/drm/i915/i915_utils.h index 2987219a6300..9754e277622f 100644 --- a/drivers/gpu/drm/i915/i915_utils.h +++ b/drivers/gpu/drm/i915/i915_utils.h @@ -355,19 +355,4 @@ 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"; -} - #endif /* !__I915_UTILS_H */ diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c index d692251ee252..d0be14d93df7 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c @@ -2023,17 +2023,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 9d6cb709ca7b..7da06aa06ced 100644 --- a/drivers/usb/core/config.c +++ b/drivers/usb/core/config.c @@ -19,11 +19,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 1ac9c1e5f773..95a87b6cd35f 100644 --- a/drivers/usb/core/generic.c +++ b/drivers/usb/core/generic.c @@ -24,11 +24,6 @@ #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/kernel.h b/include/linux/kernel.h index 4fa360a13c1e..3375f054aefd 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -1008,4 +1008,25 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } /* OTHER_WRITABLE? Generally considered a bad idea. */ \ BUILD_BUG_ON_ZERO((perms) & 2) + \ (perms)) + +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
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. While the main goal here is to abstract recurring patterns, and slightly clean up the code base by not open coding the ternary operators, there are also some space savings to be had via better string constant pooling. 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> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_utils.h | 15 ------------- .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 ---------- drivers/usb/core/config.c | 5 ----- drivers/usb/core/generic.c | 5 ----- include/linux/kernel.h | 21 +++++++++++++++++++ 5 files changed, 21 insertions(+), 36 deletions(-)