From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CAA7AC433EF for ; Mon, 20 Jun 2022 15:53:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2C0486B0071; Mon, 20 Jun 2022 11:53:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 247C16B0073; Mon, 20 Jun 2022 11:53:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0EB608E0001; Mon, 20 Jun 2022 11:53:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id EDF586B0071 for ; Mon, 20 Jun 2022 11:53:45 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id C88BF34BFD for ; Mon, 20 Jun 2022 15:53:45 +0000 (UTC) X-FDA: 79599059610.04.13981AC Received: from eu-smtp-delivery-151.mimecast.com (eu-smtp-delivery-151.mimecast.com [185.58.86.151]) by imf21.hostedemail.com (Postfix) with ESMTP id 2E88D1C001D for ; Mon, 20 Jun 2022 15:53:44 +0000 (UTC) Received: from AcuMS.aculab.com (156.67.243.121 [156.67.243.121]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384) id uk-mta-405-7RBaScP0Muydi72A4Eiqlw-1; Mon, 20 Jun 2022 16:53:41 +0100 X-MC-Unique: 7RBaScP0Muydi72A4Eiqlw-1 Received: from AcuMS.Aculab.com (fd9f:af1c:a25b:0:994c:f5c2:35d6:9b65) by AcuMS.aculab.com (fd9f:af1c:a25b:0:994c:f5c2:35d6:9b65) with Microsoft SMTP Server (TLS) id 15.0.1497.36; Mon, 20 Jun 2022 16:53:38 +0100 Received: from AcuMS.Aculab.com ([fe80::994c:f5c2:35d6:9b65]) by AcuMS.aculab.com ([fe80::994c:f5c2:35d6:9b65%12]) with mapi id 15.00.1497.036; Mon, 20 Jun 2022 16:53:38 +0100 From: David Laight To: 'Kent Overstreet' CC: "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "pmladek@suse.com" , "rostedt@goodmis.org" , "enozhatsky@chromium.org" , "linux@rasmusvillemoes.dk" , "willy@infradead.org" Subject: RE: [PATCH v4 01/34] lib/printbuf: New data structure for printing strings Thread-Topic: [PATCH v4 01/34] lib/printbuf: New data structure for printing strings Thread-Index: AQHYhD6p9ckAsWkCSk+0B0i5FtmPu61XsWoggACqvYCAABJf0A== Date: Mon, 20 Jun 2022 15:53:38 +0000 Message-ID: <5156ce6a38ab4f9c87ddf29ee05a266a@AcuMS.aculab.com> References: <20220620004233.3805-1-kent.overstreet@gmail.com> <20220620004233.3805-2-kent.overstreet@gmail.com> <20220620153043.vgtfrltebiyprufz@moria.home.lan> In-Reply-To: <20220620153043.vgtfrltebiyprufz@moria.home.lan> Accept-Language: en-GB, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.202.205.107] MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: aculab.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655740425; a=rsa-sha256; cv=none; b=sAeO2hLKUhlT4kgvVGRWTCkjJwqOK47GhYvUKgIWfOJ8Dllpe1wdV4o/ueVjzR4iL631xx QtJHnhCCcb2EgxtfJDRsINpeZwxC6a/MbVwLwRGp+vncsNDRMdg8RC3zBu8w8lnWorZmWf tN0XWYhCC5OpEcvysCIBS7s3TADgb2A= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655740425; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WieFts+TsiI/NpPKJEi7zcvDDluLJKB+QcGhPm+kOm4=; b=0R9FiOXcIRjanW5hClFq5HPorjvBhrg7K0J+1/SFzqPFdOErldLgr2+4CLJv6VD3nU2Lk1 GgJWvmzG3kMl2PgdR+RC5mM0gg6TLQ9ndaJgbpg5bFEhcwwGjHBetI9Y3MD+SFSIDVSunn egzYBLNf7nT7Ajps2w7jOvstRvbvLYw= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf21.hostedemail.com: domain of david.laight@aculab.com designates 185.58.86.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com Authentication-Results: imf21.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=aculab.com; spf=pass (imf21.hostedemail.com: domain of david.laight@aculab.com designates 185.58.86.151 as permitted sender) smtp.mailfrom=david.laight@aculab.com X-Rspam-User: X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 2E88D1C001D X-Stat-Signature: hckh6ma8ezeu3xuqzj4ngbw5jhkrcmd1 X-HE-Tag: 1655740424-101712 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: From: Kent Overstreet > Sent: 20 June 2022 16:31 >=20 > On Mon, Jun 20, 2022 at 04:44:10AM +0000, David Laight wrote: > > From: Kent Overstreet > > > Sent: 20 June 2022 01:42 > > > > > > This adds printbufs: a printbuf points to a char * buffer and knows t= he > > > size of the output buffer as well as the current output position. > > > > > > Future patches will be adding more features to printbuf, but initiall= y > > > printbufs are targeted at refactoring and improving our existing code= in > > > lib/vsprintf.c - so this initial printbuf patch has the features > > > required for that. > > > > > > Signed-off-by: Kent Overstreet > > > Reviewed-by: Matthew Wilcox (Oracle) > > > --- > > > include/linux/printbuf.h | 122 +++++++++++++++++++++++++++++++++++++= ++ > > > 1 file changed, 122 insertions(+) > > > create mode 100644 include/linux/printbuf.h > > > > > > diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h > > > new file mode 100644 > > > index 0000000000..8186c447ca > > > --- /dev/null > > > +++ b/include/linux/printbuf.h > > > @@ -0,0 +1,122 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1+ */ > > > +/* Copyright (C) 2022 Kent Overstreet */ > > > + > > > +#ifndef _LINUX_PRINTBUF_H > > > +#define _LINUX_PRINTBUF_H > > > + > > > +#include > > > +#include > > > + > > > +/* > > > + * Printbufs: String buffer for outputting (printing) to, for vsnpri= ntf > > > + */ > > > + > > > +struct printbuf { > > > +=09char=09=09=09*buf; > > > +=09unsigned=09=09size; > > > +=09unsigned=09=09pos; > > > > No naked unsigneds. >=20 > This is the way I've _always_ written kernel code - single word type name= s. I'm pretty sure the coding standards require 'int'. > > > > > +}; > > > + > > > +/* > > > + * Returns size remaining of output buffer: > > > + */ > > > +static inline unsigned printbuf_remaining_size(struct printbuf *out) > > > +{ > > > +=09return out->pos < out->size ? out->size - out->pos : 0; > > > +} > > > + > > > +/* > > > + * Returns number of characters we can print to the output buffer - = i.e. > > > + * excluding the terminating nul: > > > + */ > > > +static inline unsigned printbuf_remaining(struct printbuf *out) > > > +{ > > > +=09return out->pos < out->size ? out->size - out->pos - 1 : 0; > > > +} > > > > Those two are so similar mistakes will be make. >=20 > If you've got ideas for better names I'd be happy to hear them - we discu= ssed > this and this was what we came up with. >=20 > > You can also just return negatives when the buffer has overlowed > > and get the callers to test < or <=3D as required. >=20 > Yeesh, no. Why not? All the callers are internal. It saves a test and branch (or cmove). > > I also wonder it is necessary to count the total length > > when the buffer isn't long enough? > > Unless there is a real pressing need for it I'd not bother. > > Setting pos =3D=3D size (after writing the '\0') allows > > overflow be detected without most of the dangers. >=20 > Because that's what snprintf() needs. >=20 > > > + > > > +static inline unsigned printbuf_written(struct printbuf *out) > > > +{ > > > +=09return min(out->pos, out->size); > > > > That excludes the '\0' for short buffers but includes > > it for overlong ones. >=20 > It actually doesn't. If size is 2 it goes 0, 1, 2, 2, 2 as bytes are added. But the string is "" "a" "a" "a" - never 2 characters.=20 ... > > > +static inline void prt_bytes(struct printbuf *out, const void *b, un= signed n) > > > +{ > > > +=09unsigned i, can_print =3D min(n, printbuf_remaining(out)); > > > + > > > +=09for (i =3D 0; i < can_print; i++) > > > +=09=09out->buf[out->pos++] =3D ((char *) b)[i]; > > > +=09out->pos +=3D n - can_print; > > > + > > > +=09printbuf_nul_terminate(out); > > > > jeepers - that can be written so much better. > > Something like: > > =09unsigned int i, pos =3D out->pos; > > =09int space =3D pos - out->size - 1; > > =09char *tgt =3D out->buf + pos; > > =09const char *src =3D b; > > =09out->pos =3D pos + n; > > > > =09if (space <=3D 0) > > =09=09return; > > =09if (n > space) > > =09=09n =3D space; > > > > =09for (i =3D 0; i < n; i++) > > =09=09tgt[i] =3D src[i]; > > =09tgt[1] =3D 0; > > >=20 > I find your version considerably harder to read, and I've stared at enoug= h > assembly that I trust the compiler to generate pretty equivalent code. It can't because it can't assume that out->buf doesn't overlap 'out'. I'm also pretty sure it can't optimise out the test before adding the '\0'. >=20 > > > +} > > > + > > > +static inline void prt_str(struct printbuf *out, const char *str) > > > +{ > > > +=09prt_bytes(out, str, strlen(str)); > > > > Do you really need to call strlen() and then process > > the buffer byte by byte? >=20 > Versus introducing a branch to check for nul into the inner loop of prt_b= ytes()? > You're not serious, are you? As opposed to the one in strlen() ? I realise that there are shift and mask algorithms from strlen() that are likely faster than a byte scan on 64 bit systems. But they are likely slower than the check when you have a loop that is scanning byte by byte. This is especially true on out of order superscaler cpu when the copy loop won't be using all the execution blocks. What might be faster on cpu (like x86) where misaligned memory access are almost entirely free is to copy 8 bytes at a time while checking for a zero at the same time. Remember kernel strings are quite often short, the overhead costs for 'fast' routines slow things down. (As you found when calling memcpy() and memset().) =09David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1= PT, UK Registration No: 1397386 (Wales)