| Message ID | 20130805083943.GA14782@sepie.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Michal, All, On Monday 05 August 2013 10:39:43 Michal Marek wrote: > On Fri, Aug 02, 2013 at 01:28:25PM -0700, Linus Torvalds wrote: > > On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > > > > > Yinghai is still working on another SR-IOV-related fix or two, which > > > will be simpler if pciehp is non-modular, so I included the Kconfig > > > changes now to get them in earlier. > > > > Hmm. Doing a trivial "make allmoconfig" for testing, I get > > > > include/config/auto.conf:3014:warning: symbol value 'm' invalid for > > HOTPLUG_PCI_PCIE > > include/config/auto.conf:4711:warning: symbol value 'm' invalid for > > HOTPLUG_PCI > > > > but that may be a build system issue with stale data from the > > *previous* "make allmodconfig". Regardless, that makes me worried. > > > > Adding Michal Marek to the discussion. I'm currently doing a new "make > > allmodconfig" after having done a "git clean -dqfx" to see if the > > error remains. If it does, I will unpull. If it is gone, I'm going to > > assume the Kconfig changes are ok, but that our build system is > > missing some dependency. > > Added Yann and the linux-kbuild list to CC. Reproducer: > > git checkout 1fe0135 > make mrproper > make allmodconfig > make silentoldconfig > git checkout aa8032b > make allmodconfig > make silentoldconfig > > conf_write_autoconf() first calls conf_split_config() to generate the > include/config/**.h hierarchy, then generates include/config/auto.conf. > For some reason, conf_split_config() reads include/config/auto.conf, > which may not exist yet or may be out of date. Yann, can anything break > if we simply do not read that file from conf_split_config(), like this? Sorry, that's a part I'm not sure about, and seems non-trivial. I'll look at it tonight (UTC+2) when I'm back home. Regards, Yann E. MORIN. > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index c55c227..8c90835 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -829,16 +829,12 @@ next: > > static int conf_split_config(void) > { > - const char *name; > char path[PATH_MAX+1]; > char *s, *d, c; > struct symbol *sym; > struct stat sb; > int res, i, fd; > > - name = conf_get_autoconfig_name(); > - conf_read_simple(name, S_DEF_AUTO); > - > if (chdir("include/config")) > return 1; > > Thanks, > Michal >
Michal, All, On 2013-08-05 10:39 +0200, Michal Marek spake thusly: > On Fri, Aug 02, 2013 at 01:28:25PM -0700, Linus Torvalds wrote: > > On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > > > > > Yinghai is still working on another SR-IOV-related fix or two, which > > > will be simpler if pciehp is non-modular, so I included the Kconfig > > > changes now to get them in earlier. > > > > Hmm. Doing a trivial "make allmoconfig" for testing, I get > > > > include/config/auto.conf:3014:warning: symbol value 'm' invalid for > > HOTPLUG_PCI_PCIE > > include/config/auto.conf:4711:warning: symbol value 'm' invalid for > > HOTPLUG_PCI > > > > but that may be a build system issue with stale data from the > > *previous* "make allmodconfig". Regardless, that makes me worried. > > > > Adding Michal Marek to the discussion. I'm currently doing a new "make > > allmodconfig" after having done a "git clean -dqfx" to see if the > > error remains. If it does, I will unpull. If it is gone, I'm going to > > assume the Kconfig changes are ok, but that our build system is > > missing some dependency. > > Added Yann and the linux-kbuild list to CC. Reproducer: > > git checkout 1fe0135 > make mrproper > make allmodconfig > make silentoldconfig > git checkout aa8032b > make allmodconfig > make silentoldconfig > > conf_write_autoconf() first calls conf_split_config() to generate the > include/config/**.h hierarchy, then generates include/config/auto.conf. > For some reason, conf_split_config() reads include/config/auto.conf, > which may not exist yet or may be out of date. Yann, can anything break > if we simply do not read that file from conf_split_config(), like this? > > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c > index c55c227..8c90835 100644 > --- a/scripts/kconfig/confdata.c > +++ b/scripts/kconfig/confdata.c > @@ -829,16 +829,12 @@ next: > > static int conf_split_config(void) > { > - const char *name; > char path[PATH_MAX+1]; > char *s, *d, c; > struct symbol *sym; > struct stat sb; > int res, i, fd; > > - name = conf_get_autoconfig_name(); > - conf_read_simple(name, S_DEF_AUTO); > - > if (chdir("include/config")) > return 1; > So, first off, this is not something I am comfortable with. So I've done some (quick) research on this, and did some testing (see below). So, looking at .config after 'make allmodconfig', the HOTPLUG_PCI and HOTPLUG_PCI_PCIE symbols are indeed set to 'y' in .config, which is expected and correct. include/config/auto.conf still has them set to 'm', which is not wrong since it has not been re-generated. Finally, after make silentoldconfig, they are both set to 'y' both in .config and in include/config/auto.conf. Expected and correct. This means the warning just informs us that a symbol's type has changed to a more restricted type (eg. tristate -> bool), and is not really a problem, since both .config and auto.conf are correct afterward. Then, as Michal explained, the warning happens while we are in conf_write_autoconf(), which is called after we have dealt with all symbols, and they are expected to be properly set now. But then, we read include/config/auto.conf, and I get a bit lost there. What I understand of the code is that, we're reading auto.conf with the S_DEF_AUTO flag, which is expected to set the SYMBOL_DEF_AUTO flags to symbols read from auto.conf. This is then used by conf_split_config to detect symbols that have changed. Basically, it loops over all symbols as such: for_all_symbols(...) { if (!has_changed(symbol)) continue touch include/config/symbol.h } has_changed(symbol) checks the old value is the same (or isn't) as the new value. Is it's the same, nothing happens. Otherwise, as the value has changed, the empty symbol.h file is generated to trigger dependency tracking in the build-system. So I checked if the .h files in include/config/ were touched. The two following tests do not invlolve the HOTPLUG_PCI_.* symbols on purpose, and involve a pristine master c095ba7: $ make allmodconfig $ make silentoldconfig $ make silentoldconfig $ make silentoldconfig - Without Michal's proposed change, unaffected symbols are not touched. For example, mtime and ctime for zswap.h are not changed after the second and third silentoldconfig. - With Michal's proposed change, then mtime and ctime for zswap.h are changed after each silentoldconfig, even though CONFIG_ZSWAP was not changed. So, I'm afraid this change is incorrect, since it would mean dependency tracking would detect targets not to be up-to-date, and everything would be rebuilt. Also, the issue has pre-existed for a long time and is only detected now. I think the tristate->bool conversions in this thread just exposed the issue, and are not the root cause. One solution would be to read auto.conf *before* we fuzz around symbols, not after. We should probably read auto.conf just after we read .config. I've spent my whole evening on this issue, it's past 00:45 here, and I'm slowly drifting away now... ;-) I believe the warning to be just spurious, so it can bear one more day before I can tackle this again. In any case, I've found the kconfig code to be relatively difficult to read and follow... :-( It's mostly an agglomerate of orgnic changes accumulated over time, it is missing coments in the right places, is rather complex with a lot of special cases spread out all over, and I fell like wanting to knit after a slew of kitten has played with tens of balls of wool... :-( Anyway, with time, I'm eventually sorting things out one at a time, but Woo... the headache... ;-] Regards, Yann E. MORIN.
On 6.8.2013 00:58, Yann E. MORIN wrote: > On 2013-08-05 10:39 +0200, Michal Marek spake thusly: >> Added Yann and the linux-kbuild list to CC. Reproducer: >> >> git checkout 1fe0135 >> make mrproper >> make allmodconfig >> make silentoldconfig >> git checkout aa8032b >> make allmodconfig >> make silentoldconfig >> >> conf_write_autoconf() first calls conf_split_config() to generate the >> include/config/**.h hierarchy, then generates include/config/auto.conf. >> For some reason, conf_split_config() reads include/config/auto.conf, >> which may not exist yet or may be out of date. Yann, can anything break >> if we simply do not read that file from conf_split_config(), like this? >> >> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c >> index c55c227..8c90835 100644 >> --- a/scripts/kconfig/confdata.c >> +++ b/scripts/kconfig/confdata.c >> @@ -829,16 +829,12 @@ next: >> >> static int conf_split_config(void) >> { >> - const char *name; >> char path[PATH_MAX+1]; >> char *s, *d, c; >> struct symbol *sym; >> struct stat sb; >> int res, i, fd; >> >> - name = conf_get_autoconfig_name(); >> - conf_read_simple(name, S_DEF_AUTO); >> - >> if (chdir("include/config")) >> return 1; [...] > > What I understand of the code is that, we're reading auto.conf with > the S_DEF_AUTO flag, which is expected to set the SYMBOL_DEF_AUTO flags > to symbols read from auto.conf. > > This is then used by conf_split_config to detect symbols that have > changed. Ah, _that_ is why it's there. [...] > The two following tests do not invlolve the HOTPLUG_PCI_.* symbols > on purpose, and involve a pristine master c095ba7: > > $ make allmodconfig > $ make silentoldconfig > $ make silentoldconfig > $ make silentoldconfig > > - Without Michal's proposed change, unaffected symbols are not > touched. For example, mtime and ctime for zswap.h are not changed > after the second and third silentoldconfig. > > - With Michal's proposed change, then mtime and ctime for zswap.h are > changed after each silentoldconfig, even though CONFIG_ZSWAP was not > changed. Yes, I only did a simple diff -rq between two generated include/config directories and did not think about mtimes. > One solution would be to read auto.conf *before* we fuzz around symbols, > not after. We should probably read auto.conf just after we read .config. Maybe the safest solution would be to silence the warning if def == S_DEF_AUTO. Because auto.conf is only read by kconfig in conf_split_config(), when it is expected to be out of date. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index c55c227..8c90835 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -829,16 +829,12 @@ next: static int conf_split_config(void) { - const char *name; char path[PATH_MAX+1]; char *s, *d, c; struct symbol *sym; struct stat sb; int res, i, fd; - name = conf_get_autoconfig_name(); - conf_read_simple(name, S_DEF_AUTO); - if (chdir("include/config")) return 1;