Re: [PATCH v2 1/6] kunit: Replace fixed-size log with dynamically-extending buffer

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 9/8/23 13:11, David Gow wrote:
> On Tue, 8 Aug 2023 at 20:35, Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
>>
>> Re-implement the log buffer as a list of buffer fragments that can
>> be extended as the size of the log info grows.
>>
>> When using parameterization the test case can run many times and create
>> a large amount of log. It's not really practical to keep increasing the
>> size of the fixed buffer every time a test needs more space. And a big
>> fixed buffer wastes memory.
>>
>> The original char *log pointer is replaced by a pointer to a list of
>> struct kunit_log_frag, each containing a fixed-size buffer.
>>
>> kunit_log_append() now attempts to append to the last kunit_log_frag in
>> the list. If there isn't enough space it will append a new kunit_log_frag
>> to the list. This simple implementation does not attempt to completely
>> fill the buffer in every kunit_log_frag.
>>
>> The 'log' member of kunit_suite, kunit_test_case and kunit_suite must be a
>> pointer because the API of kunit_log() requires that is the same type in
>> all  three structs. As kunit.log is a pointer to the 'log' of the current
>> kunit_case, it must be a pointer in the other two structs.
>>
>> The existing kunit-test.c log tests have been updated to build against the
>> new fragmented log implementation.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
> 
> Looks good to me.
> 
> A few small notes inline below, mostly around the possibility of
> either embedding the list_head in the kunit_case struct directly
> (rather than using a pointer), or of pointing directly to the first
> fragment, rather than a separately-allocated struct list_head. Neither
> are showstoppers, though (and if it increases complexity at all, it's
> possibly premature optimization).
>

I did start out trying to use the first fragment as the list head.
Trouble with this is that the functions in list.h expect to have a
dummy list_head node that is only the head, but not an actual list
member. It's possible to workaround this but the shenanigans involved is
likely to trip someone up later so reverted to doing the list the way
the API intended.

For the pointers, I did consider embedding the list_head instead of
using a pointer. But then the struct kunit can't refer to the
kunit_case list, it can only take it over. There can only be one list
head because the ->prev and ->next pointers of the first and last
members in the list can only point to one head.

After playing around with it I decided that it wasn't worth trying to
avoid the pointers. At least... it wasn't worth spending a lot of time
trying to avoid them for an initial implementation.

Maybe some magic with typeof() in the kunit_log() would let us use
different types for the members of kunit_suite, kunit_case, kunit?
Then the list_head can be directly embedded in the first two but a
pointer in kunit?

> Otherwise, some test nitpicks and the fact that this will need a
> trivial rebase due to the module filtering stuff landing in
> kselftest/kunit.
> 
> Reviewed-by: David Gow <davidgow@google.com>
> 

...

>>   static void kunit_log_newline_test(struct kunit *test)
>>   {
>> +       struct kunit_log_frag *frag;
>> +
>>          kunit_info(test, "Add newline\n");
>>          if (test->log) {
>> -               KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(test->log, "Add newline\n"),
>> -                       "Missing log line, full log:\n%s", test->log);
>> -               KUNIT_EXPECT_NULL(test, strstr(test->log, "Add newline\n\n"));
>> +               frag = list_first_entry(test->log, struct kunit_log_frag, list);
>> +               KUNIT_ASSERT_NOT_NULL_MSG(test, strstr(frag->buf, "Add newline\n"),
>> +                       "Missing log line, full log:\n%s", frag->buf);
> 
> I'm not super thrilled that this only operates on the first fragment.
> Could we at least note that this is not the "full log" in the
> assertion message here, and maybe also assert that the log hasn't
> grown to a second fragment?
> 

The only aim in this first patch is to make sure that kunit-test.c still
builds. I've added extra newline test cases in later patches.

...

> 
> I was going to wonder whether or not we should cache the length of the
> current fragment somewhere, but thinking about it, it's probably not
> worth it given we're only measuring a single fragment, and it's capped
> at 256 bytes.
> 
Yes, I had the same thought but decided to leave it as something that
can be done later. But as you say it's doubtful whether it's worth the
extra storage space when the buffer fragments are small. On x86_64
simply adding a length member could add 8 bytes per fragment (because of
rounding). If the size of the fragment buffer is capped at 256 we could
use single byte for the length and hope the compiler doesn't insert
padding between a char and a char[] array.

Take a look at what happens when you log a message to the kernel log and
by comparison this kunit logging is super-lightweight.

(I did look at whether we could re-use the existing kernel log
implementation but decided it was too heavily hardcoded to how the
kernel log works.)