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

Groups > linux.kernel > #1194678 > unrolled thread

Re: [PATCH v2] kthread: Export kthread functions

Started byAndrew Morton <akpm@linux-foundation.org>
First post2015-07-28 23:30 +0200
Last post2015-08-03 04:50 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] kthread: Export kthread functions Andrew Morton <akpm@linux-foundation.org> - 2015-07-28 23:30 +0200
    Re: [PATCH v2] kthread: Export kthread functions yalin wang <yalin.wang2010@gmail.com> - 2015-07-30 05:50 +0200
      Re: [PATCH v2] kthread: Export kthread functions yalin wang <yalin.wang2010@gmail.com> - 2015-07-31 06:20 +0200
        Re: [PATCH v2] kthread: Export kthread functions yalin wang <yalin.wang2010@gmail.com> - 2015-08-03 04:30 +0200
          Re: [PATCH v2] kthread: Export kthread functions Jes Sorensen <Jes.Sorensen@redhat.com> - 2015-08-03 04:50 +0200

#1194678 — Re: [PATCH v2] kthread: Export kthread functions

FromAndrew Morton <akpm@linux-foundation.org>
Date2015-07-28 23:30 +0200
SubjectRe: [PATCH v2] kthread: Export kthread functions
Message-ID<pRkBI-4uH-5@gated-at.bofh.it>
On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <david.kershner@unisys.com> wrote:

> The s-Par visornic driver, currently in staging, processes a queue
> being serviced by the an s-Par service partition. We can get a message
> that something has happened with the Service Partition, when that
> happens, we must not access the channel until we get a message that the
> service partition is back again.
> 
> The visornic driver has a thread for processing the channel, when we
> get the message, we need to be able to park the thread and then resume
> it when the problem clears.
> 
> We can do this with kthread_park and unpark but they are not exported
> from the kernel, this patch exports the needed functions.
> 
> Signed-off-by: David Kershner <david.kershner@unisys.com>

Please accumulate the acked-by's and reviewed-by's in the changelog as
they are received.   I presently have

Acked-by: Ingo Molnar <mingo@kernel.org>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Richard Weinberger <richard.weinberger@gmail.com>
Cc: Tejun Heo <tj@kernel.org>


I'll scoot this into mainline probably this week to make life simpler
for the various trees.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1195702

Fromyalin wang <yalin.wang2010@gmail.com>
Date2015-07-30 05:50 +0200
Message-ID<pRN10-3ho-11@gated-at.bofh.it>
In reply to#1194678
> On Jul 29, 2015, at 18:34, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Tue, 28 Jul 2015, Andrew Morton wrote:
> 
>> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <david.kershner@unisys.com> wrote:
>> 
>>> The s-Par visornic driver, currently in staging, processes a queue
>>> being serviced by the an s-Par service partition. We can get a message
>>> that something has happened with the Service Partition, when that
>>> happens, we must not access the channel until we get a message that the
>>> service partition is back again.
>>> 
>>> The visornic driver has a thread for processing the channel, when we
>>> get the message, we need to be able to park the thread and then resume
>>> it when the problem clears.
>>> 
>>> We can do this with kthread_park and unpark but they are not exported
>>> from the kernel, this patch exports the needed functions.
>>> 
>>> Signed-off-by: David Kershner <david.kershner@unisys.com>
>> 
>> Please accumulate the acked-by's and reviewed-by's in the changelog as
>> they are received.   I presently have
>> 
>> Acked-by: Ingo Molnar <mingo@kernel.org>
>> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Richard Weinberger <richard.weinberger@gmail.com>
>> Cc: Tejun Heo <tj@kernel.org>
>> 
>> 
>> I'll scoot this into mainline probably this week to make life simpler
>> for the various trees.
> 
i am curious why not make some tiny functions to be inline ?
so that don’t need EXPORT_SYMOBLS , shrink the kernel size.
Thanks--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1196574

