From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752867AbcF2Szo (ORCPT ); Wed, 29 Jun 2016 14:55:44 -0400 Received: from mga14.intel.com ([192.55.52.115]:13237 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751391AbcF2Szn (ORCPT ); Wed, 29 Jun 2016 14:55:43 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,547,1459839600"; d="scan'208";a="1012233318" Message-ID: <1467226354.30123.336.camel@linux.intel.com> Subject: Re: [PATCH 1/2] lib: hexdump: use a look-up table to do hex_to_bin From: Andy Shevchenko To: Michal Nazarewicz , zengzhaoxiu@163.com, linux-kernel@vger.kernel.org Cc: Zhaoxiu Zeng , Andrew Morton , Hidehiro Kawai , Borislav Petkov , Michal Hocko , Rasmus Villemoes , Nicolas Iooss , "Steven Rostedt (Red Hat)" , Gustavo Padovan , Geert Uytterhoeven , Horacio Mijail Anton Quiles Date: Wed, 29 Jun 2016 21:52:34 +0300 In-Reply-To: References: <1467216957-58885-1-git-send-email-zengzhaoxiu@163.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2016-06-29 at 20:31 +0200, Michal Nazarewicz wrote: > On Thu, Jun 30 2016, zengzhaoxiu wrote: > > From: Zhaoxiu Zeng > > > > Signed-off-by: Zhaoxiu Zeng > > --- > >  include/linux/kernel.h | 15 ++++++++++++++- > >  lib/hexdump.c          | 36 +++++++++++++++++++----------------- > >  2 files changed, 33 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > > index 94aa10f..72a0432 100644 > > --- a/include/linux/kernel.h > > +++ b/include/linux/kernel.h > > @@ -532,7 +532,20 @@ static inline char *hex_byte_pack_upper(char > > *buf, u8 byte) > >   return buf; > >  } > >   > > -extern int hex_to_bin(char ch); > > +extern const int8_t h2b_lut[]; > > + > > +/** > > + * hex_to_bin - convert a hex digit to its real value > > + * @ch: ascii character represents hex digit > > + * > > + * hex_to_bin() converts one hex digit to its actual value or -1 in > > case of bad > > + * input. > > + */ > > +static inline int hex_to_bin(char ch) > > +{ > > + return h2b_lut[(unsigned char)ch]; > > +} > > + > >  extern int __must_check hex2bin(u8 *dst, const char *src, size_t > > count); > >  extern char *bin2hex(char *dst, const void *src, size_t count); > >   > > diff --git a/lib/hexdump.c b/lib/hexdump.c > > index 992457b..878697f 100644 > > --- a/lib/hexdump.c > > +++ b/lib/hexdump.c > > @@ -18,23 +18,25 @@ EXPORT_SYMBOL(hex_asc); > >  const char hex_asc_upper[] = "0123456789ABCDEF"; > >  EXPORT_SYMBOL(hex_asc_upper); > >   > > -/** > > - * hex_to_bin - convert a hex digit to its real value > > - * @ch: ascii character represents hex digit > > - * > > - * hex_to_bin() converts one hex digit to its actual value or -1 in > > case of bad > > - * input. > > - */ > > -int hex_to_bin(char ch) > > -{ > > - if ((ch >= '0') && (ch <= '9')) > > - return ch - '0'; > > - ch = tolower(ch); > > - if ((ch >= 'a') && (ch <= 'f')) > > - return ch - 'a' + 10; > > - return -1; > > -} > > -EXPORT_SYMBOL(hex_to_bin); > > +const int8_t h2b_lut[] = { > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > +  0,  1,  2,  3,  4,  5,  6,  7,  8,  9, -1, -1, -1, -1, -1, > > -1, > > + -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, 10, 11, 12, 13, 14, 15, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > + -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, > > -1, > > +}; > > +EXPORT_SYMBOL(h2b_lut); > >   > >  /** > >   * hex2bin - convert an ascii hexadecimal string to its binary > > representation > > So this replaces table lookup in tolower with an explicit table lookup > here while also removing some branches. > > Is that an improvement?  Hard to say.  _ctype table is used by all the > other isfoo macros so there’s a chance it’s already in cache the first > time hex_to_bin is called.  Having to read the data into cache may > overwhelm advantages of lack of branches. > > Perhaps it’s better to get rid of existing table lookup instead (as > well > as a single branch): > > --------- >8 ------------------------------------------------------- > ---- > From c6a104a0e3c11ef5172dd00d2dcd44df486d1b58 Mon Sep 17 00:00:00 2001 > From: Michal Nazarewicz > Date: Wed, 29 Jun 2016 20:16:34 +0200 > Subject: [PATCH] lib: hexdump: avoid _ctype table lookup in hex_to_bin >  function > > tolower macro maps to __tolower function which calls isupper to > to determine if character is an upper case letter before converting > it to lower case.  This preservers non-letters unchanged which is > what you want in usual case. > > However, hex_to_bin does not care about non-letter characters so > such conversion can be performed as long as (i) upper case letters > become lower case, (ii) lower case letters are unchanged and (iii) > non-letters stay non-letters. > > This is exactly what _tolower function does and using it makes it > possible to avoid _ctype table lookup performed by the isupper > table. > > Furthermore, since _tolower conversion is done unconditionally, this > also eliminates a single branch. This change I agree with since _tolower() is specific for lib internal usage in the kernel. FWIW: Reviewed-by: Andy Shevchenko > > Signed-off-by: Michal Nazarewicz > --- >  lib/hexdump.c | 2 +- >  1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/hexdump.c b/lib/hexdump.c > index 992457b..f184d7a 100644 > --- a/lib/hexdump.c > +++ b/lib/hexdump.c > @@ -29,7 +29,7 @@ int hex_to_bin(char ch) >  { >   if ((ch >= '0') && (ch <= '9')) >   return ch - '0'; > - ch = tolower(ch); > + ch = _tolower(ch); >   if ((ch >= 'a') && (ch <= 'f')) >   return ch - 'a' + 10; >   return -1; -- Andy Shevchenko Intel Finland Oy