Skip to contentSkip to conversation

drivers/dmapool: use TASK_UNINTERRUPTIBLE instead of TASK_INTERRUPTIBLE

PATCH In review [email protected] 3 participants 4 messages Mar 6, 2005 – Mar 7, 2005 Driver core

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 12/14 drivers/dmapool: use TASK_UNINTERRUPTIBLE instead of TASK_INTERRUPTIBLE

use TASK_UNINTERRUPTIBLE instead of TASK_INTERRUPTIBLE

Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>

---

 kj-domen/drivers/base/dmapool.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)
drivers/base/dmapool.c renamed from drivers/base/dmapool.c~task_unint-drivers_base_dmapool +1 -1
diff -puN drivers/base/dmapool.c~task_unint-drivers_base_dmapool drivers/base/dmapool.c
@@ -293,7 +293,7 @@ restart:
 		if (mem_flags & __GFP_WAIT) {
 			DECLARE_WAITQUEUE (wait, current);
 
-			current->state = TASK_INTERRUPTIBLE;
+			set_current_state(TASK_UNINTERRUPTIBLE);
 			add_wait_queue (&pool->waitq, &wait);
 			spin_unlock_irqrestore (&pool->lock, flags);

_

#1 lore raw
Quoted 18 lines · [email protected] wrote:

use TASK_UNINTERRUPTIBLE instead of TASK_INTERRUPTIBLE

Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>

---

  kj-domen/drivers/base/dmapool.c |    2 +-
  1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/base/dmapool.c~task_unint-drivers_base_dmapool drivers/base/dmapool.c
--- kj/drivers/base/dmapool.c~task_unint-drivers_base_dmapool 2005-03-05 16:11:21.000000000 +0100
+++ kj-domen/drivers/base/dmapool.c 2005-03-05 16:11:21.000000000 +0100
@@ -293,7 +293,7 @@ restart:
if (mem_flags & __GFP_WAIT) {
DECLARE_WAITQUEUE (wait, current);

Quoted 4 lines

- current->state = TASK_INTERRUPTIBLE;
+ set_current_state(TASK_UNINTERRUPTIBLE);
add_wait_queue (&pool->waitq, &wait); spin_unlock_irqrestore (&pool->lock, flags);

This code is alread a bit odd. If we're prepared to sleep in there, then why use GFP_ATOMIC?

If it is so that we can dig a bit deeper into the free page pools then something like __GFP_WAIT|__GFP_HIGH would be preferable.

And why isn't mem_flags passed into pool_alloc_page() verbatim?

I agree on the TASK_UNINTERRUPTIBLE change: if the calling task happens to have signal_pending() then the schedule_timeout() will fall right through. Why should we change kernel memory allocation strategy if the user hit ^C?

Also, __set_current_state() can be user here: the add_wait_queue() contains the necessary barriers. (Grubby, but we do that in quite a few places with this particular code sequence (we should have an add_wait_queue() variant which does the add_wait_queue+__set_current_state all in one hit (but let's not, else I'll be buried in another 1000 cleanuplets))).

#2 lore raw
Quoted 24 lines · Andrew Morton
Quoted 15 lines · [email protected] wrote:

use TASK_UNINTERRUPTIBLE instead of TASK_INTERRUPTIBLE

Signed-off-by Nishanth Aravamudan <[email protected]>
Signed-off-by Domen Puncer <[email protected]>

---

  kj-domen/drivers/base/dmapool.c |    2 +-
  1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN drivers/base/dmapool.c~task_unint-drivers_base_dmapool drivers/base/dmapool.c
--- kj/drivers/base/dmapool.c~task_unint-drivers_base_dmapool 2005-03-05 16:11:21.000000000 +0100
+++ kj-domen/drivers/base/dmapool.c 2005-03-05 16:11:21.000000000 +0100 @@ -293,7 +293,7 @@ restart:

Quoted 7 lines
              if (mem_flags & __GFP_WAIT) {
                      DECLARE_WAITQUEUE (wait, current);

- current->state = TASK_INTERRUPTIBLE;

 +                    set_current_state(TASK_UNINTERRUPTIBLE);
                      add_wait_queue (&pool->waitq, &wait);
                      spin_unlock_irqrestore (&pool->lock, flags);
Quoted 7 lines

This code is alread a bit odd. If we're prepared to sleep in there, then why use GFP_ATOMIC?

If it is so that we can dig a bit deeper into the free page pools then something like __GFP_WAIT|__GFP_HIGH would be preferable.

And why isn't mem_flags passed into pool_alloc_page() verbatim?

Sorry, far beyond my abilities :(

Quoted 3 lines

I agree on the TASK_UNINTERRUPTIBLE change: if the calling task happens to have signal_pending() then the schedule_timeout() will fall right through. Why should we change kernel memory allocation strategy if the user hit ^C?

Yup, didn't make much sense to me.

Quoted 5 lines

Also, __set_current_state() can be user here: the add_wait_queue() contains the necessary barriers. (Grubby, but we do that in quite a few places with this particular code sequence (we should have an add_wait_queue() variant which does the add_wait_queue+__set_current_state all in one hit (but let's not, else I'll be buried in another 1000 cleanuplets))).

Ok, I will re-spin this patch. Or would you prefer an incremental one?

Thanks,
Nish

#3 lore raw
Quoted 8 lines · Nish Aravamudan
Quoted 6 lines

Also, __set_current_state() can be user here: the add_wait_queue() contains the necessary barriers. (Grubby, but we do that in quite a few places with this particular code sequence (we should have an add_wait_queue() variant which does the add_wait_queue+__set_current_state all in one hit (but let's not, else I'll be buried in another 1000 cleanuplets))).

Ok, I will re-spin this patch. Or would you prefer an incremental one?

Let's forget about this one while we work out whether that code is doing what we want it to do.