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 5550FC43334 for ; Mon, 20 Jun 2022 16:14:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C17818E0001; Mon, 20 Jun 2022 12:14:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BC7508E0002; Mon, 20 Jun 2022 12:14:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A8EDE8E0001; Mon, 20 Jun 2022 12:14:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 9AF866B0074 for ; Mon, 20 Jun 2022 12:14:31 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id 7304F120202 for ; Mon, 20 Jun 2022 16:14:31 +0000 (UTC) X-FDA: 79599111942.20.CF06D5F Received: from mail-qv1-f47.google.com (mail-qv1-f47.google.com [209.85.219.47]) by imf21.hostedemail.com (Postfix) with ESMTP id E24721C00B3 for ; Mon, 20 Jun 2022 16:14:30 +0000 (UTC) Received: by mail-qv1-f47.google.com with SMTP id p31so16523078qvp.5 for ; Mon, 20 Jun 2022 09:14:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=p4CMxnr4TX5oTAzHAubXZp5xX3+REtv8nCM4eiESU5Q=; b=R71i2J2sED1s83Ouq64dlBdAsgl6Hc+byj0jgTAfBVjpFq7/xHf3OP2w+HnJ8mmJ5x zjwTLG17LwnFGi/zwaPhTCuQafI6PcICLitDLgiLnvw7axAxXqSdKOih1Ezff8KJghUs 7OZupfpYvYEJH+xKVcd3+aARs9CbYHe4ZNoeISKMVeB+Bpg2gwX7UUIy4WQjar9mRDjd vs78qa26KuW5gQit2fAiUFaE6/XUEcMiakyXens8dSJSaY02+sKtjsZhGYD5UbyyTtVH j7PUm+fvN4xa8AT0ug9fRenkTLUeECXlYPxq4NoKviDlBgc1l8fXZl/mnm5pM0SaLh3k OXlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=p4CMxnr4TX5oTAzHAubXZp5xX3+REtv8nCM4eiESU5Q=; b=twGcizp5ntDG/MKczgVNxtTm3QbsaxE4zjSp9NF/Em3rsy/RQTkFq5prroU/fl/U6A Bd1iQ9kUZRmi1r8v/7mQpldNMm1TinpDDbsSxdtnpZJw4EiqGhyb84lOhBIAw+XRfARx lpB5/HZjDU9W6PGQ9kJeCXUQVC2cSDaN5cc1Ho021whtidEGIEHEy4WPgwzSmhm1NzBk CJ3vh5fhBdXleYNip/cJAGHgbM3bMwMV5xmICFCSQOAb/7RPwxjqbLXPy1ZFd/yAP3jt aLifOmjWeE24dRtZHACDfMZ7cDaAn49gR8w5XszCztnpL0vRIiMp7HDV6SQngMsCZ3xZ T+FQ== X-Gm-Message-State: AJIora+cy8zF8yK7doDoZyWpwdMJQnGr8HyH71Ziu1tUO9iqaGFNpYNK lU/BfpHw6uurYFhzDicUPw== X-Google-Smtp-Source: AGRyM1u8NQs9vos0RvN+IdXTEzRMdEFf/mAMb2m50gpqQYvGPYboO9VcT0Y/URUd//KABL+M7SKtbQ== X-Received: by 2002:a05:6214:20aa:b0:467:cb4b:a1fa with SMTP id 10-20020a05621420aa00b00467cb4ba1famr19378623qvd.9.1655741669967; Mon, 20 Jun 2022 09:14:29 -0700 (PDT) Received: from localhost (c-73-219-103-14.hsd1.vt.comcast.net. [73.219.103.14]) by smtp.gmail.com with ESMTPSA id j17-20020a05622a039100b00304fe96c7aasm11294030qtx.24.2022.06.20.09.14.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 20 Jun 2022 09:14:29 -0700 (PDT) Date: Mon, 20 Jun 2022 12:14:28 -0400 From: Kent Overstreet To: David Laight 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 Message-ID: <20220620161428.xdowwr6zsvfgm5qi@moria.home.lan> References: <20220620004233.3805-1-kent.overstreet@gmail.com> <20220620004233.3805-2-kent.overstreet@gmail.com> <20220620153043.vgtfrltebiyprufz@moria.home.lan> <5156ce6a38ab4f9c87ddf29ee05a266a@AcuMS.aculab.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5156ce6a38ab4f9c87ddf29ee05a266a@AcuMS.aculab.com> ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655741671; a=rsa-sha256; cv=none; b=0Wm81RxFOsNOCBKEPJtq64nuykOwxMzptnROvbtirpbfpttSqjRy9vI2xufGHoKwLd6IMs qCtzDy24G9Rw0yupZNa7IIUSK3LX2ikM2YJyNfVrc/1PVCIZaSN+LcoylDfFveJHgk/hPc fwitB1NVT8/N/PeaF+Nwtz2QYx7OjgQ= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=R71i2J2s; spf=pass (imf21.hostedemail.com: domain of kent.overstreet@gmail.com designates 209.85.219.47 as permitted sender) smtp.mailfrom=kent.overstreet@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655741671; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=p4CMxnr4TX5oTAzHAubXZp5xX3+REtv8nCM4eiESU5Q=; b=G4Tww0tM1F65Y7PNb5bUveN9xlzrgjocjxI5dlbnwlmzChWufSwkEDclt0BTguSa3BTA+m NYEGK/PFMtSsXaX+tfDUOKiFfeUqOUDN3al0ahapXtm1xVpA+y/42xm+kmf1W2g3iTRVgF XoAO39oES896OfaaPhrDOazwgilPHE8= X-Stat-Signature: 4bznsgymecs7wzxmin9qjtwjfghx3pde X-Rspamd-Queue-Id: E24721C00B3 Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=R71i2J2s; spf=pass (imf21.hostedemail.com: domain of kent.overstreet@gmail.com designates 209.85.219.47 as permitted sender) smtp.mailfrom=kent.overstreet@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspamd-Server: rspam07 X-Rspam-User: X-HE-Tag: 1655741670-48700 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: On Mon, Jun 20, 2022 at 03:53:38PM +0000, David Laight wrote: > From: Kent Overstreet > > Sent: 20 June 2022 16:31 > > > > 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 the > > > > size of the output buffer as well as the current output position. > > > > > > > > Future patches will be adding more features to printbuf, but initially > > > > 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 vsnprintf > > > > + */ > > > > + > > > > +struct printbuf { > > > > + char *buf; > > > > + unsigned size; > > > > + unsigned pos; > > > > > > No naked unsigneds. > > > > This is the way I've _always_ written kernel code - single word type names. > > I'm pretty sure the coding standards require 'int'. I've been contributing code to the kernel for many years and I'm picky about my style, I'm not about to change now. > > > > > > > > +}; > > > > + > > > > +/* > > > > + * Returns size remaining of output buffer: > > > > + */ > > > > +static inline unsigned printbuf_remaining_size(struct printbuf *out) > > > > +{ > > > > + return 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) > > > > +{ > > > > + return out->pos < out->size ? out->size - out->pos - 1 : 0; > > > > +} > > > > > > Those two are so similar mistakes will be make. > > > > If you've got ideas for better names I'd be happy to hear them - we discussed > > this and this was what we came up with. > > > > > You can also just return negatives when the buffer has overlowed > > > and get the callers to test < or <= as required. > > > > Yeesh, no. > > Why not? > All the callers are internal. > It saves a test and branch (or cmove). Because this is a subtle thing and having two separate helpers better documents the _intent_ of the code. I prioritize having clear and understandable code over shaving every branch. printbuf_remaining() is the one almost all callers want to use; printbuf_remaining_size() is only for a few callers that are doing weird things and should probably be converted to something more standard. > > > > 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 == size (after writing the '\0') allows > > > overflow be detected without most of the dangers. > > > > Because that's what snprintf() needs. > > > > > > + > > > > +static inline unsigned printbuf_written(struct printbuf *out) > > > > +{ > > > > + return min(out->pos, out->size); > > > > > > That excludes the '\0' for short buffers but includes > > > it for overlong ones. > > > > 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. Ah, you're right. Ok, that's a bug, I'll fix that. > 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().) Look, from scanning the kernel log I get you're the kind of programmer who likes shaving every branch and instruction. I've spent a _lot_ of time in my career staring at profiles and assembly and counting cycles (and was working with someone juinor doing that just last night), and what I've found over the years is that time and again... it's memory accesses and cache misses that matter, and not much else. I put a lot of effort into writing high performance code, and what I find is that my time is better spent when I focus on writing clear and understandable code, and making sure that things are laid out in memory intelligently, instead of trying to generate the "perfect" assembly from all the code I write. Perfect can be the enemy of good. If you want to submit patches later optimizing this stuff, be my guest - _but_, if it comes at the cost of making the code harder to read I'll want to see benchmark improvements, and of more than a few percent here and there.