next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Naresh Kamboju]

The 32-bit arm, mips and powerpc the tinyconfig builds failed on today's
Linux next-20240712 tag with gcc and clang builds.
The defconfig builds pass.

  GOOD: next-20240711
  BAD:  next-20240712

Build error:
------
arm-linux-gnueabihf-ld: kernel/task_work.o: in function `task_work_add':
task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

Build link:
-----
  [1] https://storage.tuxsuite.com/public/linaro/lkft/builds/2j8Y3PffxvTIFJIR8HODmHEU9oM/
  [2] https://storage.tuxsuite.com/public/linaro/lkft/builds/2j8Y3PffxvTIFJIR8HODmHEU9oM/config

Build details:
--------
  git_describe: next-20240712
  git_repo: https://gitlab.com/Linaro/lkft/mirrors/next/linux-next
  git_sha: 3fe121b622825ff8cc995a1e6b026181c48188db
  git_short_log: 3fe121b62282 ("Add linux-next specific files for 20240712")
  arch: arm, mips, powerpc, s390
  config: tinyconfig
  toolchain: gcc-13, gcc-12 and clang

--
Linaro LKFT
https://lkft.linaro.org

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Arnd Bergmann]

On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
> The 32-bit arm, mips and powerpc the tinyconfig builds failed on today's
> Linux next-20240712 tag with gcc and clang builds.
> The defconfig builds pass.
>
>   GOOD: next-20240711
>   BAD:  next-20240712
>
> Build error:
> ------
> arm-linux-gnueabihf-ld: kernel/task_work.o: in function `task_work_add':
> task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>

The call to this function was added in 466e4d801cd4 ("task_work:
Add TWA_NMI_CURRENT as an additional notify mode.").  It's possible
that we may have to always enable IRQ_WORK even on non-SMP
kernels now. In practice it is already enabled in most
configurations for one reason or another, the the cost is
likely very small.

Otherwise checking for CONFIG_HAVE_NMI in the new code might work.

     Arnd

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Peter Zijlstra]

