Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755169AbbGZA5m (ORCPT ); Sat, 25 Jul 2015 20:57:42 -0400 Received: from mail1.bemta7.messagelabs.com ([216.82.254.100]:41345 "EHLO mail1.bemta7.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755092AbbGZA5k (ORCPT ); Sat, 25 Jul 2015 20:57:40 -0400 X-Greylist: delayed 431 seconds by postgrey-1.27 at vger.kernel.org; Sat, 25 Jul 2015 20:57:40 EDT X-Env-Sender: David.Kershner@unisys.com X-Msg-Ref: server-10.tower-201.messagelabs.com!1437871818!23201659!10 X-Originating-IP: [192.61.61.104] X-StarScan-Received: X-StarScan-Version: 6.13.16; banners=-,-,- X-VirusChecked: Checked From: "Kershner, David A" To: Richard Weinberger 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 Thread-Topic: [PATCH] kthread: Export kthread functions Thread-Index: AQHQxmJ4ghdWVEb8dk+4gAIK6Itam53sa2GAgACBc5A= Date: Sun, 26 Jul 2015 00:50:00 +0000 Message-ID: References: <1437777920-31156-1-git-send-email-david.kershner@unisys.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [172.17.124.182] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t6Q0vpXY002138 Content-Length: 6091 Lines: 172 > -----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 > 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(); /* 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?