Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH V2] dynamic_debug: Add config option of
 DYNAMIC_DEBUG_CORE
To:     Orson Zhai <orsonzhai@gmail.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Brendan Higgins <brendanhiggins@google.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Ard Biesheuvel <ardb@kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        David Gow <davidgow@google.com>,
        Mark Rutland <mark.rutland@arm.com>, joe@perches.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Android Kernel Team <kernel-team@android.com>,
        Orson Zhai <orson.unisoc@gmail.com>
References: <1585156347-8365-1-git-send-email-orson.unisoc@gmail.com>
 <CA+H2tpE5Fe5zJKEUM7H9xT10+ZGKVmZ2Aa5D7j_eq7dvZyX5wA@mail.gmail.com>
From:   Jason Baron <jbaron@akamai.com>
Message-ID: <b8119cba-e660-d831-1f09-f99641e3d938@akamai.com>
Date:   Mon, 30 Mar 2020 11:19:18 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <CA+H2tpE5Fe5zJKEUM7H9xT10+ZGKVmZ2Aa5D7j_eq7dvZyX5wA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 3/28/20 2:17 AM, Orson Zhai wrote:
> Hi Jason,
> 
> On Thu, Mar 26, 2020 at 1:13 AM Orson Zhai <orson.unisoc@gmail.com> wrote:
>>
>> Instead of enabling whole kernel with CONFIG_DYNAMIC_DEBUG, CONFIG_
>> DYNAMIC_DEBUG_CORE will only make the basic function definition and
>> exported symbols to be built without replacing pr_debug or dev_dbg
>> to dynamic version unless DEBUG is defined for any desired modules
> 
> How do you think about this idea?
> Optionally enable dynamic debug of modules by the DEBUG macro.
> 

Hi Orson,

So I like the idea of being able to use pr_debug() as a way for modules to
tie into dynamic_debug() without having to enable everything via
CONFIG_DYNAMIC_DEBUG. However the 'DEBUG' already has a specific meaning
which is to enable the printing or enable by default (can be turned off
at run-time if CONFIG_DYNAMIC_DEBUG is set).

So I think generally modules want the printing off by default. So maybe
what we want is instead of:

>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))

is:

>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG_MODULE))


That is introduce a new per-module definition I've called it
'DEBUG_MODULE' (perhaps we can name it better), that enables
dynamic debugging without having to turn it on globally. Then,
modules can in addition enabled 'DEBUG' if they want all the printing
on by default.

Thanks,

-Jason


