Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1636278 > unrolled thread

[PATCH/RFC] signal: Export signal_wake_up_state() to modules

Back to article view | Back to linux.kernel

Contents

  [PATCH/RFC] signal: Export signal_wake_up_state() to modules Geert Uytterhoeven <geert+renesas@glider.be> - 2017-05-05 11:50 +0200
    Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Christoph Hellwig <hch@infradead.org> - 2017-05-05 12:10 +0200
      Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-05 12:20 +0200
        Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-05 13:10 +0200
          Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Christoph Hellwig <hch@infradead.org> - 2017-05-05 13:30 +0200
            Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-05 13:40 +0200
    Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Oleg Nesterov <oleg@redhat.com> - 2017-05-06 21:50 +0200
      Re: [PATCH/RFC] signal: Export signal_wake_up_state() to modules Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-08 01:30 +0200

#1636278 — [PATCH/RFC] signal: Export signal_wake_up_state() to modules

When using signal_wake_up() from a loadable kernel module:

    ERROR: "signal_wake_up_state" [drivers/spi/spi-sh-msiof.ko] undefined!

Export signal_wake_up_state() to modules to fix this.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
I'm using signal_wake_up() to abort a task blocked on
wait_for_completion_interruptible(), cfr. sh_msiof_slave_abort() in
"spi: sh-msiof: Add slave mode support"
(http://www.spinics.net/lists/devicetree/msg175575.html).

Is exporting signal_wake_up_state() an acceptable solution?
Alternatively, I can extract the code to abort an completion into a
generic abort_completion() function, and export that.

Or is there a better way to abort a completion?

Thanks!
---
 kernel/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7e59ebc2c25e669e..f130598acd34f08b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -660,6 +660,7 @@ void signal_wake_up_state(struct task_struct *t, unsigned int state)
 	if (!wake_up_state(t, state | TASK_INTERRUPTIBLE))
 		kick_process(t);
 }
