| Message ID | cover.1709127473.git.geert+renesas@glider.be |
|---|---|
| Headers | show |
| Series | printk_index: Fix false positives | expand |
On Wed, Feb 28, 2024 at 03:00:03PM +0100, Geert Uytterhoeven wrote: > When printk-indexing is enabled, each dev_printk() invocation emits a > pi_entry structure. This is even true when the dev_printk() is > protected by an always-false check, as is typically the case for debug > messages: while the actual code to print the message is optimized out by > the compiler, the pi_entry structure is still emitted. > > Avoid emitting pi_entry structures for unavailable dev_printk() kernel > messages by: > 1. Introducing a dev_no_printk() helper, mimicked after the existing > no_printk() helper, which calls _dev_printk() instead of > dev_printk(), > 2. Replacing all "if (0) dev_printk(...)" constructs by calls to the > new helper. > > This reduces the size of an arm64 defconfig kernel with > CONFIG_PRINTK_INDEX=y by 957 KiB. ... > +/* > + * Dummy dev_printk for disabled debugging statements to use whilst maintaining dev_printk() > + * gcc's format checking. > + */ > +#define dev_no_printk(level, dev, fmt, ...) \ > + ({ \ > + if (0) \ > + _dev_printk(level, dev, fmt, ##__VA_ARGS__); \ > + })
On Wed, Feb 28, 2024 at 03:00:01PM +0100, Geert Uytterhoeven wrote: > Hi all, > > When printk-indexing is enabled, each printk() invocation emits a > pi_entry structure, containing the format string and other information > related to its location in the kernel sources. This is even true when > the printk() is protected by an always-false check, as is typically the > case for debug messages: while the actual code to print the message is > optimized out by the compiler, the pi_entry structure is still emitted. > Hence when debugging is disabled, this leads to the inclusion in the > index of lots of printk formats that cannot be emitted by the current > kernel. > > This series fixes that for the common debug helpers under include/. > It reduces the size of an arm64 defconfig kernel with > CONFIG_PRINTK_INDEX=y by ca. 1.5 MiB, or 28% of the overhead of > enabling CONFIG_PRINTK_INDEX=y. > > Notes: > - netdev_(v)dbg() and netif_(v)dbg() are not affected, as > net{dev,if}_printk() do not implement printk-indexing, except > for the single global internal instance of __netdev_printk(). > - This series fixes only debug code in global header files under > include/. There are more cases to fix in subsystem-specific header > files and in sources files. The whole series makes a lot of sense and gives a good examples for above mentioned subsystem specific code on how to do it in a better way. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hi Andy, On Wed, Feb 28, 2024 at 6:39 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Feb 28, 2024 at 03:00:03PM +0100, Geert Uytterhoeven wrote: > > When printk-indexing is enabled, each dev_printk() invocation emits a > > pi_entry structure. This is even true when the dev_printk() is > > protected by an always-false check, as is typically the case for debug > > messages: while the actual code to print the message is optimized out by > > the compiler, the pi_entry structure is still emitted. > > > > Avoid emitting pi_entry structures for unavailable dev_printk() kernel > > messages by: > > 1. Introducing a dev_no_printk() helper, mimicked after the existing > > no_printk() helper, which calls _dev_printk() instead of > > dev_printk(), > > 2. Replacing all "if (0) dev_printk(...)" constructs by calls to the > > new helper. > > > > This reduces the size of an arm64 defconfig kernel with > > CONFIG_PRINTK_INDEX=y by 957 KiB. > > ... > > > +/* > > + * Dummy dev_printk for disabled debugging statements to use whilst maintaining > > dev_printk() I fully agree. But the surrounding comments don't, so I gave in. Gr{oetje,eeting}s, Geert
On Wed, Feb 28, 2024 at 08:33:19PM +0100, Geert Uytterhoeven wrote: > On Wed, Feb 28, 2024 at 6:39 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Feb 28, 2024 at 03:00:03PM +0100, Geert Uytterhoeven wrote: ... > > dev_printk() > > I fully agree. But the surrounding comments don't, so I gave in. Is there any better justification to keep a technical debt? I mean, the comment here can be better than existed ones, if you feel uncomfortable with it, you can fix the rest in a separate patch. Would it be a big deal? :-)
On 2/28/24 22:00, Geert Uytterhoeven wrote: > Hi all, > > When printk-indexing is enabled, each printk() invocation emits a > pi_entry structure, containing the format string and other information > related to its location in the kernel sources. This is even true when > the printk() is protected by an always-false check, as is typically the > case for debug messages: while the actual code to print the message is > optimized out by the compiler, the pi_entry structure is still emitted. > Hence when debugging is disabled, this leads to the inclusion in the > index of lots of printk formats that cannot be emitted by the current > kernel. > > This series fixes that for the common debug helpers under include/. > It reduces the size of an arm64 defconfig kernel with > CONFIG_PRINTK_INDEX=y by ca. 1.5 MiB, or 28% of the overhead of > enabling CONFIG_PRINTK_INDEX=y. > > Notes: > - netdev_(v)dbg() and netif_(v)dbg() are not affected, as > net{dev,if}_printk() do not implement printk-indexing, except > for the single global internal instance of __netdev_printk(). > - This series fixes only debug code in global header files under > include/. There are more cases to fix in subsystem-specific header > files and in sources files. > > Thanks for your comments! > > Geert Uytterhoeven (4): > printk: Let no_printk() use _printk() > dev_printk: Add and use dev_no_printk() > dyndbg: Use *no_printk() helpers > ceph: Use no_printk() helper > > include/linux/ceph/ceph_debug.h | 18 +++++++----------- > include/linux/dev_printk.h | 25 +++++++++++++------------ > include/linux/dynamic_debug.h | 4 ++-- > include/linux/printk.h | 2 +- > 4 files changed, 23 insertions(+), 26 deletions(-) > This series LGTM. Reviewed-by: Xiubo Li <xiubli@redhat.com>
Thanks for working on this! This whole patchset looks good to me.
Reviewed-by: Chris Down <chris@chrisdown.name>
On Wed 2024-02-28 15:00:01, Geert Uytterhoeven wrote: > Hi all, > > When printk-indexing is enabled, each printk() invocation emits a > pi_entry structure, containing the format string and other information > related to its location in the kernel sources. This is even true when > the printk() is protected by an always-false check, as is typically the > case for debug messages: while the actual code to print the message is > optimized out by the compiler, the pi_entry structure is still emitted. > Hence when debugging is disabled, this leads to the inclusion in the > index of lots of printk formats that cannot be emitted by the current > kernel. > > This series fixes that for the common debug helpers under include/. > It reduces the size of an arm64 defconfig kernel with > CONFIG_PRINTK_INDEX=y by ca. 1.5 MiB, or 28% of the overhead of > enabling CONFIG_PRINTK_INDEX=y. > > Notes: > - netdev_(v)dbg() and netif_(v)dbg() are not affected, as > net{dev,if}_printk() do not implement printk-indexing, except > for the single global internal instance of __netdev_printk(). > - This series fixes only debug code in global header files under > include/. There are more cases to fix in subsystem-specific header > files and in sources files. > > Thanks for your comments! > > Geert Uytterhoeven (4): > printk: Let no_printk() use _printk() > dev_printk: Add and use dev_no_printk() > dyndbg: Use *no_printk() helpers > ceph: Use no_printk() helper > > include/linux/ceph/ceph_debug.h | 18 +++++++----------- > include/linux/dev_printk.h | 25 +++++++++++++------------ > include/linux/dynamic_debug.h | 4 ++-- > include/linux/printk.h | 2 +- > 4 files changed, 23 insertions(+), 26 deletions(-) The whole series looks good to me: Reviewed-by: Petr Mladek <pmladek@suse.com> I am going take it via printk tree for 6.10. I am sorry that I haven't looked at it in time before the merge window for 6.9. I have been snowed under various tasks. The changes are not complicated. But they also are not critical to be pushed an expedite way. Best Regards, Petr
On Tue 2024-03-19 16:09:01, Petr Mladek wrote: > On Wed 2024-02-28 15:00:01, Geert Uytterhoeven wrote: > > Hi all, > > > > When printk-indexing is enabled, each printk() invocation emits a > > pi_entry structure, containing the format string and other information > > related to its location in the kernel sources. This is even true when > > the printk() is protected by an always-false check, as is typically the > > case for debug messages: while the actual code to print the message is > > optimized out by the compiler, the pi_entry structure is still emitted. > > Hence when debugging is disabled, this leads to the inclusion in the > > index of lots of printk formats that cannot be emitted by the current > > kernel. > > > > This series fixes that for the common debug helpers under include/. > > It reduces the size of an arm64 defconfig kernel with > > CONFIG_PRINTK_INDEX=y by ca. 1.5 MiB, or 28% of the overhead of > > enabling CONFIG_PRINTK_INDEX=y. > > > > Notes: > > - netdev_(v)dbg() and netif_(v)dbg() are not affected, as > > net{dev,if}_printk() do not implement printk-indexing, except > > for the single global internal instance of __netdev_printk(). > > - This series fixes only debug code in global header files under > > include/. There are more cases to fix in subsystem-specific header > > files and in sources files. > > > > Thanks for your comments! > > > > Geert Uytterhoeven (4): > > printk: Let no_printk() use _printk() > > dev_printk: Add and use dev_no_printk() > > dyndbg: Use *no_printk() helpers > > ceph: Use no_printk() helper > > > > include/linux/ceph/ceph_debug.h | 18 +++++++----------- > > include/linux/dev_printk.h | 25 +++++++++++++------------ > > include/linux/dynamic_debug.h | 4 ++-- > > include/linux/printk.h | 2 +- > > 4 files changed, 23 insertions(+), 26 deletions(-) > > The whole series looks good to me: > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > I am going take it via printk tree for 6.10. JFYI, the patchset has been committed into printk/linux.git, branch for-6.10. Best Regards, Petr
Hi all, When printk-indexing is enabled, each printk() invocation emits a pi_entry structure, containing the format string and other information related to its location in the kernel sources. This is even true when the printk() is protected by an always-false check, as is typically the case for debug messages: while the actual code to print the message is optimized out by the compiler, the pi_entry structure is still emitted. Hence when debugging is disabled, this leads to the inclusion in the index of lots of printk formats that cannot be emitted by the current kernel. This series fixes that for the common debug helpers under include/. It reduces the size of an arm64 defconfig kernel with CONFIG_PRINTK_INDEX=y by ca. 1.5 MiB, or 28% of the overhead of enabling CONFIG_PRINTK_INDEX=y. Notes: - netdev_(v)dbg() and netif_(v)dbg() are not affected, as net{dev,if}_printk() do not implement printk-indexing, except for the single global internal instance of __netdev_printk(). - This series fixes only debug code in global header files under include/. There are more cases to fix in subsystem-specific header files and in sources files. Thanks for your comments! Geert Uytterhoeven (4): printk: Let no_printk() use _printk() dev_printk: Add and use dev_no_printk() dyndbg: Use *no_printk() helpers ceph: Use no_printk() helper include/linux/ceph/ceph_debug.h | 18 +++++++----------- include/linux/dev_printk.h | 25 +++++++++++++------------ include/linux/dynamic_debug.h | 4 ++-- include/linux/printk.h | 2 +- 4 files changed, 23 insertions(+), 26 deletions(-)