> Best Regards,
> -Orson
> 
>> together by users.
>>
>> This is useful for people who only want to enable dynamic debug for some
>> specific kernel modules without worrying about whole kernel image size will
>> be significantly increased and more memory consumption caused by CONFIG_
>> DYNAMIC_DEBUG.
>>
>> Signed-off-by: Orson Zhai <orson.unisoc@gmail.com>
>> ---
>> Changes from V1:
>> - Rewrite commit message for more generic usage.
>> - Add combination use of CONFIG_DYNAMIC_DEBUG_CORE and DEBUG to enable
>>   dynamic debug seperately.
>> - Ignore empty _ddtable and return success when only the core is enabled.
>> - Fix all typoes.
>>
>>  include/linux/dev_printk.h    |  6 ++++--
>>  include/linux/dynamic_debug.h |  2 +-
>>  include/linux/printk.h        | 12 ++++++++----
>>  lib/Kconfig.debug             | 18 ++++++++++++++++++
>>  lib/Makefile                  |  2 +-
>>  lib/dynamic_debug.c           |  9 +++++++--
>>  6 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/dev_printk.h b/include/linux/dev_printk.h
>> index 5aad06b..ed40030 100644
>> --- a/include/linux/dev_printk.h
>> +++ b/include/linux/dev_printk.h
>> @@ -109,7 +109,8 @@ void _dev_info(const struct device *dev, const char *fmt, ...)
>>  #define dev_info(dev, fmt, ...)                                                \
>>         _dev_info(dev, dev_fmt(fmt), ##__VA_ARGS__)
>>
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>>  #define dev_dbg(dev, fmt, ...)                                         \
>>         dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
>>  #elif defined(DEBUG)
>> @@ -181,7 +182,8 @@ do {                                                                        \
>>         dev_level_ratelimited(dev_notice, dev, fmt, ##__VA_ARGS__)
>>  #define dev_info_ratelimited(dev, fmt, ...)                            \
>>         dev_level_ratelimited(dev_info, dev, fmt, ##__VA_ARGS__)
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>>  /* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>  #define dev_dbg_ratelimited(dev, fmt, ...)                             \
>>  do {                                                                   \
>> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
>> index 4cf02ec..abcd5fd 100644
>> --- a/include/linux/dynamic_debug.h
>> +++ b/include/linux/dynamic_debug.h
>> @@ -48,7 +48,7 @@ struct _ddebug {
>>
>>
>>
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG_CORE)
>>  int ddebug_add_module(struct _ddebug *tab, unsigned int n,
>>                                 const char *modname);
>>  extern int ddebug_remove_module(const char *mod_name);
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index 1e6108b..44d5378 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -292,7 +292,8 @@ extern int kptr_restrict;
>>   * These can be used to print at the various log levels.
>>   * All of these will print unconditionally, although note that pr_debug()
>>   * and other debug macros are compiled out unless either DEBUG is defined
>> - * or CONFIG_DYNAMIC_DEBUG is set.
>> + * or CONFIG_DYNAMIC_DEBUG is set, or both CONFIG_DYNAMIC_DEBUG_CORE and
>> + * DEBUG is defined.
>>   */
>>  #define pr_emerg(fmt, ...) \
>>         printk(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>> @@ -327,7 +328,8 @@ extern int kptr_restrict;
>>
>>
>>  /* If you are writing a driver, please use dev_dbg instead */
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>>  #include <linux/dynamic_debug.h>
>>
>>  /* dynamic_pr_debug() uses pr_fmt() internally so we don't need it here */
>> @@ -453,7 +455,8 @@ extern int kptr_restrict;
>>  #endif
>>
>>  /* If you are writing a driver, please use dev_dbg instead */
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>>  /* descriptor check is first to prevent flooding with "callbacks suppressed" */
>>  #define pr_debug_ratelimited(fmt, ...)                                 \
>>  do {                                                                   \
>> @@ -500,7 +503,8 @@ static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
>>
>>  #endif
>>
>> -#if defined(CONFIG_DYNAMIC_DEBUG)
>> +#if defined(CONFIG_DYNAMIC_DEBUG) || \
>> +       (defined(CONFIG_DYNAMIC_CORE) && defined(DEBUG))
>>  #define print_hex_dump_debug(prefix_str, prefix_type, rowsize, \
>>                              groupsize, buf, len, ascii)        \
>>         dynamic_hex_dump(prefix_str, prefix_type, rowsize,      \
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 69def4a..8381b19 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -99,6 +99,7 @@ config DYNAMIC_DEBUG
>>         default n
>>         depends on PRINTK
>>         depends on DEBUG_FS
>> +       select DYNAMIC_DEBUG_CORE
>>         help
>>
>>           Compiles debug level messages into the kernel, which would not
>> @@ -164,6 +165,23 @@ config DYNAMIC_DEBUG
>>           See Documentation/admin-guide/dynamic-debug-howto.rst for additional
>>           information.
>>
>> +config DYNAMIC_DEBUG_CORE
>> +       bool "Enable core functions of dynamic debug support"
>> +       depends on PRINTK
>> +       depends on DEBUG_FS
>> +       help
>> +         Enable this option to build ddebug_* and __dynamic_* routines
>> +         into kernel. If you want enable whole dynamic debug features,
>> +         select CONFIG_DYNAMIC_DEBUG directly and this option will be
>> +         automatically selected as well.
>> +
>> +         This option can be selected with DEBUG together which could be
>> +         defined for desired kernel modules to enable dynamic debug
>> +         features instead for whole kernel. Especially being used in
>> +         the case that kernel modules are built out of kernel tree to
>> +         have dynamic debug capabilities without affecting the kernel
>> +         base.
>> +
>>  config SYMBOLIC_ERRNAME
>>         bool "Support symbolic error names in printf"
>>         default y if PRINTK
>> diff --git a/lib/Makefile b/lib/Makefile
>> index 611872c..2096d83 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -183,7 +183,7 @@ lib-$(CONFIG_GENERIC_BUG) += bug.o
>>
>>  obj-$(CONFIG_HAVE_ARCH_TRACEHOOK) += syscall.o
>>
>> -obj-$(CONFIG_DYNAMIC_DEBUG) += dynamic_debug.o
>> +obj-$(CONFIG_DYNAMIC_DEBUG_CORE) += dynamic_debug.o
>>  obj-$(CONFIG_SYMBOLIC_ERRNAME) += errname.o
>>
>>  obj-$(CONFIG_NLATTR) += nlattr.o
>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>> index c604091..34f303a 100644
>> --- a/lib/dynamic_debug.c
>> +++ b/lib/dynamic_debug.c
>> @@ -1014,8 +1014,13 @@ static int __init dynamic_debug_init(void)
>>         int verbose_bytes = 0;
>>
>>         if (__start___verbose == __stop___verbose) {
>> -               pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
>> -               return 1;
>> +               if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG)) {
>> +                       pr_warn("_ddebug table is empty in a CONFIG_DYNAMIC_DEBUG build\n");
>> +                       return 1;
>> +               }
>> +               pr_info("Ignore empty _ddebug table in a core only build\n");
>> +               ddebug_init_success = 1;
>> +               return 0;
>>         }
>>         iter = __start___verbose;
>>         modname = iter->modname;
>> --
>> 2.7.4
>>