From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brendan Higgins Subject: Re: [PATCH v9 03/18] kunit: test: add string_stream a std::stream like string builder Date: Mon, 15 Jul 2019 14:11:50 -0700 Message-ID: References: <20190712081744.87097-1-brendanhiggins@google.com> <20190712081744.87097-4-brendanhiggins@google.com> <20190715204356.4E3F92145D@mail.kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20190715204356.4E3F92145D@mail.kernel.org> Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd Cc: Frank Rowand , Greg KH , Josh Poimboeuf , Kees Cook , Kieran Bingham , Luis Chamberlain , Peter Zijlstra , Rob Herring , shuah , Theodore Ts'o , Masahiro Yamada , devicetree , dri-devel , kunit-dev@googlegroups.com, "open list:DOCUMENTATION" , linux-fsdevel@vger.kernel.org, linux-kbuild , Linux Kernel Mailing List , open list:KERNEL SELFTEST FRAMEWORK List-Id: linux-nvdimm@lists.01.org On Mon, Jul 15, 2019 at 1:43 PM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-07-12 01:17:29) > > diff --git a/include/kunit/string-stream.h b/include/kunit/string-stream.h > > new file mode 100644 > > index 0000000000000..0552a05781afe > > --- /dev/null > > +++ b/include/kunit/string-stream.h > > @@ -0,0 +1,49 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * C++ stream style string builder used in KUnit for building messages. > > + * > > + * Copyright (C) 2019, Google LLC. > > + * Author: Brendan Higgins > > + */ > > + > > +#ifndef _KUNIT_STRING_STREAM_H > > +#define _KUNIT_STRING_STREAM_H > > + > > +#include > > +#include > > +#include > > What is this include for? I'd expect to see linux/list.h instead. Sorry about that. I used to reference count this before I made it a kunit managed resource. > > +#include > > + > > +struct string_stream_fragment { > > + struct list_head node; > > + char *fragment; > > +}; > > + > > +struct string_stream { > > + size_t length; > > + struct list_head fragments; > > + /* length and fragments are protected by this lock */ > > + spinlock_t lock; > > +}; > > + > > diff --git a/kunit/string-stream.c b/kunit/string-stream.c > > new file mode 100644 > > index 0000000000000..0463a92dad74b > > --- /dev/null > > +++ b/kunit/string-stream.c > > @@ -0,0 +1,147 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * C++ stream style string builder used in KUnit for building messages. > > + * > > + * Copyright (C) 2019, Google LLC. > > + * Author: Brendan Higgins > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +int string_stream_vadd(struct string_stream *stream, > > + const char *fmt, > > + va_list args) > > +{ > > + struct string_stream_fragment *frag_container; > > + int len; > > + va_list args_for_counting; > > + unsigned long flags; > > + > > + /* Make a copy because `vsnprintf` could change it */ > > + va_copy(args_for_counting, args); > > + > > + /* Need space for null byte. */ > > + len = vsnprintf(NULL, 0, fmt, args_for_counting) + 1; > > + > > + va_end(args_for_counting); > > + > > + frag_container = kmalloc(sizeof(*frag_container), GFP_KERNEL); > > This is confusing in that it allocates with GFP_KERNEL but then grabs a > spinlock to add and remove from the fragment list. Is it ever going to > be called from a place where it can't sleep? If so, the GFP_KERNEL needs > to be changed. Otherwise, maybe a mutex would work better to protect > access to the fragment list. Right, using a mutex here would be fine. Sorry, I meant to filter for my usage of them after you asked me to remove them in 01, but evidently I forgot to do so. Sorry, will fix. > I also wonder if it would be better to just have a big slop buffer of a > 4K page or something so that we almost never have to allocate anything > with a string_stream and we can just rely on a reader consuming data > while writers are writing. That might work out better, but I don't quite > understand the use case for the string stream. That makes sense, but might that also waste memory since we will almost never need that much memory? > > + if (!frag_container) > > + return -ENOMEM; > > + > > + frag_container->fragment = kmalloc(len, GFP_KERNEL); > > + if (!frag_container->fragment) { > > + kfree(frag_container); > > + return -ENOMEM; > > + } > > + > > + len = vsnprintf(frag_container->fragment, len, fmt, args); > > + spin_lock_irqsave(&stream->lock, flags); > > + stream->length += len; > > + list_add_tail(&frag_container->node, &stream->fragments); > > + spin_unlock_irqrestore(&stream->lock, flags); > > + > > + return 0; > > +} > > + > [...] > > + > > +bool string_stream_is_empty(struct string_stream *stream) > > +{ > > + bool is_empty; > > + unsigned long flags; > > + > > + spin_lock_irqsave(&stream->lock, flags); > > I'm not sure what benefit grabbing the lock is having here. If the list > isn't empty after this is called then the race isn't resolved by > grabbing and releasing the lock. The function is returning stale data in > that case. Good point, I didn't realize list_empty was protected by READ_ONCE. Will fix. > > + is_empty = list_empty(&stream->fragments); > > + spin_unlock_irqrestore(&stream->lock, flags); > > + > > + return is_empty; > > +} > > + > > +static int string_stream_init(struct kunit_resource *res, void *context) > > +{ > > + struct string_stream *stream; > > + > > + stream = kzalloc(sizeof(*stream), GFP_KERNEL); > > + if (!stream) > > + return -ENOMEM; > > + > > + res->allocation = stream; > > + INIT_LIST_HEAD(&stream->fragments); > > + spin_lock_init(&stream->lock); > > + > > + return 0; > > +} > > + > > +static void string_stream_free(struct kunit_resource *res) > > +{ > > + struct string_stream *stream = res->allocation; > > + > > + string_stream_clear(stream); > > + kfree(stream); > > +} > > + > > +struct string_stream *alloc_string_stream(struct kunit *test) > > +{ > > + struct kunit_resource *res; > > + > > + res = kunit_alloc_resource(test, > > + string_stream_init, > > + string_stream_free, > > + NULL); > > + > > + if (!res) > > + return NULL; > > + > > + return res->allocation; > > Maybe kunit_alloc_resource() should just return res->allocation, or > NULL, so that these functions can be simplified to 'return > kunit_alloc_resource()'? Does the caller ever care to do anything with > struct kunit_resource anyway? Another good point. I think originally I thought it might, but now with the mandatory init function, the user has to provide a function where they can do the init work. They might as well do it there. Will fix.