From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Alastair D'Silva" Subject: RE: [PATCH v2 4/7] lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags Date: Wed, 8 May 2019 21:41:15 +1000 Message-ID: <0a1c01d50592$f90f6f00$eb2e4d00$@d-silva.org> References: <20190508070148.23130-1-alastair@au1.ibm.com> <20190508070148.23130-5-alastair@au1.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Language: en-au Sender: linux-kernel-owner@vger.kernel.org To: 'David Laight' , 'Alastair D'Silva' Cc: 'Jani Nikula' , 'Joonas Lahtinen' , 'Rodrigo Vivi' , 'David Airlie' , 'Daniel Vetter' , 'Dan Carpenter' , 'Karsten Keil' , 'Jassi Brar' , 'Tom Lendacky' , "'David S. Miller'" , 'Jose Abreu' , 'Kalle Valo' , 'Stanislaw Gruszka' , 'Benson Leung' , 'Enric Balletbo i Serra' , "'James E.J. Bottomley'" , "'Martin K. Petersen'" List-Id: dri-devel@lists.freedesktop.org > -----Original Message----- > From: David Laight > Sent: Wednesday, 8 May 2019 7:20 PM > To: 'Alastair D'Silva' ; alastair@d-silva.org > Cc: Jani Nikula ; Joonas Lahtinen > ; Rodrigo Vivi = ; > David Airlie ; Daniel Vetter ; Dan > Carpenter ; Karsten Keil pingi.de>; Jassi Brar ; Tom Lendacky > ; David S. Miller ; > Jose Abreu ; Kalle Valo > ; Stanislaw Gruszka ; > Benson Leung ; Enric Balletbo i Serra > ; James E.J. Bottomley > ; Martin K. Petersen ; > Greg Kroah-Hartman ; Alexander Viro > ; Petr Mladek ; Sergey > Senozhatsky ; Steven Rostedt > ; Andrew Morton ; > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; = linux- > kernel@vger.kernel.org; netdev@vger.kernel.org; > ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux- > scsi@vger.kernel.org; linux-fbdev@vger.kernel.org; > devel@driverdev.osuosl.org; linux-fsdevel@vger.kernel.org > Subject: RE: [PATCH v2 4/7] lib/hexdump.c: Replace ascii bool in > hex_dump_to_buffer with flags >=20 > From: Alastair D'Silva > > Sent: 08 May 2019 08:02 > > To: alastair@d-silva.org > ... > > --- a/include/linux/printk.h > > +++ b/include/linux/printk.h > > @@ -480,13 +480,13 @@ enum { > > DUMP_PREFIX_OFFSET > > }; > > > > -extern int hex_dump_to_buffer(const void *buf, size_t len, int = rowsize, > > - int groupsize, char *linebuf, size_t linebuflen, > > - bool ascii); > > - > > #define HEXDUMP_ASCII (1 << 0) > > #define HEXDUMP_SUPPRESS_REPEATED (1 << 1) >=20 > These ought to be BIT(0) and BIT(1) Thanks, I'll address that. >=20 > > +extern int hex_dump_to_buffer(const void *buf, size_t len, int = rowsize, > > + int groupsize, char *linebuf, size_t linebuflen, > > + u64 flags); >=20 > Why 'u64 flags' ? > How many flags do you envisage ?? > Your HEXDUMP_ASCII (etc) flags are currently signed values and might = get > sign extended causing grief. > 'unsigned int flags' is probably sufficient. I was trying to avoid having to change the prototype again in the = future, but it's not a big deal, if enough work goes in to require more = than 32 bits, it can be updated at that point. >=20 > I've not really looked at the code, it seems OTT in places though. I'll wait for more concrete criticisms here, this it a bit too vague to = take any action on. > If someone copies it somewhere where the performance matters (I've = user > space code which is dominated by its tracing!) then you don't want all = the > function calls and conditionals even if you want some of the = functionality. Calling hexdump (even in it's unaltered form) in performance critical = code is always going to suck. As you mentioned before, it's all based = around printf. A performance conscious user would be better off building = their code around hex_asc_hi/lo instead (see lib/vsprintf.c:hex_string). --=20 Alastair D'Silva mob: 0423 762 819 skype: alastair_dsilva msn: alastair@d-silva.org blog: http://alastair.d-silva.org Twitter: @EvilDeece