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.)