On Fri, Jul 12, 2024 at 02:28:38PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
> > The 32-bit arm, mips and powerpc the tinyconfig builds failed on today's
> > Linux next-20240712 tag with gcc and clang builds.
> > The defconfig builds pass.
> >
> >   GOOD: next-20240711
> >   BAD:  next-20240712
> >
> > Build error:
> > ------
> > arm-linux-gnueabihf-ld: kernel/task_work.o: in function `task_work_add':
> > task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'
> >
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> 
> The call to this function was added in 466e4d801cd4 ("task_work:
> Add TWA_NMI_CURRENT as an additional notify mode.").  It's possible

Thanks, that initial report was mostly useless without that sha. I do
wonder why 0day build robot didn't complain to me about this. It seems
something it should definitely find.

> that we may have to always enable IRQ_WORK even on non-SMP
> kernels now. In practice it is already enabled in most
> configurations for one reason or another, the the cost is
> likely very small.
> 
> Otherwise checking for CONFIG_HAVE_NMI in the new code might work.

ARM seems to have HAVE_NMI while also being one of the architectures
that is now failing.

I'm a bit confused though, perf is already depending on irq_work (and
perf is the user of TWA_NMI_CURRENT). So I'm not exactly sure what
config fail is leading to all this.

I suppose we can do something like the below.. it'll end up in a runtime
fail for whoever manages to use TWA_NMI_CURRENT without also having
irq_work enabled, but that should currently be nobody.

---
 kernel/task_work.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/task_work.c b/kernel/task_work.c
index 5c2daa7ad3f9..276e245b7e7e 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -81,9 +81,11 @@ int task_work_add(struct task_struct *task, struct callback_head *work,
 	case TWA_SIGNAL_NO_IPI:
 		__set_notify_signal(task);
 		break;
+#ifdef CONFIG_IRQ_WORK
 	case TWA_NMI_CURRENT:
 		irq_work_queue(this_cpu_ptr(&irq_work_NMI_resume));
 		break;
+#endif
 	default:
 		WARN_ON_ONCE(1);
 		break;

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Arnd Bergmann]

On Fri, Jul 12, 2024, at 15:24, Peter Zijlstra wrote:
> On Fri, Jul 12, 2024 at 02:28:38PM +0200, Arnd Bergmann wrote:
>> On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
>
>> that we may have to always enable IRQ_WORK even on non-SMP
>> kernels now. In practice it is already enabled in most
>> configurations for one reason or another, the the cost is
>> likely very small.
>> 
>> Otherwise checking for CONFIG_HAVE_NMI in the new code might work.
>
> ARM seems to have HAVE_NMI while also being one of the architectures
> that is now failing.

Right, in this case we would also need

--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -236,6 +236,7 @@ config HAVE_FUNCTION_ERROR_INJECTION
 
 config HAVE_NMI
        bool
+       select IRQ_WORK
 
 config HAVE_FUNCTION_DESCRIPTORS
        bool

> I'm a bit confused though, perf is already depending on irq_work (and
> perf is the user of TWA_NMI_CURRENT). So I'm not exactly sure what
> config fail is leading to all this.

Ok, this is the bit I was missing. If it's only needed for
perf, then the problem is that the caller is built in unconditionally
even when perf is disabled, otherwise it would be caught by
the existing:

config PERF_EVENTS
        bool "Kernel performance events and counters"
        select IRQ_WORK

> I suppose we can do something like the below.. it'll end up in a runtime
> fail for whoever manages to use TWA_NMI_CURRENT without also having
> irq_work enabled, but that should currently be nobody.

>  		break;
> +#ifdef CONFIG_IRQ_WORK
>  	case TWA_NMI_CURRENT:
>  		irq_work_queue(this_cpu_ptr(&irq_work_NMI_resume));
>  		break;
> +#endif
>  	default:

This looks more fragile than necessary. as it might fail if anything
else starts using TWA_NMI_CURRENT without selecting IRQ_WORK.
I would prefer either something that makes it still run into
a compile-time error (e.g. hiding the TWA_NMI_CURRENT inside
the same #ifdef), or just making IRQ_WORK unconditional
as I first suggested.

Configurations without IRQ_WORK are extremely rare, since
it also gets selected by CONFIG_PRINTK, which can only be
turned off for CONFIG_EXPERT=y and is almost always a good
idea even for small kernels.

      Arnd

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Peter Zijlstra]

On Fri, Jul 12, 2024 at 04:11:54PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 12, 2024, at 15:24, Peter Zijlstra wrote:
> > On Fri, Jul 12, 2024 at 02:28:38PM +0200, Arnd Bergmann wrote:
> >> On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
> >
> >> that we may have to always enable IRQ_WORK even on non-SMP
> >> kernels now. In practice it is already enabled in most
> >> configurations for one reason or another, the the cost is
> >> likely very small.
> >> 
> >> Otherwise checking for CONFIG_HAVE_NMI in the new code might work.
> >
> > ARM seems to have HAVE_NMI while also being one of the architectures
> > that is now failing.
> 
> Right, in this case we would also need
> 
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -236,6 +236,7 @@ config HAVE_FUNCTION_ERROR_INJECTION
>  
>  config HAVE_NMI
>         bool
> +       select IRQ_WORK
>  
>  config HAVE_FUNCTION_DESCRIPTORS
>         bool

Yeah, that works for me I suppose.

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Nathan Chancellor]

On Fri, Jul 12, 2024 at 05:03:33PM +0200, Peter Zijlstra wrote:
> On Fri, Jul 12, 2024 at 04:11:54PM +0200, Arnd Bergmann wrote:
> > On Fri, Jul 12, 2024, at 15:24, Peter Zijlstra wrote:
> > > On Fri, Jul 12, 2024 at 02:28:38PM +0200, Arnd Bergmann wrote:
> > >> On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
> > >
> > >> that we may have to always enable IRQ_WORK even on non-SMP
> > >> kernels now. In practice it is already enabled in most
> > >> configurations for one reason or another, the the cost is
> > >> likely very small.
> > >> 
> > >> Otherwise checking for CONFIG_HAVE_NMI in the new code might work.
> > >
> > > ARM seems to have HAVE_NMI while also being one of the architectures
> > > that is now failing.
> > 
> > Right, in this case we would also need
> > 
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -236,6 +236,7 @@ config HAVE_FUNCTION_ERROR_INJECTION
> >  
> >  config HAVE_NMI
> >         bool
> > +       select IRQ_WORK
> >  
> >  config HAVE_FUNCTION_DESCRIPTORS
> >         bool
> 
> Yeah, that works for me I suppose.

Was there any conclusion to this thread that I missed? The
configurations Naresh mentioned in the original post are now broken in
mainline because the pull request was sent without any solution to this
issue.

FWIW, that diff doesn't fix ARCH=powerpc tinyconfig, as it does not have
CONFIG_HAVE_NMI. kernel/task_work.c is unconditionally built so
shouldn't IRQ_WORK now be 'default y' (or just eliminated altogether)?

Cheers,
Nathan

Re: next-20240712: task_work.c:(.text+0xc2): undefined reference to `irq_work_queue'

