RE: [PATCH] kthread: Export kthread functions
> -Original Message-
> From: Neil Horman [mailto:nhor...@redhat.com]
> Sent: Monday, July 27, 2015 11:49 AM
> To: Ingo Molnar
> Cc: Kershner, David A; a...@linux-foundation.org; t...@kernel.org;
> la...@cn.fujitsu.com; n...@linux.vnet.ibm.com; t...@linutronix.de;
> mi...@redhat.com; linux-kernel@vger.kernel.org;
> jes.soren...@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 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
> > > ---
> > > 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
> >
> > Thanks,
> >
> > Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Export kthread functions
On Mon, Jul 27, 2015 at 05:45:50PM +0200, Ingo Molnar wrote:
>
> * 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
> > ---
> > 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
>
> Thanks,
>
> Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Export kthread functions
* 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
> ---
> 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
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Export kthread functions
On Sat, 25 Jul 2015, Richard Weinberger wrote: > On Sat, Jul 25, 2015 at 12:45 AM, 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] kthread: Export kthread functions
> -Original Message-
> From: Richard Weinberger [mailto:richard.weinber...@gmail.com]
> Sent: Saturday, July 25, 2015 8:05 AM
> To: Kershner, David A
> Cc: Andrew Morton; Tejun Heo; la...@cn.fujitsu.com;
> n...@linux.vnet.ibm.com; nhor...@redhat.com; Thomas Gleixner; Ingo
> Molnar; LKML; jes.soren...@redhat.com; *S-Par-Maintainer
> Subject: Re: [PATCH] kthread: Export kthread functions
>
> On Sat, Jul 25, 2015 at 12:45 AM, 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.
>
> 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
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
Signed-off-by: Benjamin Romer
---
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();
Re: [PATCH] kthread: Export kthread functions
On Sat, Jul 25, 2015 at 12:45 AM, 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] kthread: Export kthread functions
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
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
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
