* Re: [PATCH] kthread: airo.c
2006-07-13 20:53 [PATCH] kthread: airo.c Sukadev Bhattiprolu
@ 2006-07-13 21:28 ` Christoph Hellwig
2006-07-13 23:00 ` Sukadev Bhattiprolu
2006-07-24 18:13 ` Sukadev Bhattiprolu
1 sibling, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2006-07-13 21:28 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: akpm, achirica, David C. Hansen, serue, clg, linux-kernel
On Thu, Jul 13, 2006 at 01:53:19PM -0700, Sukadev Bhattiprolu wrote:
> Andrew,
>
> Javier Achirica, one of the major contributors to drivers/net/wireless/airo.c
> took a look at this patch, and doesn't have any problems with it. It doesn't
> fix any bugs and is just a cleanup, so it certainly isn't a candidate
> for this mainline cycle
I'm not sure it's that easy. I think it needs some more love:
- switch to wake_uo_process
- kill JOB_DIE
- cleanup a the convoluted mess in airo_thread a bit
Note that it's still reimplementing the single threaded workqueue
functionality quite badly. So if someone could switch it over and while
we're at it try to kill the idiociy of doing the trylock in the calling
context and only then calling the thread by always calling the thread
(which also solves the synchronization problem).
Anywhy, here's a small incremental patch ontop of yours to implement my
above items:
Index: linux-2.6/drivers/net/wireless/airo.c
===================================================================
--- linux-2.6.orig/drivers/net/wireless/airo.c 2006-07-13 23:23:28.000000000 +0200
+++ linux-2.6/drivers/net/wireless/airo.c 2006-07-13 23:25:38.000000000 +0200
@@ -1173,7 +1173,6 @@
#define FLAG_FLASHING 15
#define FLAG_WPA_CAPABLE 16
unsigned long flags;
-#define JOB_DIE 0
#define JOB_XMIT 1
#define JOB_XMIT11 2
#define JOB_STATS 3
@@ -1191,7 +1190,6 @@
struct task_struct *list_bss_task;
struct task_struct *airo_thread_task;
struct semaphore sem;
- wait_queue_head_t thr_wait;
unsigned long expires;
struct {
struct sk_buff *skb;
@@ -2182,7 +2180,7 @@
set_bit(FLAG_PENDING_XMIT, &priv->flags);
netif_stop_queue(dev);
set_bit(JOB_XMIT, &priv->jobs);
- wake_up_interruptible(&priv->thr_wait);
+ wake_up_process(priv->airo_thread_task);
} else
airo_end_xmit(dev);
return 0;
@@ -2253,7 +2251,7 @@
set_bit(FLAG_PENDING_XMIT11, &priv->flags);
netif_stop_queue(dev);
set_bit(JOB_XMIT11, &priv->jobs);
- wake_up_interruptible(&priv->thr_wait);
+ wake_up_process(priv->airo_thread_task);
} else
airo_end_xmit11(dev);
return 0;
@@ -2295,7 +2293,7 @@
/* Get stats out of the card if available */
if (down_trylock(&local->sem) != 0) {
set_bit(JOB_STATS, &local->jobs);
- wake_up_interruptible(&local->thr_wait);
+ wake_up_process(local->airo_thread_task);
} else
airo_read_stats(local);
}
@@ -2322,7 +2320,7 @@
change_bit(FLAG_PROMISC, &ai->flags);
if (down_trylock(&ai->sem) != 0) {
set_bit(JOB_PROMISC, &ai->jobs);
- wake_up_interruptible(&ai->thr_wait);
+ wake_up_process(ai->airo_thread_task);
} else
airo_set_promisc(ai);
}
@@ -2399,7 +2397,6 @@
}
clear_bit(FLAG_REGISTERED, &ai->flags);
}
- set_bit(JOB_DIE, &ai->jobs);
kthread_stop(ai->airo_thread_task);
/*
@@ -2809,7 +2806,6 @@
sema_init(&ai->sem, 1);
ai->config.len = 0;
ai->pci = pci;
- init_waitqueue_head (&ai->thr_wait);
ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
if (IS_ERR(ai->airo_thread_task))
goto err_out_free;
@@ -2927,7 +2923,6 @@
err_out_unlink:
del_airo_dev(dev);
err_out_thr:
- set_bit(JOB_DIE, &ai->jobs);
kthread_stop(ai->airo_thread_task);
err_out_free:
free_netdev(dev);
@@ -3055,70 +3050,52 @@
wireless_send_event(ai->dev, SIOCGIWSCAN, &wrqu, NULL);
}
-static int airo_thread(void *data) {
+static int airo_thread(void *data)
+{
struct net_device *dev = data;
struct airo_info *ai = dev->priv;
- int locked;
- while(1) {
+ while (1) {
/* make swsusp happy with our thread */
try_to_freeze();
- if (test_bit(JOB_DIE, &ai->jobs))
- break;
+ for (;;) {
+ unsigned long wake_at;
- if (ai->jobs) {
- locked = down_interruptible(&ai->sem);
- } else {
- wait_queue_t wait;
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (kthread_should_stop())
+ break;
+ if (ai->jobs)
+ break;
+ if (ai->scan_timeout &&
+ time_after_eq(jiffies, ai->scan_timeout)) {
+ set_bit(JOB_SCAN_RESULTS, &ai->jobs);
+ break;
+ }
- init_waitqueue_entry(&wait, current);
- add_wait_queue(&ai->thr_wait, &wait);
- for (;;) {
- set_current_state(TASK_INTERRUPTIBLE);
- if (ai->jobs)
- break;
- if (ai->expires || ai->scan_timeout) {
- if (ai->scan_timeout &&
- time_after_eq(jiffies,ai->scan_timeout)){
- set_bit(JOB_SCAN_RESULTS, &ai->jobs);
- break;
- } else if (ai->expires &&
- time_after_eq(jiffies,ai->expires)){
- set_bit(JOB_AUTOWEP, &ai->jobs);
- break;
- }
- if (!kthread_should_stop()) {
- unsigned long wake_at;
- if (!ai->expires || !ai->scan_timeout) {
- wake_at = max(ai->expires,
- ai->scan_timeout);
- } else {
- wake_at = min(ai->expires,
- ai->scan_timeout);
- }
- schedule_timeout(wake_at - jiffies);
- continue;
- }
- } else if (!kthread_should_stop()) {
- schedule();
- continue;
- }
+ if (ai->expires &&
+ time_after_eq(jiffies, ai->expires)){
+ set_bit(JOB_AUTOWEP, &ai->jobs);
break;
}
- current->state = TASK_RUNNING;
- remove_wait_queue(&ai->thr_wait, &wait);
- locked = 1;
- }
- if (locked)
- continue;
+ if (ai->expires && ai->scan_timeout)
+ wake_at = min(ai->expires, ai->scan_timeout);
+ else if (ai->expires || ai->scan_timeout)
+ wake_at = max(ai->expires, ai->scan_timeout);
+ else
+ wake_at = MAX_SCHEDULE_TIMEOUT;
- if (test_bit(JOB_DIE, &ai->jobs)) {
- up(&ai->sem);
- break;
+ schedule_timeout(wake_at - jiffies);
}
+ __set_current_state(TASK_RUNNING);
+ if (kthread_should_stop())
+ break;
+
+ if (down_interruptible(&ai->sem))
+ continue;
+
if (ai->power.event || test_bit(FLAG_FLASHING, &ai->flags)) {
up(&ai->sem);
continue;
@@ -3180,7 +3157,7 @@
OUT4500( apriv, EVACK, EV_MIC );
if (test_bit(FLAG_MIC_CAPABLE, &apriv->flags)) {
set_bit(JOB_MIC, &apriv->jobs);
- wake_up_interruptible(&apriv->thr_wait);
+ wake_up_process(apriv->airo_thread_task);
}
}
if ( status & EV_LINK ) {
@@ -3234,13 +3211,13 @@
if (down_trylock(&apriv->sem) != 0) {
set_bit(JOB_EVENT, &apriv->jobs);
- wake_up_interruptible(&apriv->thr_wait);
+ wake_up_process(apriv->airo_thread_task);
} else
airo_send_event(dev);
} else if (!scan_forceloss) {
if (auto_wep && !apriv->expires) {
apriv->expires = RUN_AT(3*HZ);
- wake_up_interruptible(&apriv->thr_wait);
+ wake_up_process(apriv->airo_thread_task);
}
/* Send event to user space */
@@ -3903,7 +3880,7 @@
if (auto_wep) {
ai->expires = RUN_AT(3*HZ);
- wake_up_interruptible(&ai->thr_wait);
+ wake_up_process(ai->airo_thread_task);
}
return SUCCESS;
@@ -7179,7 +7156,7 @@
out:
up(&ai->sem);
if (wake)
- wake_up_interruptible(&ai->thr_wait);
+ wake_up_process(ai->airo_thread_task);
return 0;
}
@@ -7677,7 +7654,7 @@
/* Get stats out of the card if available */
if (down_trylock(&local->sem) != 0) {
set_bit(JOB_WSTATS, &local->jobs);
- wake_up_interruptible(&local->thr_wait);
+ wake_up_process(local->airo_thread_task);
} else
airo_read_wireless_stats(local);
}
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kthread: airo.c
2006-07-13 20:53 [PATCH] kthread: airo.c Sukadev Bhattiprolu
2006-07-13 21:28 ` Christoph Hellwig
@ 2006-07-24 18:13 ` Sukadev Bhattiprolu
2006-07-26 6:18 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Sukadev Bhattiprolu @ 2006-07-24 18:13 UTC (permalink / raw)
To: akpm
Cc: achirica, hch, linville, David C. Hansen, serue, clr, netdev,
linux-kernel
Sukadev Bhattiprolu [sukadev@us.ibm.com] wrote:
| Andrew,
|
| Javier Achirica, one of the major contributors to drivers/net/wireless/airo.c
| took a look at this patch, and doesn't have any problems with it. It doesn't
| fix any bugs and is just a cleanup, so it certainly isn't a candidate
| for this mainline cycle
Here is the same patch, merged up to 2.6.18-rc2. Christoph's patch (see
http://lkml.org/lkml/2006/7/13/332) still applies cleanly on top of this.
-----
The airo driver is currently caching a pid for later use, but with the
implementation of containers, pids themselves do not uniquely identify
a task. The driver is also using kernel_thread() which is deprecated in
drivers.
This patch essentially replaces the kernel_thread() with kthread_create().
It also stores the task_struct of the airo_thread rather than its pid.
Since this introduces a second task_struct in struct airo_info, the patch
renames airo_info.task to airo_info.list_bss_task.
As an extension of these changes, the patch further:
- replaces kill_proc() with kthread_stop()
- replaces signal_pending() with kthread_should_stop()
- removes thread completion synchronisation which is handled by
kthread_stop().
Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>
Cc: Javier Achirica <achirica@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: John Linville <linville@tuxdriver.com>
drivers/net/wireless/airo.c | 38 +++++++++++++++-----------------------
1 files changed, 15 insertions(+), 23 deletions(-)
Index: linux-2.6.18-rc1-mm2/drivers/net/wireless/airo.c
===================================================================
--- linux-2.6.18-rc1-mm2.orig/drivers/net/wireless/airo.c 2006-07-14 14:04:01.000000000 -0700
+++ linux-2.6.18-rc1-mm2/drivers/net/wireless/airo.c 2006-07-20 19:44:50.000000000 -0700
@@ -47,6 +47,7 @@
#include <linux/pci.h>
#include <asm/uaccess.h>
#include <net/ieee80211.h>
+#include <linux/kthread.h>
#include "airo.h"
@@ -1187,11 +1188,10 @@ struct airo_info {
int whichbap);
unsigned short *flash;
tdsRssiEntry *rssi;
- struct task_struct *task;
+ struct task_struct *list_bss_task;
+ struct task_struct *airo_thread_task;
struct semaphore sem;
- pid_t thr_pid;
wait_queue_head_t thr_wait;
- struct completion thr_exited;
unsigned long expires;
struct {
struct sk_buff *skb;
@@ -1736,9 +1736,9 @@ static int readBSSListRid(struct airo_in
issuecommand(ai, &cmd, &rsp);
up(&ai->sem);
/* Let the command take effect */
- ai->task = current;
+ ai->list_bss_task = current;
ssleep(3);
- ai->task = NULL;
+ ai->list_bss_task = NULL;
}
rc = PC4500_readrid(ai, first ? ai->bssListFirst : ai->bssListNext,
list, ai->bssListRidLen, 1);
@@ -2400,8 +2400,7 @@ void stop_airo_card( struct net_device *
clear_bit(FLAG_REGISTERED, &ai->flags);
}
set_bit(JOB_DIE, &ai->jobs);
- kill_proc(ai->thr_pid, SIGTERM, 1);
- wait_for_completion(&ai->thr_exited);
+ kthread_stop(ai->airo_thread_task);
/*
* Clean out tx queue
@@ -2811,9 +2810,8 @@ static struct net_device *_init_airo_car
ai->config.len = 0;
ai->pci = pci;
init_waitqueue_head (&ai->thr_wait);
- init_completion (&ai->thr_exited);
- ai->thr_pid = kernel_thread(airo_thread, dev, CLONE_FS | CLONE_FILES);
- if (ai->thr_pid < 0)
+ ai->airo_thread_task = kthread_run(airo_thread, dev, dev->name);
+ if (IS_ERR(ai->airo_thread_task))
goto err_out_free;
ai->tfm = NULL;
rc = add_airo_dev( dev );
@@ -2930,8 +2928,7 @@ err_out_unlink:
del_airo_dev(dev);
err_out_thr:
set_bit(JOB_DIE, &ai->jobs);
- kill_proc(ai->thr_pid, SIGTERM, 1);
- wait_for_completion(&ai->thr_exited);
+ kthread_stop(ai->airo_thread_task);
err_out_free:
free_netdev(dev);
return NULL;
@@ -3063,13 +3060,7 @@ static int airo_thread(void *data) {
struct airo_info *ai = dev->priv;
int locked;
- daemonize("%s", dev->name);
- allow_signal(SIGTERM);
-
while(1) {
- if (signal_pending(current))
- flush_signals(current);
-
/* make swsusp happy with our thread */
try_to_freeze();
@@ -3097,7 +3088,7 @@ static int airo_thread(void *data) {
set_bit(JOB_AUTOWEP, &ai->jobs);
break;
}
- if (!signal_pending(current)) {
+ if (!kthread_should_stop()) {
unsigned long wake_at;
if (!ai->expires || !ai->scan_timeout) {
wake_at = max(ai->expires,
@@ -3109,7 +3100,7 @@ static int airo_thread(void *data) {
schedule_timeout(wake_at - jiffies);
continue;
}
- } else if (!signal_pending(current)) {
+ } else if (!kthread_should_stop()) {
schedule();
continue;
}
@@ -3154,7 +3145,8 @@ static int airo_thread(void *data) {
else /* Shouldn't get here, but we make sure to unlock */
up(&ai->sem);
}
- complete_and_exit (&ai->thr_exited, 0);
+
+ return 0;
}
static irqreturn_t airo_interrupt ( int irq, void* dev_id, struct pt_regs *regs) {
@@ -3235,8 +3227,8 @@ static irqreturn_t airo_interrupt ( int
if(newStatus == ASSOCIATED || newStatus == REASSOCIATED) {
if (auto_wep)
apriv->expires = 0;
- if (apriv->task)
- wake_up_process (apriv->task);
+ if (apriv->list_bss_task)
+ wake_up_process(apriv->list_bss_task);
set_bit(FLAG_UPDATE_UNI, &apriv->flags);
set_bit(FLAG_UPDATE_MULTI, &apriv->flags);
^ permalink raw reply [flat|nested] 6+ messages in thread