+EXPORT_SYMBOL_GPL(signal_wake_up_state);
 
 /*
  * Remove signals in mask from the pending set and queue.
-- 
2.7.4

[toc] | [next] | [standalone]

#1636285

On Fri, May 05, 2017 at 11:49:11AM +0200, Geert Uytterhoeven wrote:
> When using signal_wake_up() from a loadable kernel module:
> 
>     ERROR: "signal_wake_up_state" [drivers/spi/spi-sh-msiof.ko] undefined!
> 
> Export signal_wake_up_state() to modules to fix this.

Or rever the commit that added the users.  This doesn't look like
functionality a driver should use..

[toc] | [prev] | [next] | [standalone]

#1636289

Hi Christoph,

On Fri, May 5, 2017 at 12:07 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 05, 2017 at 11:49:11AM +0200, Geert Uytterhoeven wrote:
>> When using signal_wake_up() from a loadable kernel module:
>>
>>     ERROR: "signal_wake_up_state" [drivers/spi/spi-sh-msiof.ko] undefined!
>>
>> Export signal_wake_up_state() to modules to fix this.
>
> Or rever the commit that added the users.  This doesn't look like

That commit is not yet applied.

> functionality a driver should use..

OK.

Then, do you have any advise on how to properly abort a completion?

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1636313

On Fri, May 5, 2017 at 12:13 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, May 5, 2017 at 12:07 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Fri, May 05, 2017 at 11:49:11AM +0200, Geert Uytterhoeven wrote:
>>> When using signal_wake_up() from a loadable kernel module:
>>>
>>>     ERROR: "signal_wake_up_state" [drivers/spi/spi-sh-msiof.ko] undefined!
>>>
>>> Export signal_wake_up_state() to modules to fix this.
>>
>> Or rever the commit that added the users.  This doesn't look like
>
> That commit is not yet applied.
>
>> functionality a driver should use..
>
> OK.
>
> Then, do you have any advise on how to properly abort a completion?

Of course I can add a flag to indicate abortion, and just call complete_all(),
but IMHO that's a bit silly, given wait_for_completion_interruptible() already
provides this side channel information through -ERESTARTSYS.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1636320

On Fri, May 05, 2017 at 01:08:47PM +0200, Geert Uytterhoeven wrote:
> Of course I can add a flag to indicate abortion, and just call complete_all(),
> but IMHO that's a bit silly, given wait_for_completion_interruptible() already
> provides this side channel information through -ERESTARTSYS.

Even if this was the right channel (which I'm not sure about) it
should be provided as an abstract API operating on the completion.

[toc] | [prev] | [next] | [standalone]

#1636324

Hi Christoph,

On Fri, May 5, 2017 at 1:22 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, May 05, 2017 at 01:08:47PM +0200, Geert Uytterhoeven wrote:
>> Of course I can add a flag to indicate abortion, and just call complete_all(),
>> but IMHO that's a bit silly, given wait_for_completion_interruptible() already
>> provides this side channel information through -ERESTARTSYS.
>
> Even if this was the right channel (which I'm not sure about) it
> should be provided as an abstract API operating on the completion.

Hence my other question in the patch comments:

| Is exporting signal_wake_up_state() an acceptable solution?
| Alternatively, I can extract the code to abort an completion into a
| generic abort_completion() function, and export that.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1636973

On 05/05, Geert Uytterhoeven wrote:
>
> I'm using signal_wake_up() to abort a task blocked on
> wait_for_completion_interruptible(), cfr. sh_msiof_slave_abort() in
> "spi: sh-msiof: Add slave mode support"
> (http://www.spinics.net/lists/devicetree/msg175575.html).
>
> Is exporting signal_wake_up_state() an acceptable solution?
> Alternatively, I can extract the code to abort an completion into a
> generic abort_completion() function, and export that.

I too do not think this is a good idea... signal_wake_up() or even
set_tsk_thread_flag() should never be used unless you actually send a
signal. Yes, freeze_task() does this too but note that recalc_sigpending()
checks freezing() and in this case we really want the target to enter the
get_signal() path.

And in fact I do not really understand why do you need it, it seems that
you can easily rework this code and avoid this hack.

Not to mention that sh_msiof_slave_abort() plays with struct completion
internals, this doesn't look good too.

Finally, clear_thread_flag(TIF_SIGPENDING) in sh_msiof_wait_for_completion()
looks wrong. Or it is only for kthreads?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1637147

Hi Oleg,

On Sat, May 6, 2017 at 9:42 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/05, Geert Uytterhoeven wrote:
>> I'm using signal_wake_up() to abort a task blocked on
>> wait_for_completion_interruptible(), cfr. sh_msiof_slave_abort() in
>> "spi: sh-msiof: Add slave mode support"
>> (http://www.spinics.net/lists/devicetree/msg175575.html).
>>
>> Is exporting signal_wake_up_state() an acceptable solution?
>> Alternatively, I can extract the code to abort an completion into a
>> generic abort_completion() function, and export that.
>
> I too do not think this is a good idea... signal_wake_up() or even
> set_tsk_thread_flag() should never be used unless you actually send a
> signal. Yes, freeze_task() does this too but note that recalc_sigpending()
> checks freezing() and in this case we really want the target to enter the
> get_signal() path.
>
> And in fact I do not really understand why do you need it, it seems that
> you can easily rework this code and avoid this hack.

You mean having my own aborted flag, and calling complete_all()?

> Not to mention that sh_msiof_slave_abort() plays with struct completion
> internals, this doesn't look good too.

One reason to turn it into a generic abort_completion() function...

> Finally, clear_thread_flag(TIF_SIGPENDING) in sh_msiof_wait_for_completion()
> looks wrong. Or it is only for kthreads?

If I don't clear that flag, the next (by the same thread[*])
wait_for_completion_interruptible() will abort immediately.

[*] Typically this is the spi message pump worker.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web