[PATCH] macvtap: use set_curren_state to ensure mb

Subsystems: networking drivers, the rest

STALE5192d

4 messages, 3 authors, 2012-06-06 · open the first message on its own page

[PATCH] macvtap: use set_curren_state to ensure mb

From: Hong zhi guo <hidden>
Date: 2012-06-05 10:46:37

replace raw assignment of current->state with
set_current_state(TASK_...) to ensure memory barrier.

Signed-off-by: Hong Zhiguo <redacted>
---
 drivers/net/macvtap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 2ee56de..62754f6 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -853,7 +853,7 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,

 	add_wait_queue(sk_sleep(&q->sk), &wait);
 	while (len) {
-		current->state = TASK_INTERRUPTIBLE;
+		set_current_state(TASK_INTERRUPTIBLE);

 		/* Read frames from the queue */
 		skb = skb_dequeue(&q->sk.sk_receive_queue);
@@ -875,7 +875,7 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,
 		break;
 	}

-	current->state = TASK_RUNNING;
+	set_current_state(TASK_RUNNING);
 	remove_wait_queue(sk_sleep(&q->sk), &wait);
 	return ret;
 }
-- 
1.7.4.1

Re: [PATCH] macvtap: use set_curren_state to ensure mb

From: Eric Dumazet <hidden>
Date: 2012-06-05 11:05:34

On Tue, 2012-06-05 at 18:46 +0800, Hong zhi guo wrote:
quoted hunk
replace raw assignment of current->state with
set_current_state(TASK_...) to ensure memory barrier.

Signed-off-by: Hong Zhiguo <redacted>
---
 drivers/net/macvtap.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 2ee56de..62754f6 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -853,7 +853,7 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,

 	add_wait_queue(sk_sleep(&q->sk), &wait);
 	while (len) {
-		current->state = TASK_INTERRUPTIBLE;
+		set_current_state(TASK_INTERRUPTIBLE);

 		/* Read frames from the queue */
 		skb = skb_dequeue(&q->sk.sk_receive_queue);
@@ -875,7 +875,7 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,
 		break;
 	}

-	current->state = TASK_RUNNING;
+	set_current_state(TASK_RUNNING);
 	remove_wait_queue(sk_sleep(&q->sk), &wait);
 	return ret;
 }

Why not using the more standard :

prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);

and

finish_wait(sk_sleep(sk), &wait);

Re: [PATCH] macvtap: use set_curren_state to ensure mb

From: Hong zhi guo <hidden>
Date: 2012-06-05 12:05:35

On Tue, Jun 5, 2012 at 7:05 PM, Eric Dumazet [off-list ref] wrote:
Why not using the more standard :

prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);

and

finish_wait(sk_sleep(sk), &wait);
Thanks for your comments.  The difference is that prepare_to_wait
inside loop introduces extra "list_empty" judgement than original
patch. But personally I prefer the newer version too:

Signed-off-by: Hong Zhiguo <redacted>
---
 drivers/net/macvtap.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 2ee56de..0737bd4 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -847,13 +847,12 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,
 			       const struct iovec *iv, unsigned long len,
 			       int noblock)
 {
-	DECLARE_WAITQUEUE(wait, current);
+	DEFINE_WAIT(wait);
 	struct sk_buff *skb;
 	ssize_t ret = 0;

-	add_wait_queue(sk_sleep(&q->sk), &wait);
 	while (len) {
-		current->state = TASK_INTERRUPTIBLE;
+		prepare_to_wait(sk_sleep(&q->sk), &wait, TASK_INTERRUPTIBLE);

 		/* Read frames from the queue */
 		skb = skb_dequeue(&q->sk.sk_receive_queue);
@@ -875,8 +874,7 @@ static ssize_t macvtap_do_read(struct
macvtap_queue *q, struct kiocb *iocb,
 		break;
 	}

-	current->state = TASK_RUNNING;
-	remove_wait_queue(sk_sleep(&q->sk), &wait);
+	finish_wait(sk_sleep(&q->sk), &wait);
 	return ret;
 }

-- 
1.7.4.1

Re: [PATCH] macvtap: use set_curren_state to ensure mb

From: David Miller <davem@davemloft.net>
Date: 2012-06-06 17:48:20

From: Hong zhi guo <redacted>
Date: Tue, 5 Jun 2012 20:05:34 +0800
On Tue, Jun 5, 2012 at 7:05 PM, Eric Dumazet [off-list ref] wrote:
quoted
Why not using the more standard :

prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);

and

finish_wait(sk_sleep(sk), &wait);
Thanks for your comments.  The difference is that prepare_to_wait
inside loop introduces extra "list_empty" judgement than original
patch. But personally I prefer the newer version too:

Signed-off-by: Hong Zhiguo <redacted>
Please don't submit patches like this:

1) When submitting new versions of a patch, make a fresh
   new mailing list posting with just the patch and it's
   commit message.

   Do not post new versions by replying to other conversations.

2) Your email client has severely corrupted the patch, breaking
   up long lines etc.  Please read Documentation/email-clients.txt
   to learn how to fix this.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help