Fromyalin wang <yalin.wang2010@gmail.com>
Date2015-07-31 06:20 +0200
Message-ID<pS9Xz-2S8-7@gated-at.bofh.it>
In reply to#1195702
> On Jul 30, 2015, at 20:02, Neil Horman <nhorman@redhat.com> wrote:
> 
> On Thu, Jul 30, 2015 at 11:48:17AM +0800, yalin wang wrote:
>> 
>>> On Jul 29, 2015, at 18:34, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> 
>>> On Tue, 28 Jul 2015, Andrew Morton wrote:
>>> 
>>>> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <david.kershner@unisys.com> wrote:
>>>> 
>>>>> The s-Par visornic driver, currently in staging, processes a queue
>>>>> being serviced by the an s-Par service partition. We can get a message
>>>>> that something has happened with the Service Partition, when that
>>>>> happens, we must not access the channel until we get a message that the
>>>>> service partition is back again.
>>>>> 
>>>>> The visornic driver has a thread for processing the channel, when we
>>>>> get the message, we need to be able to park the thread and then resume
>>>>> it when the problem clears.
>>>>> 
>>>>> We can do this with kthread_park and unpark but they are not exported
>>>>> from the kernel, this patch exports the needed functions.
>>>>> 
>>>>> Signed-off-by: David Kershner <david.kershner@unisys.com>
>>>> 
>>>> Please accumulate the acked-by's and reviewed-by's in the changelog as
>>>> they are received.   I presently have
>>>> 
>>>> Acked-by: Ingo Molnar <mingo@kernel.org>
>>>> Acked-by: Neil Horman <nhorman@tuxdriver.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Richard Weinberger <richard.weinberger@gmail.com>
>>>> Cc: Tejun Heo <tj@kernel.org>
>>>> 
>>>> 
>>>> I'll scoot this into mainline probably this week to make life simpler
>>>> for the various trees.
>>> 
>> i am curious why not make some tiny functions to be inline ?
>> so that don’t need EXPORT_SYMOBLS , shrink the kernel size.
>> Thanks
> 
> Because exporting symbols isn't a big deal, and the compiler can decide when its
> best to inline these functions.  As it is, they aren't that small, if you expand
> all their internals
> 
> Neil
> 

this is my test on arm64 arch,
 i think kthread_should_stop() kthread_should_park() kthread_data() should be inline :


without inline:
ffffffc0000d265c <smpboot_thread_fn>:
                                 ……….
ffffffc0000d26a0:       b9001a61        str     w1, [x19,#24]
ffffffc0000d26a4:       97fff260        bl      ffffffc0000cf024 <kthread_should_stop>
ffffffc0000d26a8:       53001c00        uxtb    w0, w0
ffffffc0000d26ac:       35000c20        cbnz    w0, ffffffc0000d2830 <smpboot_thread_fn+0x1d4>
ffffffc0000d26b0:       97fff4aa        bl      ffffffc0000cf958 <kthread_should_park>
ffffffc0000d26b4:       53001c00        uxtb    w0, w0
ffffffc0000d26b8:       34000320        cbz     w0, ffffffc0000d271c <smpboot_thread_fn+0xc0>
ffffffc0000d26bc:       f9400a60        ldr     x0, [x19,#16]
ffffffc0000d26c0:       f900001f        str     xzr, [x0]


ffffffc0000cf958 <kthread_should_park>:
ffffffc0000cf958:       910003e0        mov     x0, sp
ffffffc0000cf95c:       9272c400        and     x0, x0, #0xffffffffffffc000
ffffffc0000cf960:       f9400800        ldr     x0, [x0,#16]
ffffffc0000cf964:       f9420400        ldr     x0, [x0,#1032]
ffffffc0000cf968:       f85c8000        ldr     x0, [x0,#-56]
ffffffc0000cf96c:       d3420800        ubfx    x0, x0, #2, #1
ffffffc0000cf970:       d65f03c0        ret






if i mark kthread_should_park to be inline :

ffffffc0000d19ec <smpboot_thread_fn>:
ffffffc0000d19ec:       a9bc7bfd        stp     x29, x30, [sp,#-64]!
ffffffc0000d19f0:       910003fd        mov     x29, sp
ffffffc0000d19f4:       a90153f3        stp     x19, x20, [sp,#16]
ffffffc0000d19f8:       aa0003f4        mov     x20, x0
ffffffc0000d19fc:       910003e0        mov     x0, sp
ffffffc0000d1a00:       a90363f7        stp     x23, x24, [sp,#48]
ffffffc0000d1a04:       a9025bf5        stp     x21, x22, [sp,#32]
ffffffc0000d1a08:       d2800035        mov     x21, #0x1                       // #1
ffffffc0000d1a0c:       9272c413        and     x19, x0, #0xffffffffffffc000
ffffffc0000d1a10:       f9400696        ldr     x22, [x20,#8]
ffffffc0000d1a14:       2a1503f7        mov     w23, w21
ffffffc0000d1a18:       52800058        mov     w24, #0x2                       // #2
ffffffc0000d1a1c:       f9400a60        ldr     x0, [x19,#16]
ffffffc0000d1a20:       f9000015        str     x21, [x0]
ffffffc0000d1a24:       d5033bbf        dmb     ish
ffffffc0000d1a28:       b9401a61        ldr     w1, [x19,#24]
ffffffc0000d1a2c:       11000421        add     w1, w1, #0x1
ffffffc0000d1a30:       b9001a61        str     w1, [x19,#24]
ffffffc0000d1a34:       f9400a62        ldr     x2, [x19,#16]
ffffffc0000d1a38:       f9420441        ldr     x1, [x2,#1032]
ffffffc0000d1a3c:       f85c8020        ldr     x0, [x1,#-56]
ffffffc0000d1a40:       37080a60        tbnz    w0, #1, ffffffc0000d1b8c <smpboot_thread_fn+0x1a0>
ffffffc0000d1a44:       f85c8020        ldr     x0, [x1,#-56]    									// kthread_should_park  line
ffffffc0000d1a48:       36100300        tbz     w0, #2, ffffffc0000d1aa8 <smpboot_thread_fn+0xbc>   // kthread_should_park  line
ffffffc0000d1a4c:       f900005f        str     xzr, [x2]
ffffffc0000d1a50:       b9401a60        ldr     w0, [x19,#24]

it is optimised to 2 instructions ,

this is my patch, hope can be merged :

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 3e6773e..9210030 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -4,6 +4,70 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 
+struct kthread {
+       unsigned long flags;
+       unsigned int cpu;
+       void *data;
+       struct completion parked;
+       struct completion exited;
+};
+
+enum KTHREAD_BITS {
+       KTHREAD_IS_PER_CPU = 0,
+       KTHREAD_SHOULD_STOP,
+       KTHREAD_SHOULD_PARK,
+       KTHREAD_IS_PARKED,
+};
+
+#define __to_kthread(vfork)    \
+       container_of(vfork, struct kthread, exited)
+
+static inline struct kthread *to_kthread(struct task_struct *k)
+{
+       return __to_kthread(k->vfork_done);
+}
+
+/**
+ * kthread_should_stop - should this kthread return now?
+ *
+ * When someone calls kthread_stop() on your kthread, it will be woken
+ * and this will return true.  You should then return, and your return
+ * value will be passed through to kthread_stop().
+ */
+static inline bool kthread_should_stop(void)
+{
+       return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags);
+}
+
+/**
+ * kthread_should_park - should this kthread park now?
+ *
+ * When someone calls kthread_park() on your kthread, it will be woken
+ * and this will return true.  You should then do the necessary
+ * cleanup and call kthread_parkme()
+ *
+ * Similar to kthread_should_stop(), but this keeps the thread alive
+ * and in a park position. kthread_unpark() "restarts" the thread and
+ * calls the thread function again.
+ */
+static inline bool kthread_should_park(void)
+{
+       return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
+}
+
+/**
+ * kthread_data - return data value specified on kthread creation
+ * @task: kthread task in question
+ *
+ * Return the data value specified when kthread @task was created.
+ * The caller is responsible for ensuring the validity of @task when
+ * calling this function.
+ */
+static inline void *kthread_data(struct task_struct *task)
+{
+       return to_kthread(task)->data;
+}
+
 __printf(4, 5)
 struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
                                           void *data,
@@ -39,10 +103,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 int kthread_stop(struct task_struct *k);
-bool kthread_should_stop(void);
-bool kthread_should_park(void);
 bool kthread_freezable_should_stop(bool *was_frozen);
-void *kthread_data(struct task_struct *k);
 void *probe_kthread_data(struct task_struct *k);
 int kthread_park(struct task_struct *k);
 void kthread_unpark(struct task_struct *k);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index baf3673..e036019 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -38,29 +38,6 @@ struct kthread_create_info
        struct list_head list;
 };
 
-struct kthread {
-       unsigned long flags;
-       unsigned int cpu;
-       void *data;
-       struct completion parked;
-       struct completion exited;
-};
-
-enum KTHREAD_BITS {
-       KTHREAD_IS_PER_CPU = 0,
-       KTHREAD_SHOULD_STOP,
-       KTHREAD_SHOULD_PARK,
-       KTHREAD_IS_PARKED,
-};
-
-#define __to_kthread(vfork)    \
-       container_of(vfork, struct kthread, exited)
-
-static inline struct kthread *to_kthread(struct task_struct *k)
-{
-       return __to_kthread(k->vfork_done);
-}
-
 static struct kthread *to_live_kthread(struct task_struct *k)
 {
        struct completion *vfork = ACCESS_ONCE(k->vfork_done);
@@ -70,36 +47,6 @@ static struct kthread *to_live_kthread(struct task_struct *k)
 }
 
 /**
...skipping...
- * When someone calls kthread_stop() on your kthread, it will be woken
- * and this will return true.  You should then return, and your return
- * value will be passed through to kthread_stop().
- */
-bool kthread_should_stop(void)
-{
-       return test_bit(KTHREAD_SHOULD_STOP, &to_kthread(current)->flags);
-}
-EXPORT_SYMBOL(kthread_should_stop);
-
-/**
- * kthread_should_park - should this kthread park now?
- *
- * When someone calls kthread_park() on your kthread, it will be woken
- * and this will return true.  You should then do the necessary
- * cleanup and call kthread_parkme()
- *
- * Similar to kthread_should_stop(), but this keeps the thread alive
- * and in a park position. kthread_unpark() "restarts" the thread and
- * calls the thread function again.
- */
-bool kthread_should_park(void)
-{
-       return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
-}
-EXPORT_SYMBOL_GPL(kthread_should_park);
-
-/**
  * kthread_freezable_should_stop - should this freezable kthread return now?
  * @was_frozen: optional out parameter, indicates whether %current was frozen
  *
@@ -125,19 +72,6 @@ bool kthread_freezable_should_stop(bool *was_frozen)
 EXPORT_SYMBOL_GPL(kthread_freezable_should_stop);
 
 /**
- * kthread_data - return data value specified on kthread creation
- * @task: kthread task in question
- *
- * Return the data value specified when kthread @task was created.
- * The caller is responsible for ensuring the validity of @task when
- * calling this function.
- */
-void *kthread_data(struct task_struct *task)
-{
-       return to_kthread(task)->data;
-}
-
-/**
  * probe_kthread_data - speculative version of kthread_data()
  * @task: possible kthread task in question
  *
---
(END)
Thanks










--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1198495

Fromyalin wang <yalin.wang2010@gmail.com>
Date2015-08-03 04:30 +0200
Message-ID<pTdFL-60J-1@gated-at.bofh.it>
In reply to#1196574
> On Aug 1, 2015, at 21:32, Neil Horman <nhorman@redhat.com> wrote:
> 
> On Sat, Aug 01, 2015 at 03:12:42PM +0800, yalin wang wrote:
>> 
>>> 在 2015年7月31日，22:14，Thomas Gleixner <tglx@linutronix.de> 写道：
>>> 
>>> 
>>> On Fri, 31 Jul 2015, Thomas Gleixner wrote:
>>> 
>>>> On Fri, 31 Jul 2015, yalin wang wrote:
>>>>> it is optimised to 2 instructions ,
>>>>> 
>>>>> this is my patch, hope can be merged :
>>>> 
>>>> We are not exposing the internals of kthread management. Period.
>>> 
>>> And your 'optimization' is completely bogus:
>>> 
>>> Before your modification:
>>> 
>>> size kernel/built-in.o
>>> 
>>>  text	   data	    bss	    dec	    hex	filename
>>> 1091514	 141498  341928 1574940  18081c	../build/kernel/built-in.o
>>> 
>>> After:
>>> 
>>>  text	   data	    bss	    dec	    hex	filename
>>> 1091664  141498  341928 1575090  1808b2	../build/kernel/built-in.o
>>> 
>>> That's an increase of text size by 150 byte. Interesting optimization.
>>> 
>>> Thanks,
>>> 
>>> 	tglx
>>> 
>>> 
>> strange,  this is my test result:
>> 
>> size   built-in.o*
>>  text	   data	    bss	    dec	    hex	filename
>> 743937	  50786	  56008	 850731	  cfb2b	built-in.o        // with the patch
>> 744069	  50786	  56008	 850863	  cfbaf	built-in.o_old  // with out the patch
>> 
> So you're willing to expose the internals of kthread_park in exchange for the
> hope of saving 132 bytes of text.
> 
> Thats just dumb.  I agree with tglx, this shouldn't change.
> 
> Neil
not just size, mainly for performance,
without inline:

ffffffc0000d26b0:       97fff4aa        bl      ffffffc0000cf958 <kthread_should_park>
ffffffc0000d26b4:       53001c00        uxtb    w0, w0

if kthread_should_park() inline:
ffffffc0000d1a44:       f85c8020        ldr     x0, [x1,#-56]    									// kthread_should_park line
ffffffc0000d1a48:       36100300        tbz     w0, #2, ffffffc0000d1aa8 <smpboot_thread_fn+0xbc>   // kthread_should_park  line

still use 2 instructions, but don’t need a function call,
maybe can do more optimisation by gcc sometimes .
Anyway, this is just a suggest,
it is up to you apply it or not. :) 

Thanks











--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1198503

FromJes Sorensen <Jes.Sorensen@redhat.com>
Date2015-08-03 04:50 +0200
Message-ID<pTdZ7-6n8-3@gated-at.bofh.it>
In reply to#1198495
yalin wang <yalin.wang2010@gmail.com> writes:
>> On Aug 1, 2015, at 21:32, Neil Horman <nhorman@redhat.com> wrote:
>>> strange,  this is my test result:
>>> 
>>> size   built-in.o*
>>>  text	   data	    bss	    dec	    hex	filename
>>> 743937 50786 56008 850731 cfb2b built-in.o // with the patch
>>> 744069 50786 56008 850863 cfbaf built-in.o_old // with out the
>>> patch
>>> 
>> So you're willing to expose the internals of kthread_park in exchange for the
>> hope of saving 132 bytes of text.
>> 
>> Thats just dumb.  I agree with tglx, this shouldn't change.
>> 
>> Neil
> not just size, mainly for performance,
> without inline:
>
> ffffffc0000d26b0: 97fff4aa bl ffffffc0000cf958 <kthread_should_park>
> ffffffc0000d26b4:       53001c00        uxtb    w0, w0
>
> if kthread_should_park() inline:
> ffffffc0000d1a44: f85c8020 ldr x0, [x1,#-56] // kthread_should_park
> line
> ffffffc0000d1a48: 36100300 tbz w0, #2, ffffffc0000d1aa8
> <smpboot_thread_fn+0xbc> // kthread_should_park line
>
> still use 2 instructions, but don’t need a function call,
> maybe can do more optimisation by gcc sometimes .
> Anyway, this is just a suggest,
> it is up to you apply it or not. :) 

kthread_park() isn't exactly a performance critical function call.
Saving two instructions does not outway the cost of exposing the
internals of the kthread API.

Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web