From: Peter Oberparleiter <oberpar@linux.ibm.com> To: Johannes Berg <johannes@sipsolutions.net>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-arch@vger.kernel.org, linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Jessica Yu <jeyu@kernel.org>, Johannes Berg <johannes.berg@intel.com>, linux-um@lists.infradead.org Subject: Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Date: Wed, 20 Jan 2021 18:04:25 +0100 [thread overview] Message-ID: <ee3bc3bf-9582-d278-5b7a-d9fa27b17800@linux.ibm.com> (raw) In-Reply-To: <20210120172041.c246a2cac2fb.I1358f584b76f1898373adfed77f4462c8705b736@changeid> On 20.01.2021 17:20, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be > selected, but avoiding the in-kernel constructor calls. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Looks good+nicer than v1 to me! I also tested this patch on s390 and can confirm that it doesn't break gcov-kernel there. Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> Andrew, do we need additional reviews/sign-offs? Could this go in via your tree? > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. Just to be sure: could you confirm that this test statement for UML also applies to v2, i.e. that it fixes the original problem you were seeing? > > v2: > * special-case UML instead of splitting the CONSTRUCTORS config > --- > init/Kconfig | 1 - > init/main.c | 8 +++++++- > kernel/gcov/Kconfig | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..29ad68325028 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE > > config CONSTRUCTORS > bool > - depends on !UML > > config IRQ_WORK > bool > diff --git a/init/main.c b/init/main.c > index c68d784376ca..a626e78dbf06 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) > /* Call all constructor functions linked into the kernel. */ > static void __init do_ctors(void) > { > -#ifdef CONFIG_CONSTRUCTORS > +/* > + * For UML, the constructors have already been called by the > + * normal setup code as it's just a normal ELF binary, so we > + * cannot do it again - but we do need CONFIG_CONSTRUCTORS > + * even on UML for modules. > + */ > +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) > ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; > > for (; fn < (ctor_fn_t *) __ctors_end; fn++) > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > index 3110c77230c7..f62de2dea8a3 100644 > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" > config GCOV_KERNEL > bool "Enable gcov-based kernel profiling" > depends on DEBUG_FS > - select CONSTRUCTORS if !UML > + select CONSTRUCTORS > default n > help > This option enables gcov-based code profiling (e.g. for code coverage > -- Peter Oberparleiter Linux on Z Development - IBM Germany
WARNING: multiple messages have this Message-ID (diff)
From: Peter Oberparleiter <oberpar@linux.ibm.com> To: Johannes Berg <johannes@sipsolutions.net>, Andrew Morton <akpm@linux-foundation.org> Cc: linux-arch@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>, Johannes Berg <johannes.berg@intel.com>, linux-um@lists.infradead.org, linux-kernel@vger.kernel.org, Jessica Yu <jeyu@kernel.org> Subject: Re: [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Date: Wed, 20 Jan 2021 18:04:25 +0100 [thread overview] Message-ID: <ee3bc3bf-9582-d278-5b7a-d9fa27b17800@linux.ibm.com> (raw) In-Reply-To: <20210120172041.c246a2cac2fb.I1358f584b76f1898373adfed77f4462c8705b736@changeid> On 20.01.2021 17:20, Johannes Berg wrote: > From: Johannes Berg <johannes.berg@intel.com> > > On ARCH=um, loading a module doesn't result in its constructors > getting called, which breaks module gcov since the debugfs files > are never registered. On the other hand, in-kernel constructors > have already been called by the dynamic linker, so we can't call > them again. > > Get out of this conundrum by allowing CONFIG_CONSTRUCTORS to be > selected, but avoiding the in-kernel constructor calls. > > Also remove the "if !UML" from GCOV selecting CONSTRUCTORS now, > since we really do want CONSTRUCTORS, just not kernel binary > ones. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Looks good+nicer than v1 to me! I also tested this patch on s390 and can confirm that it doesn't break gcov-kernel there. Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com> Andrew, do we need additional reviews/sign-offs? Could this go in via your tree? > --- > Tested with a kernel configured with CONFIG_GCOV_KERNEL, without > the patch nothing ever appears in /sys/kernel/debug/gcov/ (apart > from the reset file), and with it we get the files and they work. Just to be sure: could you confirm that this test statement for UML also applies to v2, i.e. that it fixes the original problem you were seeing? > > v2: > * special-case UML instead of splitting the CONSTRUCTORS config > --- > init/Kconfig | 1 - > init/main.c | 8 +++++++- > kernel/gcov/Kconfig | 2 +- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..29ad68325028 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -76,7 +76,6 @@ config CC_HAS_ASM_INLINE > > config CONSTRUCTORS > bool > - depends on !UML > > config IRQ_WORK > bool > diff --git a/init/main.c b/init/main.c > index c68d784376ca..a626e78dbf06 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1066,7 +1066,13 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void) > /* Call all constructor functions linked into the kernel. */ > static void __init do_ctors(void) > { > -#ifdef CONFIG_CONSTRUCTORS > +/* > + * For UML, the constructors have already been called by the > + * normal setup code as it's just a normal ELF binary, so we > + * cannot do it again - but we do need CONFIG_CONSTRUCTORS > + * even on UML for modules. > + */ > +#if defined(CONFIG_CONSTRUCTORS) && !defined(CONFIG_UML) > ctor_fn_t *fn = (ctor_fn_t *) __ctors_start; > > for (; fn < (ctor_fn_t *) __ctors_end; fn++) > diff --git a/kernel/gcov/Kconfig b/kernel/gcov/Kconfig > index 3110c77230c7..f62de2dea8a3 100644 > --- a/kernel/gcov/Kconfig > +++ b/kernel/gcov/Kconfig > @@ -4,7 +4,7 @@ menu "GCOV-based kernel profiling" > config GCOV_KERNEL > bool "Enable gcov-based kernel profiling" > depends on DEBUG_FS > - select CONSTRUCTORS if !UML > + select CONSTRUCTORS > default n > help > This option enables gcov-based code profiling (e.g. for code coverage > -- Peter Oberparleiter Linux on Z Development - IBM Germany _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um
next prev parent reply other threads:[~2021-01-20 17:11 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-19 11:18 [PATCH] init/module: split CONFIG_CONSTRUCTORS to fix module gcov on UML Johannes Berg 2021-01-19 11:18 ` Johannes Berg 2021-01-20 16:07 ` Peter Oberparleiter 2021-01-20 16:07 ` Peter Oberparleiter 2021-01-20 16:09 ` Johannes Berg 2021-01-20 16:09 ` Johannes Berg 2021-01-20 16:20 ` Peter Oberparleiter 2021-01-20 16:20 ` Peter Oberparleiter 2021-01-20 16:20 ` [PATCH v2] init/gcov: allow CONFIG_CONSTRUCTORS on UML to fix module gcov Johannes Berg 2021-01-20 16:20 ` Johannes Berg 2021-01-20 17:04 ` Peter Oberparleiter [this message] 2021-01-20 17:04 ` Peter Oberparleiter 2021-01-20 17:38 ` Johannes Berg 2021-01-20 17:38 ` Johannes Berg 2021-05-08 13:50 ` Lambert 2021-05-10 11:37 ` Johannes Berg 2021-05-10 11:37 ` Johannes Berg 2021-05-10 13:31 ` lambertdev 2021-05-10 13:31 ` lambertdev 2021-05-14 13:55 ` Peter Oberparleiter 2021-05-14 13:55 ` Peter Oberparleiter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ee3bc3bf-9582-d278-5b7a-d9fa27b17800@linux.ibm.com \ --to=oberpar@linux.ibm.com \ --cc=akpm@linux-foundation.org \ --cc=arnd@arndb.de \ --cc=jeyu@kernel.org \ --cc=johannes.berg@intel.com \ --cc=johannes@sipsolutions.net \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-um@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.