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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
On Sat, Jul 25, 2015 at 12:45 AM, David Kershner
<[email protected]> 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
> -----Original Message-----
> From: Richard Weinberger [mailto:[email protected]]
> Sent: Saturday, July 25, 2015 8:05 AM
> To: Kershner, David A
> Cc: Andrew Morton; Tejun Heo; [email protected];
> [email protected]; [email protected]; Thomas Gleixner; Ingo
> Molnar; LKML; [email protected]; *S-Par-Maintainer
> Subject: Re: [PATCH] kthread: Export kthread functions
>
> On Sat, Jul 25, 2015 at 12:45 AM, David Kershner
> <[email protected]> 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 <[email protected]>
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 <[email protected]>
Signed-off-by: Benjamin Romer <[email protected]>
---
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Sat, 25 Jul 2015, Richard Weinberger wrote:
> On Sat, Jul 25, 2015 at 12:45 AM, David Kershner
> <[email protected]> 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
* David Kershner <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks,
Ingo
On Mon, Jul 27, 2015 at 05:45:50PM +0200, Ingo Molnar wrote:
>
> * David Kershner <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> Thanks,
>
> Ingo
> -----Original Message-----
> From: Neil Horman [mailto:[email protected]]
> Sent: Monday, July 27, 2015 11:49 AM
> To: Ingo Molnar
> Cc: Kershner, David A; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; *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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> >
> > Thanks,
> >
> > Ingo
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 <[email protected]>
---
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
On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <[email protected]> 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 <[email protected]>
Please accumulate the acked-by's and reviewed-by's in the changelog as
they are received. I presently have
Acked-by: Ingo Molnar <[email protected]>
Acked-by: Neil Horman <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: Tejun Heo <[email protected]>
I'll scoot this into mainline probably this week to make life simpler
for the various trees.
On Tue, 28 Jul 2015, Andrew Morton wrote:
> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <[email protected]> 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 <[email protected]>
>
> Please accumulate the acked-by's and reviewed-by's in the changelog as
> they are received. I presently have
>
> Acked-by: Ingo Molnar <[email protected]>
> Acked-by: Neil Horman <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Tejun Heo <[email protected]>
>
>
> I'll scoot this into mainline probably this week to make life simpler
> for the various trees.
Acked-by: Thomas Gleixner <[email protected]>
> On Jul 29, 2015, at 18:34, Thomas Gleixner <[email protected]> wrote:
>
> On Tue, 28 Jul 2015, Andrew Morton wrote:
>
>> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <[email protected]> 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 <[email protected]>
>>
>> Please accumulate the acked-by's and reviewed-by's in the changelog as
>> they are received. I presently have
>>
>> Acked-by: Ingo Molnar <[email protected]>
>> Acked-by: Neil Horman <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Richard Weinberger <[email protected]>
>> Cc: Tejun Heo <[email protected]>
>>
>>
>> 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-
On Thu, Jul 30, 2015 at 11:48:17AM +0800, yalin wang wrote:
>
> > On Jul 29, 2015, at 18:34, Thomas Gleixner <[email protected]> wrote:
> >
> > On Tue, 28 Jul 2015, Andrew Morton wrote:
> >
> >> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <[email protected]> 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 <[email protected]>
> >>
> >> Please accumulate the acked-by's and reviewed-by's in the changelog as
> >> they are received. I presently have
> >>
> >> Acked-by: Ingo Molnar <[email protected]>
> >> Acked-by: Neil Horman <[email protected]>
> >> Cc: Thomas Gleixner <[email protected]>
> >> Cc: Richard Weinberger <[email protected]>
> >> Cc: Tejun Heo <[email protected]>
> >>
> >>
> >> 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
> On Jul 30, 2015, at 20:02, Neil Horman <[email protected]> wrote:
>
> On Thu, Jul 30, 2015 at 11:48:17AM +0800, yalin wang wrote:
>>
>>> On Jul 29, 2015, at 18:34, Thomas Gleixner <[email protected]> wrote:
>>>
>>> On Tue, 28 Jul 2015, Andrew Morton wrote:
>>>
>>>> On Tue, 28 Jul 2015 11:59:01 -0400 David Kershner <[email protected]> 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 <[email protected]>
>>>>
>>>> Please accumulate the acked-by's and reviewed-by's in the changelog as
>>>> they are received. I presently have
>>>>
>>>> Acked-by: Ingo Molnar <[email protected]>
>>>> Acked-by: Neil Horman <[email protected]>
>>>> Cc: Thomas Gleixner <[email protected]>
>>>> Cc: Richard Weinberger <[email protected]>
>>>> Cc: Tejun Heo <[email protected]>
>>>>
>>>>
>>>> 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
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
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
> ?? 2015??7??31?գ?22:14??Thomas Gleixner <[email protected]> д????
>
>
> 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-
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
On Sat, Aug 01, 2015 at 03:12:42PM +0800, yalin wang wrote:
>
> > 在 2015年7月31日，22:14，Thomas Gleixner <[email protected]> 写道：
> >
> >
> > 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
> On Aug 1, 2015, at 21:32, Neil Horman <[email protected]> wrote:
>
> On Sat, Aug 01, 2015 at 03:12:42PM +0800, yalin wang wrote:
>>
>>> 在 2015年7月31日，22:14，Thomas Gleixner <[email protected]> 写道：
>>>
>>>
>>> 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
yalin wang <[email protected]> writes:
>> On Aug 1, 2015, at 21:32, Neil Horman <[email protected]> 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