It's wasteful to call vsnprintf() only to figure out the length of the
string. The string might fit in the available buffer space but we have to
vsnprintf() again to do that.
Instead, attempt to format the string to the available buffer at the same
time as finding the string length. Only if the string didn't fit the
available space is it necessary to extend the log and format the string
again to a new fragment buffer.
Signed-off-by: Richard Fitzgerald <[email protected]>
---
lib/kunit/test.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 28d0048d6171..230ec5e9824f 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
if (!log)
return;
- /* Evaluate length of line to add to log */
+ frag = list_last_entry(log, struct kunit_log_frag, list);
+ log_len = strlen(frag->buf);
+ len_left = sizeof(frag->buf) - log_len - 1;
+
+ /* Attempt to print formatted line to current fragment */
va_start(args, fmt);
- len = vsnprintf(NULL, 0, fmt, args) + 1;
+ len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1;
va_end(args);
+ if (len <= len_left)
+ goto out_newline;
+
+ /* Abandon the truncated result of vsnprintf */
+ frag->buf[log_len] = '\0';
+
if (len > sizeof(frag->buf) - 1) {
va_start(args, fmt);
tmp = kvasprintf(GFP_KERNEL, fmt, args);
@@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
return;
}
- frag = list_last_entry(log, struct kunit_log_frag, list);
- log_len = strlen(frag->buf);
- len_left = sizeof(frag->buf) - log_len - 1;
-
- if (len > len_left) {
- frag = kunit_log_extend(log);
- if (!frag)
- return;
-
- len_left = sizeof(frag->buf) - 1;
- log_len = 0;
- }
+ frag = kunit_log_extend(log);
+ if (!frag)
+ return;
/* Print formatted line to the log */
va_start(args, fmt);
- vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args);
+ vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args);
va_end(args);
-
+out_newline:
/* Add newline to end of log if not already present. */
kunit_log_newline(frag);
}
--
2.30.2
On Wed, Aug 9, 2023 at 10:54 AM Richard Fitzgerald
<[email protected]> wrote:
>
> It's wasteful to call vsnprintf() only to figure out the length of the
> string. The string might fit in the available buffer space but we have to
> vsnprintf() again to do that.
>
> Instead, attempt to format the string to the available buffer at the same
> time as finding the string length. Only if the string didn't fit the
> available space is it necessary to extend the log and format the string
> again to a new fragment buffer.
>
> Signed-off-by: Richard Fitzgerald <[email protected]>
Hello!
I find this version slightly more confusing but I realize that the
extra vsnprintf() call is unnecessary. So I am ok with this version
except for one question below.
I am curious what David Gow thinks and it would also be good to have
another set of eyes here.
My testing of this showed no problems.
Thanks!
-Rae
> ---
> lib/kunit/test.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 28d0048d6171..230ec5e9824f 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
> if (!log)
> return;
>
> - /* Evaluate length of line to add to log */
> + frag = list_last_entry(log, struct kunit_log_frag, list);
> + log_len = strlen(frag->buf);
> + len_left = sizeof(frag->buf) - log_len - 1;
> +
> + /* Attempt to print formatted line to current fragment */
> va_start(args, fmt);
> - len = vsnprintf(NULL, 0, fmt, args) + 1;
> + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1;
> va_end(args);
>
> + if (len <= len_left)
> + goto out_newline;
> +
> + /* Abandon the truncated result of vsnprintf */
> + frag->buf[log_len] = '\0';
> +
> if (len > sizeof(frag->buf) - 1) {
> va_start(args, fmt);
> tmp = kvasprintf(GFP_KERNEL, fmt, args);
> @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
> return;
> }
>
> - frag = list_last_entry(log, struct kunit_log_frag, list);
> - log_len = strlen(frag->buf);
> - len_left = sizeof(frag->buf) - log_len - 1;
> -
> - if (len > len_left) {
> - frag = kunit_log_extend(log);
> - if (!frag)
> - return;
> -
> - len_left = sizeof(frag->buf) - 1;
> - log_len = 0;
> - }
> + frag = kunit_log_extend(log);
Are we meant to be using the remainder of the last fragment to its
capacity or just start again with a new fragment and leave some of the
last fragment empty? I worry we abandoned using the last fragment or
is that the intention? Let me know if I understand this correctly.
> + if (!frag)
> + return;
>
> /* Print formatted line to the log */
> va_start(args, fmt);
> - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args);
> + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args);
> va_end(args);
> -
> +out_newline:
> /* Add newline to end of log if not already present. */
> kunit_log_newline(frag);
> }
> --
> 2.30.2
>
On Wed, 9 Aug 2023 at 23:54, Richard Fitzgerald
<[email protected]> wrote:
>
> It's wasteful to call vsnprintf() only to figure out the length of the
> string. The string might fit in the available buffer space but we have to
> vsnprintf() again to do that.
>
> Instead, attempt to format the string to the available buffer at the same
> time as finding the string length. Only if the string didn't fit the
> available space is it necessary to extend the log and format the string
> again to a new fragment buffer.
>
> Signed-off-by: Richard Fitzgerald <[email protected]>
> ---
This looks good.
The only case I'm not totally convinced about is the last one, where
the message doesn't fit in the current log fragment, but is not a
whole fragment itself. I'm not sure if the added efficiency of not
doing a second vsprintf() is worth the memory fragmentation of always
starting a new fragment: the worst case where a log message is just
over half the length of frag->buf would result in every message being
in its own fragment (which would not necessarily have a consistent
size), and memory use would be ~doubled.
But I could accept it either way: until there's a real-world example
which strongly benefits from either implementation, let's go with
whatever we have working.
Reviewed-by: David Gow <[email protected]>
Cheers,
-- David
> lib/kunit/test.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 28d0048d6171..230ec5e9824f 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -196,11 +196,21 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
> if (!log)
> return;
>
> - /* Evaluate length of line to add to log */
> + frag = list_last_entry(log, struct kunit_log_frag, list);
> + log_len = strlen(frag->buf);
> + len_left = sizeof(frag->buf) - log_len - 1;
> +
> + /* Attempt to print formatted line to current fragment */
> va_start(args, fmt);
> - len = vsnprintf(NULL, 0, fmt, args) + 1;
> + len = vsnprintf(frag->buf + log_len, len_left, fmt, args) + 1;
> va_end(args);
>
> + if (len <= len_left)
> + goto out_newline;
> +
> + /* Abandon the truncated result of vsnprintf */
> + frag->buf[log_len] = '\0';
> +
> if (len > sizeof(frag->buf) - 1) {
> va_start(args, fmt);
> tmp = kvasprintf(GFP_KERNEL, fmt, args);
> @@ -212,24 +222,15 @@ void kunit_log_append(struct list_head *log, const char *fmt, ...)
> return;
> }
>
> - frag = list_last_entry(log, struct kunit_log_frag, list);
> - log_len = strlen(frag->buf);
> - len_left = sizeof(frag->buf) - log_len - 1;
> -
> - if (len > len_left) {
> - frag = kunit_log_extend(log);
> - if (!frag)
> - return;
> -
> - len_left = sizeof(frag->buf) - 1;
> - log_len = 0;
> - }
> + frag = kunit_log_extend(log);
> + if (!frag)
> + return;
>
> /* Print formatted line to the log */
> va_start(args, fmt);
> - vsnprintf(frag->buf + log_len, min(len, len_left), fmt, args);
> + vsnprintf(frag->buf, sizeof(frag->buf) - 1, fmt, args);
> va_end(args);
> -
> +out_newline:
> /* Add newline to end of log if not already present. */
> kunit_log_newline(frag);
> }
> --
> 2.30.2
>