Re: [PATCH v2] kunit: fix failure to build without printk
From: shuah
Date: Mon Sep 02 2019 - 10:40:03 EST
On 9/2/19 6:52 AM, Petr Mladek wrote:
On Fri 2019-08-30 16:37:10, Brendan Higgins wrote:
On Fri, Aug 30, 2019 at 11:22:43PM +0000, Tim.Bird@xxxxxxxx wrote:
-----Original Message-----
From: Brendan Higgins
On Fri, Aug 30, 2019 at 3:46 PM Joe Perches <joe@xxxxxxxxxxx> wrote:
On Fri, 2019-08-30 at 21:58 +0000, Tim.Bird@xxxxxxxx wrote:
From: Joe Perches
[]
IMHO %pV should be avoided if possible. Just because people are
doing it doesn't mean it should be used when it is not necessary.
Well, as the guy that created %pV, I of course
have a different opinion.
then wouldn't it be easier to pass in the
kernel level as a separate parameter and then strip off all printk
headers like this:
Depends on whether or not you care for overall
object size. Consolidated formats with the
embedded KERN_<LEVEL> like suggested are smaller
overall object size.
This is an argument I can agree with. I'm generally in favor of
things that lessen kernel size creep. :-)
As am I.
Sorry, to be clear, we are talking about the object size penalty due
to adding a single parameter to a function. Is that right?
Not exactly. The argument is that pre-pending the different KERN_LEVEL
strings onto format strings can result in several versions of nearly identical strings
being compiled into the object file. By parameterizing this (that is, adding
'%s' into the format string, and putting the level into the string as an argument),
it prevents this duplication of format strings.
I haven't seen the data on duplication of format strings, and how much this
affects it, but little things can add up. Whether it matters in this case depends
on whether the format strings that kunit uses are also used elsewhere in the kernel,
and whether these same format strings are used with multiple kernel message levels.
-- Tim
I thought this portion of the discussion was about whether Joe's version
of kunit_printk was better or my critique of his version of kunit_printk:
Joe's:
-void kunit_printk(const char *level,
- const struct kunit *test,
- const char *fmt, ...)
+void kunit_printk(const struct kunit *test, const char *fmt, ...)
{
+ char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
struct va_format vaf;
va_list args;
+ int kern_level;
va_start(args, fmt);
+ while ((kern_level = printk_get_level(fmt)) != 0) {
+ size_t size = printk_skip_level(fmt) - fmt;
+
+ if (kern_level >= '0' && kern_level <= '7') {
+ memcpy(lvl, fmt, size);
+ lvl[size] = '\0';
+ }
+ fmt += size;
+ }
+
vaf.fmt = fmt;
vaf.va = &args;
- kunit_vprintk(test, level, &vaf);
+ printk("%s\t# %s %pV\n", lvl, test->name, &vaf);
va_end(args);
}
Mine:
void kunit_printk(const char *level,
const struct kunit *test,
const char *fmt, ...)
{
struct va_format vaf;
va_list args;
va_start(args, fmt);
+ fmt = printk_skip_headers(fmt);
+
vaf.fmt = fmt;
vaf.va = &args;
- kunit_vprintk(test, level, &vaf);
+ printk("%s\t# %s %pV\n", level, test->name, &vaf);
va_end(args);
}
I thought you and Joe were arguing that "Joe's" resulted in a smaller
object size than "Mine" (not to be confused with the actual patch I
presented here, which is what Sergey suggested I do on a different
thread).
I really don't feel strongly about what Sergey suggested I do (which is
what this patch originally introduced), versus, what Joe suggested,
versus what I suggested in response to Joe (or any of the things
suggested on other threads). I just want to pick one, fix the breakage
in linux-next, and move on with my life.
I am a bit lost in all the versions ;-) Though, I like most this
patch. I think that it is based on Sergey's suggestion.
I am too.
I think that object size is not a huge concern for unit testing.
Also if I get it correctly, the object is bigger only when
the same string is used with different log levels. I am not
sure how often this happen.
Feel free to use for this patch:
Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>
Brendan,
Send me the version Sergey suggested with a short summary of the
discussion in the commit log. Tag it v3 so I don't pull the wrong
patch in.
I am going to just ignore the checkpatch warn on this and get it in.
Thanks for the discussion. It helped me clarify my understanding of
the printk.
thanks,
-- Shuah