* [PATCH] kthread: Export kthread functions
@ 2015-07-24 22:45 David Kershner
2015-07-24 23:14 ` Neil Horman
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: David Kershner @ 2015-07-24 22:45 UTC (permalink / raw)
To: akpm
Cc: tj, laijs, nacc, nhorman, tglx, mingo, linux-kernel,
jes.sorensen, sparmaintainer, David Kershner
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>
---
kernel/kthread.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c..bad80c1 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -97,6 +97,7 @@ bool kthread_should_park(void)
{
return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
}
+EXPORT_SYMBOL(kthread_should_park);
/**
* kthread_freezable_should_stop - should this freezable kthread return now?
@@ -171,6 +172,7 @@ void kthread_parkme(void)
{
__kthread_parkme(to_kthread(current));
}
+EXPORT_SYMBOL(kthread_parkme);
static int kthread(void *_create)
{
@@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
if (kthread)
__kthread_unpark(k, kthread);
}
+EXPORT_SYMBOL(kthread_unpark);
/**
* kthread_park - park a thread created by kthread_create().
@@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
}
return ret;
}
+EXPORT_SYMBOL(kthread_park);
/**
* kthread_stop - stop a thread created by kthread_create().
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] kthread: Export kthread functions
2015-07-24 22:45 [PATCH] kthread: Export kthread functions David Kershner
@ 2015-07-24 23:14 ` Neil Horman
2015-07-25 12:05 ` Richard Weinberger
` (2 subsequent siblings)
3 siblings, 0 replies; 21+ messages in thread
From: Neil Horman @ 2015-07-24 23:14 UTC (permalink / raw)
To: David Kershner
Cc: akpm, tj, laijs, nacc, tglx, mingo, linux-kernel, jes.sorensen,
sparmaintainer
On Fri, Jul 24, 2015 at 06:45:20PM -0400, David Kershner 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>
Given that these functions are visible in the header and part of the
kthread_api, I think it makes good sense to do this
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> ---
> kernel/kthread.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 10e489c..bad80c1 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -97,6 +97,7 @@ bool kthread_should_park(void)
> {
> return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
> }
> +EXPORT_SYMBOL(kthread_should_park);
>
> /**
> * kthread_freezable_should_stop - should this freezable kthread return now?
> @@ -171,6 +172,7 @@ void kthread_parkme(void)
> {
> __kthread_parkme(to_kthread(current));
> }
> +EXPORT_SYMBOL(kthread_parkme);
>
> static int kthread(void *_create)
> {
> @@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
> if (kthread)
> __kthread_unpark(k, kthread);
> }
> +EXPORT_SYMBOL(kthread_unpark);
>
> /**
> * kthread_park - park a thread created by kthread_create().
> @@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
> }
> return ret;
> }
> +EXPORT_SYMBOL(kthread_park);
>
> /**
> * kthread_stop - stop a thread created by kthread_create().
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kthread: Export kthread functions
2015-07-24 22:45 [PATCH] kthread: Export kthread functions David Kershner
2015-07-24 23:14 ` Neil Horman
@ 2015-07-25 12:05 ` Richard Weinberger
2015-07-26 0:50 ` Kershner, David A
2015-07-26 8:46 ` Thomas Gleixner
2015-07-27 15:45 ` Ingo Molnar
2015-07-28 15:59 ` [PATCH v2] " David Kershner
3 siblings, 2 replies; 21+ messages in thread
From: Richard Weinberger @ 2015-07-25 12:05 UTC (permalink / raw)
To: David Kershner
Cc: Andrew Morton, Tejun Heo, laijs, nacc, nhorman, Thomas Gleixner,
Ingo Molnar, LKML, jes.sorensen, sparmaintainer
On Sat, Jul 25, 2015 at 12:45 AM, 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.
Are you sure that you need these function?
You would be the first user.
Please see: https://lkml.org/lkml/2015/7/8/1150
--
Thanks,
//richard
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] kthread: Export kthread functions
2015-07-25 12:05 ` Richard Weinberger
@ 2015-07-26 0:50 ` Kershner, David A
2015-07-26 8:46 ` Thomas Gleixner
1 sibling, 0 replies; 21+ messages in thread
From: Kershner, David A @ 2015-07-26 0:50 UTC (permalink / raw)
To: Richard Weinberger
Cc: Andrew Morton, Tejun Heo, laijs, nacc, nhorman, Thomas Gleixner,
Ingo Molnar, LKML, jes.sorensen, *S-Par-Maintainer
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6091 bytes --]
> -----Original Message-----
> From: Richard Weinberger [mailto:richard.weinberger@gmail.com]
> Sent: Saturday, July 25, 2015 8:05 AM
> To: Kershner, David A
> Cc: Andrew Morton; Tejun Heo; laijs@cn.fujitsu.com;
> nacc@linux.vnet.ibm.com; nhorman@redhat.com; Thomas Gleixner; Ingo
> Molnar; LKML; jes.sorensen@redhat.com; *S-Par-Maintainer
> Subject: Re: [PATCH] kthread: Export kthread functions
>
> On Sat, Jul 25, 2015 at 12:45 AM, 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.
>
> Are you sure that you need these function?
> You would be the first user.
> Please see: https://lkml.org/lkml/2015/7/8/1150
>
The driver is in staging, and this is the patch that requested it. I'll defer to Neil
logistics of it, but this is why we are attempting this.
From: Neil Horman <nhorman@redhat.com>
Missed this in my initial review. The usage counter that guards against
kthread task is horribly racy. The atomic is self consistent, but theres
nothing that prevents the service_resp_queue function from re-incrementing
it immediately after the check in disable_with_timeout is complete. Its
just a improper usage of atomics as a serialization mechanism.
Instead use kthread_park to pause the thread in its activity so that
buffers can be properly freed without racing against usage in
service_resp_queue
Signed-off-by: Neil Horman <nhorman@redhat.com>
Signed-off-by: Benjamin Romer <benjamin.romer@unisys.com>
---
drivers/staging/unisys/visornic/visornic_main.c | 23 ++++++-----------------
kernel/kthread.c | 4 ++++
2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 4d49937..aeecb14 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -126,7 +126,6 @@ struct visornic_devdata {
unsigned short old_flags; /* flags as they were prior to
* set_multicast_list
*/
- atomic_t usage; /* count of users */
int num_rcv_bufs; /* indicates how many rcv buffers
* the vnic will post
*/
@@ -565,19 +564,7 @@ visornic_disable_with_timeout(struct net_device *netdev, const int timeout)
spin_lock_irqsave(&devdata->priv_lock, flags);
}
- /* Wait for usage to go to 1 (no other users) before freeing
- * rcv buffers
- */
- if (atomic_read(&devdata->usage) > 1) {
- while (1) {
- set_current_state(TASK_INTERRUPTIBLE);
- spin_unlock_irqrestore(&devdata->priv_lock, flags);
- schedule_timeout(msecs_to_jiffies(10));
- spin_lock_irqsave(&devdata->priv_lock, flags);
- if (atomic_read(&devdata->usage))
- break;
- }
- }
+ kthread_park(devdata->threadinfo.task);
/* we've set enabled to 0, so we can give up the lock. */
spin_unlock_irqrestore(&devdata->priv_lock, flags);
@@ -594,6 +581,7 @@ visornic_disable_with_timeout(struct net_device *netdev, const int timeout)
}
}
+ kthread_unpark(devdata->threadinfo.task);
return 0;
}
@@ -1622,7 +1610,7 @@ send_rcv_posts_if_needed(struct visornic_devdata *devdata)
* Returns when response queue is empty or when the threadd stops.
*/
static void
-drain_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
+service_resp_queue(struct uiscmdrsp *cmdrsp, struct visornic_devdata *devdata)
{
unsigned long flags;
struct net_device *netdev;
@@ -1742,6 +1730,8 @@ process_incoming_rsps(void *v)
devdata->rsp_queue, (atomic_read(
&devdata->interrupt_rcvd) == 1),
msecs_to_jiffies(devdata->thread_wait_ms));
+ if (kthread_should_park())
+ kthread_parkme();
/* periodically check to see if there are any rcf bufs which
* need to get sent to the IOSP. This can only happen if
@@ -1749,7 +1739,7 @@ process_incoming_rsps(void *v)
*/
atomic_set(&devdata->interrupt_rcvd, 0);
send_rcv_posts_if_needed(devdata);
- drain_queue(cmdrsp, devdata);
+ service_resp_queue(cmdrsp, devdata);
}
kfree(cmdrsp);
@@ -1809,7 +1799,6 @@ static int visornic_probe(struct visor_device *dev)
init_waitqueue_head(&devdata->rsp_queue);
spin_lock_init(&devdata->priv_lock);
devdata->enabled = 0; /* not yet */
- atomic_set(&devdata->usage, 1);
/* Setup rcv bufs */
channel_offset = offsetof(struct spar_io_channel_protocol,
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c..bad80c1 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -97,6 +97,7 @@ bool kthread_should_park(void)
{
return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
}
+EXPORT_SYMBOL(kthread_should_park);
/**
* kthread_freezable_should_stop - should this freezable kthread return now?
@@ -171,6 +172,7 @@ void kthread_parkme(void)
{
__kthread_parkme(to_kthread(current));
}
+EXPORT_SYMBOL(kthread_parkme);
static int kthread(void *_create)
{
@@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
if (kthread)
__kthread_unpark(k, kthread);
}
+EXPORT_SYMBOL(kthread_unpark);
/**
* kthread_park - park a thread created by kthread_create().
@@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
}
return ret;
}
+EXPORT_SYMBOL(kthread_park);
/**
* kthread_stop - stop a thread created by kthread_create().
--
2.1.4
> --
> Thanks,
> //richard
ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] kthread: Export kthread functions
2015-07-25 12:05 ` Richard Weinberger
2015-07-26 0:50 ` Kershner, David A
@ 2015-07-26 8:46 ` Thomas Gleixner
1 sibling, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2015-07-26 8:46 UTC (permalink / raw)
To: Richard Weinberger
Cc: David Kershner, Andrew Morton, Tejun Heo, laijs, nacc, nhorman,
Ingo Molnar, LKML, jes.sorensen, sparmaintainer
On Sat, 25 Jul 2015, Richard Weinberger wrote:
> On Sat, Jul 25, 2015 at 12:45 AM, 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.
>
> Are you sure that you need these function?
> You would be the first user.
And a reasonable one. The use case is sensible and it's preferrable
that people reuse known to work core facilities instead of creating
their own variants.
> Please see: https://lkml.org/lkml/2015/7/8/1150
Please do not use lkml.org links. lkml.org sucks.
Thanks,
tglx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kthread: Export kthread functions
2015-07-24 22:45 [PATCH] kthread: Export kthread functions David Kershner
2015-07-24 23:14 ` Neil Horman
2015-07-25 12:05 ` Richard Weinberger
@ 2015-07-27 15:45 ` Ingo Molnar
2015-07-27 15:49 ` Neil Horman
2015-07-28 15:59 ` [PATCH v2] " David Kershner
3 siblings, 1 reply; 21+ messages in thread
From: Ingo Molnar @ 2015-07-27 15:45 UTC (permalink / raw)
To: David Kershner
Cc: akpm, tj, laijs, nacc, nhorman, tglx, mingo, linux-kernel,
jes.sorensen, sparmaintainer
* 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>
> ---
> kernel/kthread.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 10e489c..bad80c1 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -97,6 +97,7 @@ bool kthread_should_park(void)
> {
> return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
> }
> +EXPORT_SYMBOL(kthread_should_park);
>
> /**
> * kthread_freezable_should_stop - should this freezable kthread return now?
> @@ -171,6 +172,7 @@ void kthread_parkme(void)
> {
> __kthread_parkme(to_kthread(current));
> }
> +EXPORT_SYMBOL(kthread_parkme);
>
> static int kthread(void *_create)
> {
> @@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
> if (kthread)
> __kthread_unpark(k, kthread);
> }
> +EXPORT_SYMBOL(kthread_unpark);
>
> /**
> * kthread_park - park a thread created by kthread_create().
> @@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
> }
> return ret;
> }
> +EXPORT_SYMBOL(kthread_park);
>
> /**
> * kthread_stop - stop a thread created by kthread_create().
Please make these EXPORT_SYMBOL_GPL(), as kthread_freezable_should_stop() is
already _GPL(), and generally new kthread APIs are exported via
EXPORT_SYMBOL_GPL().
With that change:
Acked-by: Ingo Molnar <mingo@kernel.org>
Thanks,
Ingo
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] kthread: Export kthread functions
2015-07-27 15:45 ` Ingo Molnar
@ 2015-07-27 15:49 ` Neil Horman
2015-07-27 16:01 ` Kershner, David A
0 siblings, 1 reply; 21+ messages in thread
From: Neil Horman @ 2015-07-27 15:49 UTC (permalink / raw)
To: Ingo Molnar
Cc: David Kershner, akpm, tj, laijs, nacc, tglx, mingo, linux-kernel,
jes.sorensen, sparmaintainer
On Mon, Jul 27, 2015 at 05:45:50PM +0200, Ingo Molnar wrote:
>
> * 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>
> > ---
> > kernel/kthread.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 10e489c..bad80c1 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -97,6 +97,7 @@ bool kthread_should_park(void)
> > {
> > return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)->flags);
> > }
> > +EXPORT_SYMBOL(kthread_should_park);
> >
> > /**
> > * kthread_freezable_should_stop - should this freezable kthread return now?
> > @@ -171,6 +172,7 @@ void kthread_parkme(void)
> > {
> > __kthread_parkme(to_kthread(current));
> > }
> > +EXPORT_SYMBOL(kthread_parkme);
> >
> > static int kthread(void *_create)
> > {
> > @@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
> > if (kthread)
> > __kthread_unpark(k, kthread);
> > }
> > +EXPORT_SYMBOL(kthread_unpark);
> >
> > /**
> > * kthread_park - park a thread created by kthread_create().
> > @@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
> > }
> > return ret;
> > }
> > +EXPORT_SYMBOL(kthread_park);
> >
> > /**
> > * kthread_stop - stop a thread created by kthread_create().
>
> Please make these EXPORT_SYMBOL_GPL(), as kthread_freezable_should_stop() is
> already _GPL(), and generally new kthread APIs are exported via
> EXPORT_SYMBOL_GPL().
>
Im ok with that. David?
Neil
> With that change:
>
> Acked-by: Ingo Molnar <mingo@kernel.org>
>
> Thanks,
>
> Ingo
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH] kthread: Export kthread functions
2015-07-27 15:49 ` Neil Horman
@ 2015-07-27 16:01 ` Kershner, David A
0 siblings, 0 replies; 21+ messages in thread
From: Kershner, David A @ 2015-07-27 16:01 UTC (permalink / raw)
To: Neil Horman, Ingo Molnar
Cc: akpm, tj, laijs, nacc, tglx, mingo, linux-kernel, jes.sorensen,
*S-Par-Maintainer
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@redhat.com]
> Sent: Monday, July 27, 2015 11:49 AM
> To: Ingo Molnar
> Cc: Kershner, David A; akpm@linux-foundation.org; tj@kernel.org;
> laijs@cn.fujitsu.com; nacc@linux.vnet.ibm.com; tglx@linutronix.de;
> mingo@redhat.com; linux-kernel@vger.kernel.org;
> jes.sorensen@redhat.com; *S-Par-Maintainer
> Subject: Re: [PATCH] kthread: Export kthread functions
>
> On Mon, Jul 27, 2015 at 05:45:50PM +0200, Ingo Molnar wrote:
> >
> > * 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>
> > > ---
> > > kernel/kthread.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > > index 10e489c..bad80c1 100644
> > > --- a/kernel/kthread.c
> > > +++ b/kernel/kthread.c
> > > @@ -97,6 +97,7 @@ bool kthread_should_park(void)
> > > {
> > > return test_bit(KTHREAD_SHOULD_PARK, &to_kthread(current)-
> >flags);
> > > }
> > > +EXPORT_SYMBOL(kthread_should_park);
> > >
> > > /**
> > > * kthread_freezable_should_stop - should this freezable kthread return
> now?
> > > @@ -171,6 +172,7 @@ void kthread_parkme(void)
> > > {
> > > __kthread_parkme(to_kthread(current));
> > > }
> > > +EXPORT_SYMBOL(kthread_parkme);
> > >
> > > static int kthread(void *_create)
> > > {
> > > @@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
> > > if (kthread)
> > > __kthread_unpark(k, kthread);
> > > }
> > > +EXPORT_SYMBOL(kthread_unpark);
> > >
> > > /**
> > > * kthread_park - park a thread created by kthread_create().
> > > @@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
> > > }
> > > return ret;
> > > }
> > > +EXPORT_SYMBOL(kthread_park);
> > >
> > > /**
> > > * kthread_stop - stop a thread created by kthread_create().
> >
> > Please make these EXPORT_SYMBOL_GPL(), as
> kthread_freezable_should_stop() is
> > already _GPL(), and generally new kthread APIs are exported via
> > EXPORT_SYMBOL_GPL().
> >
> Im ok with that. David?
> Neil
>
Fine with that as well.
David
> > With that change:
> >
> > Acked-by: Ingo Molnar <mingo@kernel.org>
> >
> > Thanks,
> >
> > Ingo
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] kthread: Export kthread functions
2015-07-24 22:45 [PATCH] kthread: Export kthread functions David Kershner
` (2 preceding siblings ...)
2015-07-27 15:45 ` Ingo Molnar
@ 2015-07-28 15:59 ` David Kershner
2015-07-28 21:27 ` Andrew Morton
3 siblings, 1 reply; 21+ messages in thread
From: David Kershner @ 2015-07-28 15:59 UTC (permalink / raw)
To: akpm
Cc: tj, laijs, nacc, nhorman, tglx, mingo, linux-kernel,
jes.sorensen, sparmaintainer, David Kershner
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>
---
kernel/kthread.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 10e489c..fdea0be 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -97,6 +97,7 @@ 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?
@@ -171,6 +172,7 @@ void kthread_parkme(void)
{
__kthread_parkme(to_kthread(current));
}
+EXPORT_SYMBOL_GPL(kthread_parkme);
static int kthread(void *_create)
{
@@ -411,6 +413,7 @@ void kthread_unpark(struct task_struct *k)
if (kthread)
__kthread_unpark(k, kthread);
}
+EXPORT_SYMBOL_GPL(kthread_unpark);
/**
* kthread_park - park a thread created by kthread_create().
@@ -441,6 +444,7 @@ int kthread_park(struct task_struct *k)
}
return ret;
}
+EXPORT_SYMBOL_GPL(kthread_park);
/**
* kthread_stop - stop a thread created by kthread_create().
--
1.9.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-28 15:59 ` [PATCH v2] " David Kershner
@ 2015-07-28 21:27 ` Andrew Morton
2015-07-29 10:34 ` Thomas Gleixner
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Morton @ 2015-07-28 21:27 UTC (permalink / raw)
To: David Kershner
Cc: tj, laijs, nacc, nhorman, tglx, mingo, linux-kernel,
jes.sorensen, sparmaintainer
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-28 21:27 ` Andrew Morton
@ 2015-07-29 10:34 ` Thomas Gleixner
2015-07-30 3:48 ` yalin wang
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-07-29 10:34 UTC (permalink / raw)
To: Andrew Morton
Cc: David Kershner, tj, laijs, nacc, nhorman, mingo, linux-kernel,
jes.sorensen, sparmaintainer
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.
Acked-by: Thomas Gleixner <tglx@linutronix.de>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-29 10:34 ` Thomas Gleixner
@ 2015-07-30 3:48 ` yalin wang
2015-07-30 12:02 ` Neil Horman
0 siblings, 1 reply; 21+ messages in thread
From: yalin wang @ 2015-07-30 3:48 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Andrew Morton, David Kershner, tj, laijs, nacc, nhorman, mingo,
linux-kernel, jes.sorensen, sparmaintainer
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-30 3:48 ` yalin wang
@ 2015-07-30 12:02 ` Neil Horman
2015-07-31 4:16 ` yalin wang
0 siblings, 1 reply; 21+ messages in thread
From: Neil Horman @ 2015-07-30 12:02 UTC (permalink / raw)
To: yalin wang
Cc: Thomas Gleixner, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, linux-kernel, jes.sorensen, sparmaintainer
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-30 12:02 ` Neil Horman
@ 2015-07-31 4:16 ` yalin wang
2015-07-31 8:19 ` Thomas Gleixner
0 siblings, 1 reply; 21+ messages in thread
From: yalin wang @ 2015-07-31 4:16 UTC (permalink / raw)
To: Neil Horman
Cc: Thomas Gleixner, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
> 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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-31 4:16 ` yalin wang
@ 2015-07-31 8:19 ` Thomas Gleixner
2015-07-31 14:14 ` Thomas Gleixner
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-07-31 8:19 UTC (permalink / raw)
To: yalin wang
Cc: Neil Horman, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
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.
Thanks,
tglx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-31 8:19 ` Thomas Gleixner
@ 2015-07-31 14:14 ` Thomas Gleixner
2015-08-01 7:12 ` yalin wang
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Gleixner @ 2015-07-31 14:14 UTC (permalink / raw)
To: yalin wang
Cc: Neil Horman, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-07-31 14:14 ` Thomas Gleixner
@ 2015-08-01 7:12 ` yalin wang
2015-08-01 7:20 ` Thomas Gleixner
2015-08-01 13:32 ` Neil Horman
0 siblings, 2 replies; 21+ messages in thread
From: yalin wang @ 2015-08-01 7:12 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Neil Horman, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
> 在 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
you can see text size is reduced.
in addition, because we don’t use EXPORT_SYMBOLS() for inline function,
some data can also be removed,
[27] __ksymtab_strings PROGBITS 0000000000000000 000a7fe0
0000000000003907 0000000000000000 A 0 0 1
0x3907
[27] __ksymtab_strings PROGBITS 0000000000000000 000a8020
000000000000392f 0000000000000000 A 0 0 1
0x392f
you can see after apply the patch, this string section is also reduced,
but size command output seems not calculate these special section,
there are lots of special sections in linux kernel image,
i think size command is not suitable to calculate kernel image, maybe it just
calculate formal section which name like .data .text etc..
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-08-01 7:12 ` yalin wang
@ 2015-08-01 7:20 ` Thomas Gleixner
2015-08-01 13:32 ` Neil Horman
1 sibling, 0 replies; 21+ messages in thread
From: Thomas Gleixner @ 2015-08-01 7:20 UTC (permalink / raw)
To: yalin wang
Cc: Neil Horman, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
On Sat, 1 Aug 2015, yalin wang wrote:
> 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
Not all architectures/compilers are giving the same results.
> i think size command is not suitable to calculate kernel image, maybe it just
> calculate formal section which name like .data .text etc..
Nonsense. .text is .text no matter what.
Anyway, it's not going to change.
Thanks,
tglx
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-08-01 7:12 ` yalin wang
2015-08-01 7:20 ` Thomas Gleixner
@ 2015-08-01 13:32 ` Neil Horman
2015-08-03 2:23 ` yalin wang
1 sibling, 1 reply; 21+ messages in thread
From: Neil Horman @ 2015-08-01 13:32 UTC (permalink / raw)
To: yalin wang
Cc: Thomas Gleixner, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-08-01 13:32 ` Neil Horman
@ 2015-08-03 2:23 ` yalin wang
2015-08-03 2:42 ` Jes Sorensen
0 siblings, 1 reply; 21+ messages in thread
From: yalin wang @ 2015-08-03 2:23 UTC (permalink / raw)
To: Neil Horman
Cc: Thomas Gleixner, Andrew Morton, David Kershner, tj, laijs, nacc,
mingo, open list, jes.sorensen, sparmaintainer
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] kthread: Export kthread functions
2015-08-03 2:23 ` yalin wang
@ 2015-08-03 2:42 ` Jes Sorensen
0 siblings, 0 replies; 21+ messages in thread
From: Jes Sorensen @ 2015-08-03 2:42 UTC (permalink / raw)
To: yalin wang
Cc: Neil Horman, Thomas Gleixner, Andrew Morton, David Kershner, tj,
laijs, nacc, mingo, open list, sparmaintainer
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-08-03 2:42 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-24 22:45 [PATCH] kthread: Export kthread functions David Kershner
2015-07-24 23:14 ` Neil Horman
2015-07-25 12:05 ` Richard Weinberger
2015-07-26 0:50 ` Kershner, David A
2015-07-26 8:46 ` Thomas Gleixner
2015-07-27 15:45 ` Ingo Molnar
2015-07-27 15:49 ` Neil Horman
2015-07-27 16:01 ` Kershner, David A
2015-07-28 15:59 ` [PATCH v2] " David Kershner
2015-07-28 21:27 ` Andrew Morton
2015-07-29 10:34 ` Thomas Gleixner
2015-07-30 3:48 ` yalin wang
2015-07-30 12:02 ` Neil Horman
2015-07-31 4:16 ` yalin wang
2015-07-31 8:19 ` Thomas Gleixner
2015-07-31 14:14 ` Thomas Gleixner
2015-08-01 7:12 ` yalin wang
2015-08-01 7:20 ` Thomas Gleixner
2015-08-01 13:32 ` Neil Horman
2015-08-03 2:23 ` yalin wang
2015-08-03 2:42 ` Jes Sorensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).