From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161138AbcFCLQR (ORCPT ); Fri, 3 Jun 2016 07:16:17 -0400 Received: from mga14.intel.com ([192.55.52.115]:13964 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932453AbcFCLQI (ORCPT ); Fri, 3 Jun 2016 07:16:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,411,1459839600"; d="scan'208";a="990326598" Message-ID: <1464952643.1767.42.camel@linux.intel.com> Subject: Re: [PATCH] lib/uuid.c: eliminate uuid_[bl]e_index arrays From: Andy Shevchenko To: George Spelvin Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, tytso@mit.edu Date: Fri, 03 Jun 2016 14:17:23 +0300 In-Reply-To: <20160531203122.7243.qmail@ns.sciencehorizons.net> References: <20160531203122.7243.qmail@ns.sciencehorizons.net> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.2-2 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 Tue, 2016-05-31 at 16:31 -0400, George Spelvin wrote: > From a0d084b1225f2efcf4b5c81871c9c446155b9b13 Mon Sep 17 00:00:00 2001 > From: George Spelvin > Date: Tue, 31 May 2016 16:00:22 -0400 > Subject: [PATCH] lib/uuid.c: eliminate uuid_[bl]e_index arrays There is a tool called git send-email. It would be better to use it directly. Also, please fix Cc list (I got bounce response) and add some key people like Rasmus. > > Both input and output code is simplified if we instead use a mapping > from binary UUID index to ASCII UUID position.  This lets us combine > hyphen-skipping and endian-swapping into one table. > > uuid_[bl]e_index were EXPORT_SYMBOLed for no obvious reason; there > are no users outside of lib/.  The replacement uuid_[bl]e_pos arrays > are not exported pending finding a need. > > The arrays are combined in one contiguous uuid_byte_pos[2][16] > array as a micro-optimization for uuid_string(). Oh, it makes readability worse. >   Choosing between > the two can be done by adding 16 rather than loading a second > full-word address. > > x86-64 code size reductions: > uuid_string: was 228 bytes, now 134 > __uuid_to_bin: was 119 bytes, now 85 x86_32? arm? > Initialized data is also reduced by 16 bytes. > Here's a patch implementing the suggestion I made earlier.  This > reduces > code size, data size, and run time for input and output of UUIDs. > > This patch is on top of the upper/lower case hex optimization for > lib/vsprintf.c I sent earlier.  If you don't have it, just ignore the > merge conflicts in uuid_string() and take the "after" version. > --- a/include/linux/uuid.h > +++ b/include/linux/uuid.h > @@ -41,8 +41,10 @@ extern void uuid_be_gen(uuid_be *u); >   >  bool __must_check uuid_is_valid(const char *uuid); >   > -extern const u8 uuid_le_index[16]; > -extern const u8 uuid_be_index[16]; > +/* For each binary byte, string offset in ASCII UUID where it appears > */ > +extern const u8 uuid_byte_pos[2][16]; > +#define uuid_be_pos (uuid_byte_pos[0]) > +#define uuid_le_pos (uuid_byte_pos[1]) >   >  int uuid_le_to_bin(const char *uuid, uuid_le *u); >  int uuid_be_to_bin(const char *uuid, uuid_be *u); > diff --git a/lib/uuid.c b/lib/uuid.c > index e116ae5f..8a439caf 100644 > --- a/lib/uuid.c > +++ b/lib/uuid.c > @@ -21,10 +21,10 @@ >  #include >  #include >   > -const u8 uuid_le_index[16] = {3,2,1,0,5,4,7,6,8,9,10,11,12,13,14,15}; > -EXPORT_SYMBOL(uuid_le_index); > -const u8 uuid_be_index[16] = {0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15}; > -EXPORT_SYMBOL(uuid_be_index); > +const u8 uuid_byte_pos[2][16] = { > + {0,2,4,6,9,11,14,16,19,21,24,26,28,30,32,34}, /* > uuid_be_pos */ > + {6,4,2,0,11,9,16,14,19,21,24,26,28,30,32,34} /* > uuid_le_pos */ > +}; And what prevent you to use two arrays? Above looks not good for reading and error prone for users. >   >  /*************************************************************** >   * Random UUID interface > @@ -97,32 +97,28 @@ bool uuid_is_valid(const char *uuid) >  } >  EXPORT_SYMBOL(uuid_is_valid); >   > -static int __uuid_to_bin(const char *uuid, __u8 b[16], const u8 > ei[16]) > +static int __uuid_to_bin(const char uuid[36], __u8 b[16], const u8 > si[16]) This one... Let's keep a prototype as is for now. >  { > - static const u8 si[16] = > {0,2,4,6,9,11,14,16,19,21,24,26,28,30,32,34}; >   unsigned int i; >   >   if (!uuid_is_valid(uuid)) >   return -EINVAL; >   > > - for (i = 0; i < 16; i++) { > - int hi = hex_to_bin(uuid[si[i]] + 0); > - int lo = hex_to_bin(uuid[si[i]] + 1); > - > - b[ei[i]] = (hi << 4) | lo; > - } > + for (i = 0; i < 16; i++) > + if (hex2bin(b + i, uuid + si[i], 1) < 0) > + return -EINVAL; How hex2bin is better here? We have validation done, no need to repeat. So, I suggest not to touch this piece of code. > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1313,38 +1313,32 @@ char *uuid_string(char *buf, char *end, const > u8 *addr, >     struct printf_spec spec, const char *fmt) >  { >   char uuid[UUID_STRING_LEN + 1]; > - char *p = uuid; >   int i; > - const u8 *index = uuid_be_index; > + const u8 *pos = uuid_be_pos; >   const char *hex = hex_asc; >   >   switch (fmt[1]) { > + case 'l': > + pos = uuid_le_pos; > + break; >   case 'L': > - hex = hex_asc_upper; /* fall-through */ > - case 'l': > - index = uuid_le_index; > - break; > + pos = uuid_le_pos; /* Fall-through */ >   case 'B': >   hex = hex_asc_upper; >   break; >   } >   > + /* Format each byte of the raw uuid into the buffer */ >   for (i = 0; i < 16; i++) { > - u8 byte = addr[index[i]]; > + u8 byte = addr[i]; > + char *p = uuid + pos[i]; >   > - *p++ = hex[byte >> 4]; > - *p++ = hex[byte & 0x0f]; > - switch (i) { > - case 3: > - case 5: > - case 7: > - case 9: > - *p++ = '-'; > - break; > - } > > + p[0] = hex[byte >> 4]; > + p[1] = hex[byte & 0x0f]; If you wish you may convert this to hex_byte_pack{,upper}(). -- Andy Shevchenko Intel Finland Oy