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 1B1AEC43334 for ; Tue, 21 Jun 2022 07:58:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 959628E0001; Tue, 21 Jun 2022 03:58:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 907F46B0073; Tue, 21 Jun 2022 03:58:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7D0298E0001; Tue, 21 Jun 2022 03:58:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 6D0E16B0072 for ; Tue, 21 Jun 2022 03:58:24 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 36FEB210CA for ; Tue, 21 Jun 2022 07:58:24 +0000 (UTC) X-FDA: 79601490528.13.7076158 Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) by imf18.hostedemail.com (Postfix) with ESMTP id B9BE71C0004 for ; Tue, 21 Jun 2022 07:58:23 +0000 (UTC) Received: by mail-lf1-f44.google.com with SMTP id g4so8631995lfv.9 for ; Tue, 21 Jun 2022 00:58:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=rAupvnq6gF+hfLMwbK5FyshoSuZewieI0Qe5jG8mpEQ=; b=SHZB3elG8KdiWQghfW62kq8Y+JxtW3U7O18wzOMTwfPmoGL+RUVIV4mssgQfpyX/rP AYyxH5BAB0QHlruVFAoHSzDP3uKW1fHvwoFgiP6f+pR8W7+QfS4k0jmuVeVzhWW8gnxx 6Ccwc2mdBKx0lDj0U9XrhF+UuQLBKJIJ4Bc4s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=rAupvnq6gF+hfLMwbK5FyshoSuZewieI0Qe5jG8mpEQ=; b=a79JJUyclC7iQvc77mrPoHNps8I8QdTDgbyIqRxtdsbm+pN6EzCEHVKET5M1au84TU gnC4nMTrJKQoJJ14K/kvR7B8WDqWBTtAAvfBnnHuVsCDJzqa8MV+cw4YHzGPUW7STSWs somspCDuKU1MJvwj+GViLjzYjTcv5LuevhOodBGXF8RUvDeDTkxSpoJ//BLNGMn7/BKA ihv3/tE5Q/KQi0I4MbWTTMpvlkzah8xPBD37ZZfgKLOhLIhv5ZiVQvjsqcXDPPMdQBub CObv6396xONBUyHpTAjKAcBPuLiKpS84JiiCjpiTWWiAzHUIqtuL4dOXcQ7t3gvfew+f VA6g== X-Gm-Message-State: AJIora+lljQRna8Sq4D98QY7Q07TQV27KI+WtUgBOE2I85uS6FL/VIrj ymStlynU8f0nt+VEo8JupiUU1g== X-Google-Smtp-Source: AGRyM1uvyHwv5Nms9vMGrT+Pj3Myl57qJQnwG96nAmt2D1U+IkwcRVqRNQQZtUK91Tb/HD+gpUxTMw== X-Received: by 2002:a05:6512:1523:b0:47f:7940:4a24 with SMTP id bq35-20020a056512152300b0047f79404a24mr2767391lfb.516.1655798301954; Tue, 21 Jun 2022 00:58:21 -0700 (PDT) Received: from [172.16.11.74] ([81.216.59.226]) by smtp.gmail.com with ESMTPSA id h20-20020a05651c125400b0024f3d1daea2sm1938664ljh.42.2022.06.21.00.58.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Jun 2022 00:58:20 -0700 (PDT) Message-ID: <44e37b45-55cf-3705-e5e6-e3921d3646c4@rasmusvillemoes.dk> Date: Tue, 21 Jun 2022 09:58:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH v4 07/34] lib/printbuf: Heap allocation Content-Language: en-US To: Kent Overstreet , linux-kernel@vger.kernel.org, linux-mm@kvack.org, pmladek@suse.com Cc: rostedt@goodmis.org, enozhatsky@chromium.org, willy@infradead.org References: <20220620004233.3805-1-kent.overstreet@gmail.com> <20220620004233.3805-8-kent.overstreet@gmail.com> From: Rasmus Villemoes In-Reply-To: <20220620004233.3805-8-kent.overstreet@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655798303; a=rsa-sha256; cv=none; b=xaiPc5qbzmODKPE514f1hxUJmjccenaC+J1InJgDAhyMHUbd1Pi+EEvL8nI22lpIvqpV3+ ZIxgW+Slp0Ym0BQYOPyRroGaoyNFxsFAmVb8WJ40shmk1jWPMfGSYk+AIMHGuibCbhspt9 YLXZZ0GZgBqiTqlZyP+tNM3atmrKcPI= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=rasmusvillemoes.dk header.s=google header.b=SHZB3elG; dmarc=none; spf=pass (imf18.hostedemail.com: domain of linux@rasmusvillemoes.dk designates 209.85.167.44 as permitted sender) smtp.mailfrom=linux@rasmusvillemoes.dk ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655798303; 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:dkim-signature; bh=rAupvnq6gF+hfLMwbK5FyshoSuZewieI0Qe5jG8mpEQ=; b=x9kluvuS3ehN3HO8bRD1kdN5dBXZHOT0pCZqQy18u1YHhd8sTxoyNqSKfysjne8Tq733ph cu82d5jzt0N9dEl2NHJoHsLH5mef2N9lwNNBgQMygdSoNDpMW7ig7l9v77ksNWbV5KvB6u 4uEP/k58zs3adME/1VX0R7O7/BKR21k= Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=rasmusvillemoes.dk header.s=google header.b=SHZB3elG; dmarc=none; spf=pass (imf18.hostedemail.com: domain of linux@rasmusvillemoes.dk designates 209.85.167.44 as permitted sender) smtp.mailfrom=linux@rasmusvillemoes.dk X-Rspam-User: X-Stat-Signature: rbot3nxg8ix53idbwhbx6k6hntmt1ihs X-Rspamd-Queue-Id: B9BE71C0004 X-Rspamd-Server: rspam08 X-HE-Tag: 1655798303-444750 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 20/06/2022 02.42, Kent Overstreet wrote: > This makes printbufs optionally heap allocated: a printbuf initialized > with the PRINTBUF initializer will automatically heap allocate and > resize as needed. > > Allocations are done with GFP_KERNEL: code should use e.g. > memalloc_nofs_save()/restore() as needed. Since we do not currently have > memalloc_nowait_save()/restore(), in contexts where it is not safe to > block we provide the helpers > > printbuf_atomic_inc() > printbuf_atomic_dec() > > When the atomic count is nonzero, memory allocations will be done with > GFP_NOWAIT. > > On memory allocation failure, output will be truncated. Code that wishes > to check for memory allocation failure (in contexts where we should > return -ENOMEM) should check if printbuf->allocation_failure is set. > Since printbufs are expected to be typically used for log messages and > on a best effort basis, we don't return errors directly. > > Other helpers provided by this patch: > > - printbuf_make_room(buf, extra) > Reallocates if necessary to make room for @extra bytes (not including > terminating null). > > - printbuf_str(buf) > Returns a null terminated string equivalent to the contents of @buf. > If @buf was never allocated (or allocation failed), returns a > constant empty string. > > - printbuf_exit(buf) > Releases memory allocated by a printbuf. > > Signed-off-by: Kent Overstreet > --- > include/linux/printbuf.h | 120 +++++++++++++++++++++++++++++++++------ > lib/Makefile | 2 +- > lib/printbuf.c | 71 +++++++++++++++++++++++ > 3 files changed, 175 insertions(+), 18 deletions(-) > create mode 100644 lib/printbuf.c > > diff --git a/include/linux/printbuf.h b/include/linux/printbuf.h > index 8186c447ca..382863afa7 100644 > --- a/include/linux/printbuf.h > +++ b/include/linux/printbuf.h > @@ -4,19 +4,69 @@ > #ifndef _LINUX_PRINTBUF_H > #define _LINUX_PRINTBUF_H > > -#include > -#include > - > /* > - * Printbufs: String buffer for outputting (printing) to, for vsnprintf > + * Printbufs: Simple strings for printing to, with optional heap allocation > + * > + * This code has provisions for use in userspace, to aid in making other code > + * portable between kernelspace and userspace. > + * > + * Basic example: > + * struct printbuf buf = PRINTBUF; > + * > + * prt_printf(&buf, "foo="); > + * foo_to_text(&buf, foo); > + * printk("%s", buf.buf); So, if prt_printf() and foo_to_text() failed to make room, buf.buf would still be NULL, right? Sure, vsnprintf() handles that gracefully, but this is probably not what you intended. > struct printbuf { > char *buf; > unsigned size; > unsigned pos; > + /* > + * If nonzero, allocations will be done with GFP_ATOMIC: > + */ > + u8 atomic; > + bool allocation_failure:1; > + bool heap_allocated:1; > }; > > +int printbuf_make_room(struct printbuf *, unsigned); > +const char *printbuf_str(const struct printbuf *); > +void printbuf_exit(struct printbuf *); > + > +/* Initializer for a heap allocated printbuf: */ > +#define PRINTBUF ((struct printbuf) { .heap_allocated = true }) > + > +/* Initializer a printbuf that points to an external buffer: */ > +#define PRINTBUF_EXTERN(_buf, _size) \ > +((struct printbuf) { \ > + .buf = _buf, \ > + .size = _size, \ > +}) > + > /* > * Returns size remaining of output buffer: > */ > @@ -49,26 +99,36 @@ static inline bool printbuf_overflowed(struct printbuf *out) > > static inline void printbuf_nul_terminate(struct printbuf *out) > { > + printbuf_make_room(out, 1); > + Shouldn't this be printbuf_make_room(out, 0)? Probably this should be split up, so that the functions that also do printbuf_make_room() which ensures room for a nul-terminator could then call __printbuf_nul_terminate(), which would just contain the below: > if (out->pos < out->size) > out->buf[out->pos] = 0; > else if (out->size) > out->buf[out->size - 1] = 0; > } > -static inline void __prt_char(struct printbuf *out, char c) > +/* Doesn't call printbuf_make_room(), doesn't nul terminate: */ > +static inline void __prt_char_reserved(struct printbuf *out, char c) > { > if (printbuf_remaining(out)) > out->buf[out->pos] = c; > out->pos++; > } > > +/* Doesn't nul terminate: */ > +static inline void __prt_char(struct printbuf *out, char c) > +{ > + printbuf_make_room(out, 1); > + __prt_char_reserved(out, c); > +} > + > static inline void prt_char(struct printbuf *out, char c) > { > __prt_char(out, c); > printbuf_nul_terminate(out); > } > > -static inline void __prt_chars(struct printbuf *out, char c, unsigned n) > +static inline void __prt_chars_reserved(struct printbuf *out, char c, unsigned n) > { > unsigned i, can_print = min(n, printbuf_remaining(out)); > > @@ -79,13 +139,18 @@ static inline void __prt_chars(struct printbuf *out, char c, unsigned n) > > static inline void prt_chars(struct printbuf *out, char c, unsigned n) > { > - __prt_chars(out, c, n); > + printbuf_make_room(out, n); > + __prt_chars_reserved(out, c, n); > printbuf_nul_terminate(out); > } > > static inline void prt_bytes(struct printbuf *out, const void *b, unsigned n) > { > - unsigned i, can_print = min(n, printbuf_remaining(out)); > + unsigned i, can_print; > + > + printbuf_make_room(out, n); > + > + can_print = min(n, printbuf_remaining(out)); > > for (i = 0; i < can_print; i++) > out->buf[out->pos++] = ((char *) b)[i]; > @@ -101,22 +166,43 @@ static inline void prt_str(struct printbuf *out, const char *str) > > static inline void prt_hex_byte(struct printbuf *out, u8 byte) > { > - __prt_char(out, hex_asc_hi(byte)); > - __prt_char(out, hex_asc_lo(byte)); > + printbuf_make_room(out, 2); > + __prt_char_reserved(out, hex_asc_hi(byte)); > + __prt_char_reserved(out, hex_asc_lo(byte)); > printbuf_nul_terminate(out); > } > > static inline void prt_hex_byte_upper(struct printbuf *out, u8 byte) > { > - __prt_char(out, hex_asc_upper_hi(byte)); > - __prt_char(out, hex_asc_upper_lo(byte)); > + printbuf_make_room(out, 2); > + __prt_char_reserved(out, hex_asc_upper_hi(byte)); > + __prt_char_reserved(out, hex_asc_upper_lo(byte)); > printbuf_nul_terminate(out); > } > > -#define PRINTBUF_EXTERN(_buf, _size) \ > -((struct printbuf) { \ > - .buf = _buf, \ > - .size = _size, \ > -}) > +/** > + * printbuf_reset - re-use a printbuf without freeing and re-initializing it: > + */ > +static inline void printbuf_reset(struct printbuf *buf) > +{ > + buf->pos = 0; > + buf->allocation_failure = 0; > +} > + > +/** > + * printbuf_atomic_inc - mark as entering an atomic section > + */ > +static inline void printbuf_atomic_inc(struct printbuf *buf) > +{ > + buf->atomic++; > +} > + > +/** > + * printbuf_atomic_inc - mark as leaving an atomic section > + */ > +static inline void printbuf_atomic_dec(struct printbuf *buf) > +{ > + buf->atomic--; > +} So, if I have a printbuf in scope, and I do irq_disable() or spin_lock() or whatnot, I'm supposed to also call printbuf_atomic_inc(), at least if the printbuf is used within the locked region. Honest question: An u8 used for this purpose cannot overflow? > #endif /* _LINUX_PRINTBUF_H */ > diff --git a/lib/Makefile b/lib/Makefile > index 6b9ffc1bd1..b4609a4258 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -34,7 +34,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \ > is_single_threaded.o plist.o decompress.o kobject_uevent.o \ > earlycpio.o seq_buf.o siphash.o dec_and_lock.o \ > nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \ > - buildid.o > + buildid.o printbuf.o > > lib-$(CONFIG_PRINTK) += dump_stack.o > lib-$(CONFIG_SMP) += cpumask.o > diff --git a/lib/printbuf.c b/lib/printbuf.c > new file mode 100644 > index 0000000000..8c70128e31 > --- /dev/null > +++ b/lib/printbuf.c > @@ -0,0 +1,71 @@ > +// SPDX-License-Identifier: LGPL-2.1+ > +/* Copyright (C) 2022 Kent Overstreet */ > + > +#ifdef __KERNEL__ > +#include > +#include > +#else > +#define EXPORT_SYMBOL(x) > +#endif > + > +#include > +#include > +#include > + > +int printbuf_make_room(struct printbuf *out, unsigned extra) > +{ > + unsigned new_size; > + char *buf; > + > + if (!out->heap_allocated) > + return 0; I think that ->allocation_failure should be sticky and make us return an early error here; if we're under memory pressure we don't want each and every prt_char() of whatever we're trying to print to end up trying to do an allocation. > + /* Reserved space for terminating nul: */ > + extra += 1; > + > + if (out->pos + extra < out->size) > + return 0; Are you sure you don't want to be careful about the possibility of out->pos+extra overflowing? And since extra has been ++'ed, shouldn't the comparison be <= ? If pos is 0, size is 2, and I want to add one char (so on entry extra is 1), this should not require a reallocation? > + new_size = roundup_pow_of_two(out->size + extra); Are you sure you don't want to be careful about the possibility of out->size+extra overflowing, or hitting that with roundup_pow_of_two() doing that? > + buf = krealloc(out->buf, new_size, !out->atomic ? GFP_KERNEL : GFP_NOWAIT); > + > + if (!buf) { > + out->allocation_failure = true; > + return -ENOMEM; > + } > + > + out->buf = buf; > + out->size = new_size; > + return 0; > +} > +EXPORT_SYMBOL(printbuf_make_room); > + > +/** > + * printbuf_str - returns printbuf's buf as a C string, guaranteed to be null Pet peeve: Please use the spelling "nul" consistently. > + * terminated > + */ > +const char *printbuf_str(const struct printbuf *buf) > +{ > + /* > + * If we've written to a printbuf then it's guaranteed to be a null > + * terminated string - but if we haven't, then we might not have > + * allocated a buffer at all: > + */ > + return buf->pos > + ? buf->buf > + : ""; > +} > +EXPORT_SYMBOL(printbuf_str); I think the documentation lacks some mention of lifetimes and caller obligations or lack thereof. Especially since the return value could become dangling not just if the printbuf is destroyed (printbuf_exit), but also any other use of the printbuf which could cause a realloc. Rasmus