[Guenter Roeck]

On Thu, Jul 18, 2024 at 09:25:27AM -0700, Nathan Chancellor wrote:
> On Fri, Jul 12, 2024 at 05:03:33PM +0200, Peter Zijlstra wrote:
> > On Fri, Jul 12, 2024 at 04:11:54PM +0200, Arnd Bergmann wrote:
> > > On Fri, Jul 12, 2024, at 15:24, Peter Zijlstra wrote:
> > > > On Fri, Jul 12, 2024 at 02:28:38PM +0200, Arnd Bergmann wrote:
> > > >> On Fri, Jul 12, 2024, at 14:13, Naresh Kamboju wrote:
> > > >
> > > >> that we may have to always enable IRQ_WORK even on non-SMP
> > > >> kernels now. In practice it is already enabled in most
> > > >> configurations for one reason or another, the the cost is
> > > >> likely very small.
> > > >> 
> > > >> Otherwise checking for CONFIG_HAVE_NMI in the new code might work.
> > > >
> > > > ARM seems to have HAVE_NMI while also being one of the architectures
> > > > that is now failing.
> > > 
> > > Right, in this case we would also need
> > > 
> > > --- a/arch/Kconfig
> > > +++ b/arch/Kconfig
> > > @@ -236,6 +236,7 @@ config HAVE_FUNCTION_ERROR_INJECTION
> > >  
> > >  config HAVE_NMI
> > >         bool
> > > +       select IRQ_WORK
> > >  
> > >  config HAVE_FUNCTION_DESCRIPTORS
> > >         bool
> > 
> > Yeah, that works for me I suppose.
> 
> Was there any conclusion to this thread that I missed? The
> configurations Naresh mentioned in the original post are now broken in
> mainline because the pull request was sent without any solution to this
> issue.
> 
> FWIW, that diff doesn't fix ARCH=powerpc tinyconfig, as it does not have
> CONFIG_HAVE_NMI. kernel/task_work.c is unconditionally built so
> shouldn't IRQ_WORK now be 'default y' (or just eliminated altogether)?
> 

Actually, many tinyconfig builds are now broken.

Example from alpha:

alpha-linux-ld: kernel/task_work.o: in function `task_work_add':
(.text+0x1b4): undefined reference to `irq_work_queue'
alpha-linux-ld: (.text+0x1bc): undefined reference to `irq_work_queue'

alpha does not set "HAVE_NMI". AFAICS task_work.c is always built,
and it now calls irq_work_queue() unconditionally, so unless that
is changed IRQ_WORK is now mandatory and must not just depend on
HAVE_NMI or anything else.

Guenter