| Message ID | 20191207014238.5507-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kconfig: Add yes2modconfig and mod2yesconfig targets. | expand |
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@i-love.sakura.ne.jp> 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@I-love.SAKURA.ne.jp> > Cc: Dmitry Vyukov <dvyukov@google.com> > --- > 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
Thank you for reviewing. On 2019/12/16 20:10, Masahiro Yamada wrote: > 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? I don't know. But I guess that all-in-one vmlinux file is easier to use (e.g. no need to copy .ko files into initramfs nor /lib/modules/ directory in the root filesystem image, no need to fetch .ko files when calculating locations in the source code from kernel addresses, no need to worry about availability of .ko loader program and request_module() dependency). >> @@ -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. I worried that simple s/=y$/=m/ or s/=m$/=y/ on tristate config fails to satisfy requirement/dependency. And I assumed that /* Update until a loop caused no more changes */ do { conf_cnt = 0; check_conf(&rootmenu); } while (conf_cnt); is the location to make modifications in order to adjust requirement/dependency. But I might be wrong. I just assumed that we should behave as if "make oldconfig" after doing simple s/=y$/=m/ or s/=m$/=y/ on tristate config. Does some later function automatically adjust requirement/dependency ? If yes, >> @@ -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? conf_rewrite_mod_or_yes() should be put into the switch statement. >> 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? OK. >> +{ >> + 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. Then, we can do "sym_add_change_count(1);" instead of "return has_changed;".
On Mon, Dec 16, 2019 at 9:59 PM Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > Thank you for reviewing. > > On 2019/12/16 20:10, Masahiro Yamada wrote: > > 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? > > I don't know. But I guess that all-in-one vmlinux file is easier to use > (e.g. no need to copy .ko files into initramfs nor /lib/modules/ directory > in the root filesystem image, no need to fetch .ko files when calculating > locations in the source code from kernel addresses, no need to worry about > availability of .ko loader program and request_module() dependency). OK. I just thought allmodconfig would be more convenient to trim unrelated code without switching yes/mod back and forth. Anyway... > >> @@ -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. > > I worried that simple s/=y$/=m/ or s/=m$/=y/ on tristate config fails to satisfy > requirement/dependency. conf_write() calls sym_calc_value() for every symbol before writing them to the .config file. > And I assumed that > > /* Update until a loop caused no more changes */ > do { > conf_cnt = 0; > check_conf(&rootmenu); > } while (conf_cnt); > > is the location to make modifications in order to adjust requirement/dependency. This is not the place to meet requirement/dependency. This loop requires the user to input his/her preference for all visible symbols. oldaskconfig, oldconfig and syncconfig are meant to be interactive (it shows a prompt for every new symbol), that is why they runs this loop. > But I might be wrong. I just assumed that we should behave as if "make oldconfig" > after doing simple s/=y$/=m/ or s/=m$/=y/ on tristate config. > Does some later function automatically adjust requirement/dependency ? Yes, conf_write(). Thanks. > If yes, > > >> @@ -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? > > conf_rewrite_mod_or_yes() should be put into the switch statement. > > >> 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? > > OK. > > >> +{ > >> + 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. > > Then, we can do "sym_add_change_count(1);" instead of "return has_changed;". > -- Best Regards Masahiro Yamada
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' @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"); } 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); + 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: /* 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) +{ + 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; + } + } + } 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; +} diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index 4fb16f316626..e8f3238dcc70 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -34,6 +34,8 @@ enum conf_def_mode { def_default, def_yes, def_mod, + def_y2m, + def_m2y, def_no, def_random }; @@ -52,6 +54,7 @@ const char *conf_get_configname(void); void sym_set_change_count(int count); void sym_add_change_count(int count); bool conf_set_all_new_symbols(enum conf_def_mode mode); +bool conf_rewrite_mod_or_yes(enum conf_def_mode mode); void set_all_choice_values(struct symbol *csym); /* confdata.c and expr.c */
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@I-love.SAKURA.ne.jp> Cc: Dmitry Vyukov <dvyukov@google.com> --- 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(-)