Message ID | 20230809155438.22470-8-rf@opensource.cirrus.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c44e:0:b0:3f2:4152:657d with SMTP id w14csp2943324vqr; Wed, 9 Aug 2023 10:06:10 -0700 (PDT) X-Google-Smtp-Source: AGHT+IE/tWqdPLvl6gDKVizUUHaenXRLCMj+eSlW9og8M3bJr3Y5qz90s/YL7WnWy0aot2M0Vfkq X-Received: by 2002:a05:6512:ea8:b0:4fe:676:8bf2 with SMTP id bi40-20020a0565120ea800b004fe06768bf2mr2632221lfb.10.1691600770410; Wed, 09 Aug 2023 10:06:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1691600770; cv=none; d=google.com; s=arc-20160816; b=j3kDqiULUY/mqyDWdGgXefplhb/XsUirJ3FKE1VVrUSEYaHhUjYJn80PEaTEG/pVbg Drp9q0aTnhk9Fb2XLWUwaA7iJKnnmqIqLHWqJR3JyPmVx4x8sY2tGRJcnfcQcSdYvAoe AAQPYnrg97i7oSDVrv/fFo++opBHZAcokJZdMeWBaMeeXKHdkAzvYvsnPmZiXhvThVi2 goXwYDJWRft96rsCvNCtVmcNtaDB/n2eIyJSTg+9Q/kThyxBhYaUFj0GpU93gChgRJvU sk/4nsS1aJDvAzHlEaMnrM/eh182jkIjOOHNeIm784EKKhplQKRZWBXvr5DhZA9Rdf4E F0kA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=EkcWozHmMlEsGAMvpqTB6x4BhQ+VCXW9AKlqr4C/ae0=; fh=bGF4ESczbIrmrNxFOdEMfzJNgTVkRsqDRt+LRN0CeSg=; b=iWUXRbSwuSDeX0dp68nDPDh42H8XB4OhLaRFvC3bGwje9jZcaA7s/03HbzCxZxM9tS 8r/rRABOZHrIqkG6HmnbUXq/eA5kpxeOPc6iB6mZGn5DB6WTqSDIkfT69ddykCMMkiPZ pnXhilDzVfmuZR5V9lBvwLDoKxdElEJBOg99iXPd4pEbDm3sWfFngB6utJ7kqZtWsNpY QoO68AoOyD3A9rbAUbpRdHhklC/REtSq0BVXnMX28+pB9qovBgKB+XVeLI0QDgGLtQ+s jKipo80g0JETM0Hyv3sCOmtJWPixaXb0tIzXdvzohW+ZTj1jujSLDpqmniIa3e4W9C0L J7eQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=QUwF7ePH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e21-20020a170906249500b00992fef51a60si8993543ejb.525.2023.08.09.10.05.30; Wed, 09 Aug 2023 10:06:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@cirrus.com header.s=PODMain02222019 header.b=QUwF7ePH; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=cirrus.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234541AbjHIPzS (ORCPT <rfc822;craechal@gmail.com> + 99 others); Wed, 9 Aug 2023 11:55:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234244AbjHIPzC (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 9 Aug 2023 11:55:02 -0400 Received: from mx0b-001ae601.pphosted.com (mx0a-001ae601.pphosted.com [67.231.149.25]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 59C221FF9; Wed, 9 Aug 2023 08:55:02 -0700 (PDT) Received: from pps.filterd (m0077473.ppops.net [127.0.0.1]) by mx0a-001ae601.pphosted.com (8.17.1.22/8.17.1.22) with ESMTP id 379CrDUe002197; Wed, 9 Aug 2023 10:54:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cirrus.com; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s= PODMain02222019; bh=EkcWozHmMlEsGAMvpqTB6x4BhQ+VCXW9AKlqr4C/ae0=; b= QUwF7ePHeVV7H0l2vQRS4JY92j3wj/W1O7Xe3tEdSzIIm15kxPQuwXGEs7tnCZan RozNdNAZFcuB3YvbSB2jb0ESmoEYM9xwmKFyXnEUzqCwhQbgpFda7JloNbH7cNlD TuuCqTbe3CSTZwN1yjlcvoEGAnvwkvktbnoV/Jr88QBuMq0/rnh9z+DluI8Q7df5 dQd8oXqUYLYxMdpy7yBXbYxOduinSDXacFiIv2pe9pbyHAzcP6/jeq8zlTQyRU1S T/xZQpg96BOqh6HzVvp6A1+3lgLn2m589oSxcjNHVlBuLjcR463Fjik0w1Etj/G8 t1n4YAvd6v+geH5f6haSzw== Received: from ediex02.ad.cirrus.com ([84.19.233.68]) by mx0a-001ae601.pphosted.com (PPS) with ESMTPS id 3sb7vtanrr-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 09 Aug 2023 10:54:51 -0500 (CDT) Received: from ediex02.ad.cirrus.com (198.61.84.81) by ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.30; Wed, 9 Aug 2023 16:54:48 +0100 Received: from ediswmail.ad.cirrus.com (198.61.86.93) by anon-ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server id 15.2.1118.30 via Frontend Transport; Wed, 9 Aug 2023 16:54:48 +0100 Received: from EDIN4L06LR3.ad.cirrus.com (EDIN4L06LR3.ad.cirrus.com [198.61.64.220]) by ediswmail.ad.cirrus.com (Postfix) with ESMTP id 68A0245D; Wed, 9 Aug 2023 15:54:48 +0000 (UTC) From: Richard Fitzgerald <rf@opensource.cirrus.com> To: <brendan.higgins@linux.dev>, <davidgow@google.com>, <rmoar@google.com> CC: <linux-kselftest@vger.kernel.org>, <kunit-dev@googlegroups.com>, <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com>, Richard Fitzgerald <rf@opensource.cirrus.com> Subject: [PATCH v3 7/7] kunit: Don't waste first attempt to format string in kunit_log_append() Date: Wed, 9 Aug 2023 16:54:38 +0100 Message-ID: <20230809155438.22470-8-rf@opensource.cirrus.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230809155438.22470-1-rf@opensource.cirrus.com> References: <20230809155438.22470-1-rf@opensource.cirrus.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Proofpoint-GUID: YESuJiL3msWuAJAE0qtyL58LJfr7ndi- X-Proofpoint-ORIG-GUID: YESuJiL3msWuAJAE0qtyL58LJfr7ndi- X-Proofpoint-Spam-Reason: safe X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1773771969473459129 X-GMAIL-MSGID: 1773771969473459129 |
Series |
kunit: Add dynamically-extending log
|
|
Commit Message
Richard Fitzgerald
Aug. 9, 2023, 3:54 p.m. UTC
It's wasteful to call vsnprintf() only to figure out the length of the
string. The string might fit in the available buffer space but we have to
vsnprintf() again to do that.
Instead, attempt to format the string to the available buffer at the same
time as finding the string length. Only if the string didn't fit the
available space is it necessary to extend the log and format the string
again to a new fragment buffer.
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
lib/kunit/test.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
Comments
On Wed, Aug 9, 2023 at 10:54 AM Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > It's wasteful to call vsnprintf() only to figure out the length of the > string. The string might fit in the available buffer space but we have to > vsnprintf() again to do that. > > Instead, attempt to format the string to the available buffer at the same > time as finding the string length. Only if the string didn't fit the > available space is it necessary to extend the log and format the string > again to a new fragment buffer. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Hello! I find this version slightly more confusing but I realize that the extra vsnprintf() call is unnecessary. So I am ok with this version except for one question below. I am curious what David Gow thinks and it would also be good to have another set of eyes here. My testing of this showed no problems. Thanks! -Rae > --- > lib/kunit/test.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 28d0048d6171..230ec5e9824f 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) > if (!log) > return; > > - /* Evaluate length of line to add to log */ > + frag = list_last_entry(log, struct kunit_log_frag, list); > + log_len = strlen(frag->buf); > + len_left = sizeof(frag->buf) - log_len - 1; > + > + /* Attempt to print formatted line to current fragment */ > va_start(args, fmt); > - len = vsnprintf(NULL, 0, fmt, args) + 1; > + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1; > va_end(args); > > + if (len <= len_left) > + goto out_newline; > + > + /* Abandon the truncated result of vsnprintf */ > + frag->buf[log_len] = '\0'; > + > if (len > sizeof(frag->buf) - 1) { > va_start(args, fmt); > tmp = kvasprintf(GFP_KERNEL, fmt, args); > @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) > return; > } > > - frag = list_last_entry(log, struct kunit_log_frag, list); > - log_len = strlen(frag->buf); > - len_left = sizeof(frag->buf) - log_len - 1; > - > - if (len > len_left) { > - frag = kunit_log_extend(log); > - if (!frag) > - return; > - > - len_left = sizeof(frag->buf) - 1; > - log_len = 0; > - } > + frag = kunit_log_extend(log); Are we meant to be using the remainder of the last fragment to its capacity or just start again with a new fragment and leave some of the last fragment empty? I worry we abandoned using the last fragment or is that the intention? Let me know if I understand this correctly. > + if (!frag) > + return; > > /* Print formatted line to the log */ > va_start(args, fmt); > - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args); > + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args); > va_end(args); > - > +out_newline: > /* Add newline to end of log if not already present. */ > kunit_log_newline(frag); > } > -- > 2.30.2 >
On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald <rf@opensource.cirrus.com> wrote: > > It's wasteful to call vsnprintf() only to figure out the length of the > string. The string might fit in the available buffer space but we have to > vsnprintf() again to do that. > > Instead, attempt to format the string to the available buffer at the same > time as finding the string length. Only if the string didn't fit the > available space is it necessary to extend the log and format the string > again to a new fragment buffer. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > --- This looks good. The only case I'm not totally convinced about is the last one, where the message doesn't fit in the current log fragment, but is not a whole fragment itself. I'm not sure if the added efficiency of not doing a second vsprintf() is worth the memory fragmentation of always starting a new fragment: the worst case where a log message is just over half the length of frag->buf would result in every message being in its own fragment (which would not necessarily have a consistent size), and memory use would be ~doubled. But I could accept it either way: until there's a real-world example which strongly benefits from either implementation, let's go with whatever we have working. Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/kunit/test.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 28d0048d6171..230ec5e9824f 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) > if (!log) > return; > > - /* Evaluate length of line to add to log */ > + frag = list_last_entry(log, struct kunit_log_frag, list); > + log_len = strlen(frag->buf); > + len_left = sizeof(frag->buf) - log_len - 1; > + > + /* Attempt to print formatted line to current fragment */ > va_start(args, fmt); > - len = vsnprintf(NULL, 0, fmt, args) + 1; > + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1; > va_end(args); > > + if (len <= len_left) > + goto out_newline; > + > + /* Abandon the truncated result of vsnprintf */ > + frag->buf[log_len] = '\0'; > + > if (len > sizeof(frag->buf) - 1) { > va_start(args, fmt); > tmp = kvasprintf(GFP_KERNEL, fmt, args); > @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) > return; > } > > - frag = list_last_entry(log, struct kunit_log_frag, list); > - log_len = strlen(frag->buf); > - len_left = sizeof(frag->buf) - log_len - 1; > - > - if (len > len_left) { > - frag = kunit_log_extend(log); > - if (!frag) > - return; > - > - len_left = sizeof(frag->buf) - 1; > - log_len = 0; > - } > + frag = kunit_log_extend(log); > + if (!frag) > + return; > > /* Print formatted line to the log */ > va_start(args, fmt); > - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args); > + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args); > va_end(args); > - > +out_newline: > /* Add newline to end of log if not already present. */ > kunit_log_newline(frag); > } > -- > 2.30.2 >
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 28d0048d6171..230ec5e9824f 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) if (!log) return; - /* Evaluate length of line to add to log */ + frag = list_last_entry(log, struct kunit_log_frag, list); + log_len = strlen(frag->buf); + len_left = sizeof(frag->buf) - log_len - 1; + + /* Attempt to print formatted line to current fragment */ va_start(args, fmt); - len = vsnprintf(NULL, 0, fmt, args) + 1; + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1; va_end(args); + if (len <= len_left) + goto out_newline; + + /* Abandon the truncated result of vsnprintf */ + frag->buf[log_len] = '\0'; + if (len > sizeof(frag->buf) - 1) { va_start(args, fmt); tmp = kvasprintf(GFP_KERNEL, fmt, args); @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...) return; } - frag = list_last_entry(log, struct kunit_log_frag, list); - log_len = strlen(frag->buf); - len_left = sizeof(frag->buf) - log_len - 1; - - if (len > len_left) { - frag = kunit_log_extend(log); - if (!frag) - return; - - len_left = sizeof(frag->buf) - 1; - log_len = 0; - } + frag = kunit_log_extend(log); + if (!frag) + return; /* Print formatted line to the log */ va_start(args, fmt); - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args); + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args); va_end(args); - +out_newline: /* Add newline to end of log if not already present. */ kunit_log_newline(frag); }