-----Original Message----- From: Brendan Higgins
On Fri, Aug 30, 2019 at 11:22:43PM +0000, Tim.Bird@sony.com wrote:
-----Original Message----- From: Brendan Higgins
On Fri, Aug 30, 2019 at 3:46 PM Joe Perches joe@perches.com wrote:
On Fri, 2019-08-30 at 21:58 +0000, Tim.Bird@sony.com 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.
When in doubt, do what the sub-system maintainer says. I'd go with Sergey's suggestion. Maintainers often are juggling a host of issues, and weighing new features and usages of their system against their long-term plans for their sub-system. Sometimes they have time to communicate all the intricacies of their counter-proposals, and sometimes not.
But they know their system best, and much more often than not provide sound advice.
If you don't have a strong feeling about it, just do what they say. -- Tim