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;
Message-ID: <1cba22b4-d8e2-e5bf-98b2-597dd1797304@opensource.cirrus.com>
Date:   Thu, 10 Aug 2023 16:09:15 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH v3 3/7] kunit: Handle logging of lines longer than the
 fragment buffer size
Content-Language: en-US
To:     David Gow <davidgow@google.com>
CC:     <brendan.higgins@linux.dev>, <rmoar@google.com>,
        <linux-kselftest@vger.kernel.org>, <kunit-dev@googlegroups.com>,
        <linux-kernel@vger.kernel.org>, <patches@opensource.cirrus.com>
References: <20230809155438.22470-1-rf@opensource.cirrus.com>
 <20230809155438.22470-4-rf@opensource.cirrus.com>
 <CABVgOSn4PWT6+TobiJd+ppmPXsL+0qtLdazgjuQmfymUfkYhnA@mail.gmail.com>
From:   Richard Fitzgerald <rf@opensource.cirrus.com>
In-Reply-To: <CABVgOSn4PWT6+TobiJd+ppmPXsL+0qtLdazgjuQmfymUfkYhnA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/8/23 15:38, David Gow wrote:
> On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald
> <rf@opensource.cirrus.com> 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@opensource.cirrus.com>
>> ---
> 
> 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).