Skip to contentSkip to conversation

net/core: use wait_event_timeout()

PATCH Applied Nishanth Aravamudan 4 participants 7 messages Jan 20, 2005 – Jan 26, 2005

Subsystems (3)
Bluetooth drivers Kernel janitors Bluetooth
lore
Share & tools
lore

Nishanth Aravamudan

author
#0 lore raw

PATCH 18/39 net/core: use wait_event_timeout()

Hi,

Please consider applying.

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Signed-off-by Nishanth Aravamudan <[email protected]>
net/bluetooth/hidp/core.c +4 -20
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>

@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 

@@ -370,28 +370,12 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(ctrl_sk->sk_sleep, (ctrl_sk->sk_state == BT_CLOSED),	HZ);
 
 	fput(session->ctrl_sock->file);
 
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(intr_sk->sk_sleep, &intr_wait);
-	while (timeo && intr_sk->sk_state != BT_CLOSED) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		timeo = schedule_timeout(timeo);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(intr_sk->sk_sleep, &intr_wait);
+	wait_event_timeout(intr_sk->sk_state, (intr_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->intr_sock->file);

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#1 lore raw

Hi Nishanth,

Quoted 4 lines

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

this one results in:

CC [M] net/bluetooth/hidp/core.o
net/bluetooth/hidp/core.c: In function `hidp_session':
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of prepare_to_wait' from incompatible pointer type net/bluetooth/hidp/core.c:547: warning: passing arg 1 of finish_wait' from incompatible pointer type net/bluetooth/hidp/core.c:551: warning: passing arg 1 of prepare_to_wait' from incompatible pointer type net/bluetooth/hidp/core.c:551: warning: passing arg 1 of finish_wait' from incompatible pointer type

Please fix this and re-submit.

Regards

Marcel

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

author
#2 lore raw

PATCH 18/39 net/core: use wait_event_timeout()

Quoted 7 lines · Marcel Holtmann

Hi Nishanth,

Quoted 4 lines

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Quoted 8 lines

this one results in:

CC [M] net/bluetooth/hidp/core.o
net/bluetooth/hidp/core.c: In function `hidp_session':
net/bluetooth/hidp/core.c:547: warning: passing arg 1 of prepare_to_wait' from incompatible pointer type net/bluetooth/hidp/core.c:547: warning: passing arg 1 of finish_wait' from incompatible pointer type net/bluetooth/hidp/core.c:551: warning: passing arg 1 of prepare_to_wait' from incompatible pointer type net/bluetooth/hidp/core.c:551: warning: passing arg 1 of finish_wait' from incompatible pointer type

Sorry for that, Marcel. The calling case in that function is slightly different than usual...fixed below.

Thanks,
Nish

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Signed-off-by Nishanth Aravamudan <[email protected]>
net/bluetooth/hidp/core.c +6 -20
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>

@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 

@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(*(ctrl_sk->sk_sleep),
+				(ctrl_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->ctrl_sock->file);
 
-	init_waitqueue_entry(&intr_wait, current);
-	add_wait_queue(intr_sk->sk_sleep, &intr_wait);
-	while (timeo && intr_sk->sk_state != BT_CLOSED) {
-		set_current_state(TASK_INTERRUPTIBLE);
-		timeo = schedule_timeout(timeo);
-	}
-	set_current_state(TASK_RUNNING);
-	remove_wait_queue(intr_sk->sk_sleep, &intr_wait);
+	wait_event_timeout(*(ctrl_sk->sk_sleep),
+			(intr_sk->sk_state == BT_CLOSED), HZ);
 
 	fput(session->intr_sock->file);

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#3 lore raw

Hi Nishanth,

Quoted 34 lines

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Signed-off-by Nishanth Aravamudan <[email protected]>
net/bluetooth/hidp/core.c +1 -5
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>

@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
 	struct sk_buff *skb;
 	int vendor = 0x0000, product = 0x0000;
 	wait_queue_t ctrl_wait, intr_wait;
-	unsigned long timeo = HZ;
 
 	BT_DBG("session %p", session);
 

@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
 	hidp_del_timer(session);
 
-	if (intr_sk->sk_state != BT_CONNECTED) {
-		init_waitqueue_entry(&ctrl_wait, current);
-		add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		while (timeo && ctrl_sk->sk_state != BT_CLOSED) {

net/bluetooth/hidp/core.c static int hidp_session(void *arg) · line 375

Quoted 10 lines
net/bluetooth/hidp/core.c +3 -7
-			set_current_state(TASK_INTERRUPTIBLE);
-			timeo = schedule_timeout(timeo);
-		}
-		set_current_state(TASK_RUNNING);
-		remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-		timeo = HZ;
-	}
+	if (intr_sk->sk_state != BT_CONNECTED)
+		wait_event_timeout(*(ctrl_sk->sk_sleep),
+				(ctrl_sk->sk_state == BT_CLOSED), HZ);

and this does not make it look better. For me this is too much pointer stuff here. I think the interface of wait_event_timeout() is wrong. It should take a pointer to wait_queue_head_t.

Regards

Marcel

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#4 lore raw
Quoted 42 lines · Marcel Holtmann

Hi Nishanth,

Quoted 40 lines

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Signed-off-by Nishanth Aravamudan <[email protected]>
net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 renamed from net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800 +1 -11
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>

@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
      struct sk_buff *skb;
      int vendor = 0x0000, product = 0x0000;
      wait_queue_t ctrl_wait, intr_wait;
-     unsigned long timeo = HZ;
 
      BT_DBG("session %p", session);
 

@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
      hidp_del_timer(session);
 
-     if (intr_sk->sk_state != BT_CONNECTED) {
-             init_waitqueue_entry(&ctrl_wait, current);
-             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-                     set_current_state(TASK_INTERRUPTIBLE);
-                     timeo = schedule_timeout(timeo);
-             }
-             set_current_state(TASK_RUNNING);
-             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-             timeo = HZ;
Quoted 8 lines

net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 static int hidp_session(void *arg) · line 375

Quoted 4 lines
net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 renamed from net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800 +3 -1
-     }
+     if (intr_sk->sk_state != BT_CONNECTED)
+             wait_event_timeout(*(ctrl_sk->sk_sleep),
+                             (ctrl_sk->sk_state == BT_CLOSED), HZ);

and this does not make it look better. For me this is too much pointer stuff here. I think the interface of wait_event_timeout() is wrong. It should take a pointer to wait_queue_head_t.

Maybe; I'm not the designer of the interface, though, so I was just trying to use what exists. To be honest, this particular driver is the *only* one of a good number I've tried to convert that has this issue. Everywhere else, {add,remove}_wait_queue() is called like so:

{add,remove}_wait_queue(&wq, &wait);

just as it is called via prepare_to_wait() in wait_event(). core.c is the first driver I've come across that called the functions via:

{add,remove}_wait_queue(wq, &wait);

wait_event() is just a large macro which expands to the first form, given wq as a parameter, so I think it should be ok, as that form is the far more common one in the kernel currently.

What do you think? I am open to other ideas/suggestions, but I'm not certain changing an existing / working interface (which other drivers are being converted to) is the best idea.

Thanks,
Nish

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

maintainer
#5 lore raw

Hi Nish,

Quoted 56 lines
Quoted 40 lines
Quoted 40 lines

Description: Use wait_event_timeout() instead of custom wait-queue code. The current code uses TASK_INTERRUPTIBLE but only cares about timing out and the wq event taking place (does not actively do anything in response to signals), so wait_event_timeout() should be ok.

Signed-off-by Nishanth Aravamudan <[email protected]>
net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 renamed from net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800 +1 -11
@@ -36,6 +36,7 @@
 #include <linux/ioctl.h>
 #include <linux/file.h>
 #include <linux/init.h>
+#include <linux/wait.h>
 #include <net/sock.h>
 
 #include <linux/input.h>

@@ -325,7 +326,6 @@ static int hidp_session(void *arg)
      struct sk_buff *skb;
      int vendor = 0x0000, product = 0x0000;
      wait_queue_t ctrl_wait, intr_wait;
-     unsigned long timeo = HZ;
 
      BT_DBG("session %p", session);
 

@@ -370,28 +370,14 @@ static int hidp_session(void *arg)
 
      hidp_del_timer(session);
 
-     if (intr_sk->sk_state != BT_CONNECTED) {
-             init_waitqueue_entry(&ctrl_wait, current);
-             add_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-             while (timeo && ctrl_sk->sk_state != BT_CLOSED) {
-                     set_current_state(TASK_INTERRUPTIBLE);
-                     timeo = schedule_timeout(timeo);
-             }
-             set_current_state(TASK_RUNNING);
-             remove_wait_queue(ctrl_sk->sk_sleep, &ctrl_wait);
-             timeo = HZ;
Quoted 8 lines

net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 static int hidp_session(void *arg) · line 375

Quoted 4 lines
net/bluetooth/hidp/core.c 2005-01-21 11:07:11.000000000 -0800 renamed from net/bluetooth/hidp/core.c 2005-01-15 16:55:44.000000000 -0800 +3 -1
-     }
+     if (intr_sk->sk_state != BT_CONNECTED)
+             wait_event_timeout(*(ctrl_sk->sk_sleep),
+                             (ctrl_sk->sk_state == BT_CLOSED), HZ);

and this does not make it look better. For me this is too much pointer stuff here. I think the interface of wait_event_timeout() is wrong. It should take a pointer to wait_queue_head_t.

Maybe; I'm not the designer of the interface, though, so I was just trying to use what exists. To be honest, this particular driver is the *only* one of a good number I've tried to convert that has this issue. Everywhere else, {add,remove}_wait_queue() is called like so:

{add,remove}_wait_queue(&wq, &wait);

Quoted 8 lines

just as it is called via prepare_to_wait() in wait_event(). core.c is the first driver I've come across that called the functions via:

{add,remove}_wait_queue(wq, &wait);

wait_event() is just a large macro which expands to the first form, given wq as a parameter, so I think it should be ok, as that form is the far more common one in the kernel currently.

we are using the wait queue from struct sock and sk_sleep is already a pointer. Converting from and to a pointer and later to the struct again to only match an API looks wrong to me and thus this API is wrong, but this is only my thought.

Dave, any comments from you about this?

Regards

Marcel

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

#6 lore raw
Quoted 6 lines · On Sun, 23 Jan 2005 10:21:08 +0100 Marcel Holtmann <[email protected]> wrote:

we are using the wait queue from struct sock and sk_sleep is already a pointer. Converting from and to a pointer and later to the struct again to only match an API looks wrong to me and thus this API is wrong, but this is only my thought.

Dave, any comments from you about this?

Not really. I think the API is ok, it's a tradeoff between one set of call types putting in a deref compared to the other. And as the patch author stated, the bluetooth side is definitely in the minority.

To be honest, I think the decision is arbitrary.

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors