Hi.
I tested this, and seems working.
Please see some comments below.
On Sat, Dec 7, 2019 at 10:43 AM Tetsuo Handa
<penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Since kernel configs provided by syzbot are close to "make allyesconfig",
> it takes long time to rebuild. This is especially waste of time when we
> need to rebuild for many times (e.g. doing manual printk() inspection,
> bisect operations).
>
> We can save time if we can exclude modules which are irrelevant to each
> problem. But "make localmodconfig" cannot exclude modules which are built
> into vmlinux because /sbin/lsmod output is used as the source of modules.
>
> Therefore, this patch adds "make yes2modconfig" which converts from =y
> to =m if possible. After confirming that the interested problem is still
> reproducible, we can try "make localmodconfig" (and/or manually tune
> based on "Modules linked in:" line) in order to exclude modules which are
> irrelevant to the interested problem. While we are at it, this patch also
> adds "make mod2yesconfig" which converts from =m to =y in case someone
> wants to convert from =m to =y after "make localmodconfig".
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> ---
> Changelog since v1:
> - Updated the available 'help' targets in Makefile.
>
> scripts/kconfig/Makefile | 4 +++-
> scripts/kconfig/conf.c | 17 +++++++++++++++++
> scripts/kconfig/confdata.c | 26 ++++++++++++++++++++++++++
> scripts/kconfig/lkc.h | 3 +++
> 4 files changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile
> index 2f1a59fa5169..4da36f83277f 100644
> --- a/scripts/kconfig/Makefile
> +++ b/scripts/kconfig/Makefile
> @@ -67,7 +67,7 @@ localyesconfig localmodconfig: $(obj)/conf
> # deprecated for external use
> simple-targets := oldconfig allnoconfig allyesconfig allmodconfig \
> alldefconfig randconfig listnewconfig olddefconfig syncconfig \
> - helpnewconfig
> + helpnewconfig yes2modconfig mod2yesconfig
>
> PHONY += $(simple-targets)
>
> @@ -135,6 +135,8 @@ help:
> @echo ' allmodconfig - New config selecting modules when possible'
> @echo ' alldefconfig - New config with all symbols set to default'
> @echo ' randconfig - New config with random answer to all options'
> + @echo ' yes2modconfig - Change answers from yes to mod if possible'
> + @echo ' mod2yesconfig - Change answers from mod to yes'
For consistency,
"mod to yes if possible" ?
Turning mod into yes may not be possible
due to some dependencies.
For example, see lib/Kconfig.debug
'depends on m' is a common idiom to limit a CONFIG option
to either m or n.
> @echo ' listnewconfig - List new options'
> @echo ' helpnewconfig - List new options and help text'
> @echo ' olddefconfig - Same as oldconfig but sets new symbols to their'
> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
> index 1f89bf1558ce..ae9ddf88c64d 100644
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -34,6 +34,8 @@ enum input_mode {
> listnewconfig,
> helpnewconfig,
> olddefconfig,
> + yes2modconfig,
> + mod2yesconfig,
> };
> static enum input_mode input_mode = oldaskconfig;
>
> @@ -467,6 +469,8 @@ static struct option long_opts[] = {
> {"listnewconfig", no_argument, NULL, listnewconfig},
> {"helpnewconfig", no_argument, NULL, helpnewconfig},
> {"olddefconfig", no_argument, NULL, olddefconfig},
> + {"yes2modconfig", no_argument, NULL, yes2modconfig},
> + {"mod2yesconfig", no_argument, NULL, mod2yesconfig},
> {NULL, 0, NULL, 0}
> };
>
> @@ -489,6 +493,8 @@ static void conf_usage(const char *progname)
> printf(" --allmodconfig New config where all options are answered with mod\n");
> printf(" --alldefconfig New config with all symbols set to default\n");
> printf(" --randconfig New config with random answer to all options\n");
> + printf(" --yes2modconfig Change answers from yes to mod if possible\n");
> + printf(" --mod2yesconfig Change answers from mod to yes\n");
Same here.
"if possible"
> }
>
> int main(int ac, char **av)
> @@ -553,6 +559,8 @@ int main(int ac, char **av)
> case listnewconfig:
> case helpnewconfig:
> case olddefconfig:
> + case yes2modconfig:
> + case mod2yesconfig:
> break;
> case '?':
> conf_usage(progname);
> @@ -587,6 +595,8 @@ int main(int ac, char **av)
> case listnewconfig:
> case helpnewconfig:
> case olddefconfig:
> + case yes2modconfig:
> + case mod2yesconfig:
> conf_read(NULL);
> break;
> case allnoconfig:
> @@ -638,6 +648,11 @@ int main(int ac, char **av)
> }
> }
>
> + if (input_mode == yes2modconfig)
> + conf_rewrite_mod_or_yes(def_y2m);
> + else if (input_mode == mod2yesconfig)
> + conf_rewrite_mod_or_yes(def_m2y);
> +
For consistency, why not put these lines into the switch statement below?
> switch (input_mode) {
> case allnoconfig:
> conf_set_all_new_symbols(def_no);
> @@ -669,6 +684,8 @@ int main(int ac, char **av)
> case listnewconfig:
> case helpnewconfig:
> case syncconfig:
> + case yes2modconfig:
> + case mod2yesconfig:
This looks like
yes2mod/mod2yesconfig are interactive modes.
Why do you need this?
I believe yes2mod/mod2yesconfig
should work non-interactively.
> /* Update until a loop caused no more changes */
> do {
> conf_cnt = 0;
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 3569d2dec37c..6832a04a1aa4 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -1362,3 +1362,29 @@ bool conf_set_all_new_symbols(enum conf_def_mode mode)
>
> return has_changed;
> }
> +
> +bool conf_rewrite_mod_or_yes(enum conf_def_mode mode)
If you do not use the return value of this function,
could you make it into a void function?
> +{
> + struct symbol *sym;
> + int i;
> + bool has_changed = false;
> +
> + if (mode == def_y2m) {
> + for_all_symbols(i, sym) {
> + if (sym_get_type(sym) == S_TRISTATE &&
> + sym->def[S_DEF_USER].tri == yes) {
> + sym->def[S_DEF_USER].tri = mod;
> + has_changed = true;
sym_add_change_count(1); seems the convention way
to inform kconfig of some options being updated.
> + }
> + }
> + } else if (mode == def_m2y) {
> + for_all_symbols(i, sym) {
> + if (sym_get_type(sym) == S_TRISTATE &&
> + sym->def[S_DEF_USER].tri == mod) {
> + sym->def[S_DEF_USER].tri = yes;
> + has_changed = true;
> + }
> + }
> + }
> + return has_changed;
Perhaps, the similar for_all_symbols() could be merged, like follows:
(this might be a matter of preference, though)
tristate old_val = (mode == def_y2m) ? yes : mod;
tristate new_val = (mode == def_y2m) ? mod : yes;
for_all_symbols(i, sym) {
if (sym_get_type(sym) == S_TRISTATE &&
sym->def[S_DEF_USER].tri == old_val) {
sym->def[S_DEF_USER].tri = new_val;
sym_add_change_count(1);
BTW, I have never contributed to the syzbot bug shooting.
So, please teach me if you know this:
Is there a a specific reason why the config set for syzbot
is close to allyesconfig instead of allmodconfig?
Thanks.
--
Best Regards
Masahiro Yamada
