Re: [PATCH v3 3/7] kunit: Handle logging of lines longer than the fragment buffer size

[David Gow]

On Thu, 10 Aug 2023 at 23:09, Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On 10/8/23 15:38, David Gow wrote:
> > On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald
> > <rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >>
> >> Add handling to kunit_log_append() for log messages that are longer than
> >> a single buffer fragment.
> >>
> >> The initial implementation of fragmented buffers did not change the logic
> >> of the original kunit_log_append(). A consequence was that it still had
> >> the original assumption that a log line will fit into one buffer.
> >>
> >> This patch checks for log messages that are larger than one fragment
> >> buffer. In that case, kvasprintf() is used to format it into a temporary
> >> buffer and that content is then split across as many fragments as
> >> necessary.
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@xxxxxxxxxxxxxxxxxxxxx>
> >> ---
> >
> > I think this looks good (and is a long-overdue addition to the logging
> > functionality).
> >
> > One thought I have (and I'm kicking myself for not thinking of it
> > earlier) is that this is starting to get very similar to the "string
> > stream" functionality in lib/kunit/string-stream.{h,c}. Now, I
> > actually think this implementation is much more efficient (using
> > larger fragments, whereas the string stream uses variable-sized ones).
> > Particularly since there are a lot of cases where string streams are
> > created, converted to a string, and then logged, there's almost
> > certainly a bunch of redundant work being done here.
> >
> > My gut feeling is that we should stick with this as-is, and maybe try
> > to either work out some better integration between string streams and
> > logging (to avoid that extra string allocation) or find some way of
> > combining them.
> >
>
> I completely failed to notice string_stream. I could re-implement this
> to use string_stream. I wonder whether appending newlines gets
> a bit inefficient with the current string_stream implementation.
> Could add newline support to string_stream and alloc one extra byte for
> each fragment just in case we need to add a newline.
>
> The string_stream implementation would waste a lot a memory if you log
> many short lines. My current code wastes memory if you log lots of lines
> that don't fit in available space in the current fragment - though it's
> simple to shuffle the formatted string backwards to fill up the previous
> fragment (I just haven't done that yet).

Yeah: I think your implementation here is overall better than the
string_stream one. string_stream might handle concurrency a bit
better, which would be nice to have as people start wanting to try
multithreaded tests.

I think the ideal solution is:
- Update string_stream to basically use this implementation.
- Update the logging code to then use this via the string_stream API
(probably with some tweaks to handle newlines)
- Optimize the string_stream append implementation to not create a
temporary string, as string streams are written to logs often. (If you
were prepared to allow string stream fragments to have variable
lengths, and do some ownership shenanigans, this could even become
O(1), though I suspect it's not worth the added complexity.)

That being said, I don't think we'd need to land all of that at once.
Even if we ported to the suboptimal string_stream API now (which would
still gain us the extensible log and some concurrency support), and
optimized string_stream later if it turned out to be tricky, that'd be
fine. (I wouldn't want to hold this up if changing string_stream was
regressing the other code which uses it, for example.)

How does that sound?

-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature