[PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Hoang-Nam Nguyen]

Hi,
this patch removes yield() and uses wait_for_completion() in order
to wait for running completion handlers finished before destroying
associated completion queue.
Thanks
Nam


Signed-off-by: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
---


 ehca_classes.h |    3 +++
 ehca_cq.c      |    3 ++-
 ehca_irq.c     |    6 +++++-
 3 files changed, 10 insertions(+), 2 deletions(-)


diff -Nurp infiniband_orig/drivers/infiniband/hw/ehca/ehca_classes.h infiniband_work/drivers/infiniband/hw/ehca/ehca_classes.h
--- infiniband_orig/drivers/infiniband/hw/ehca/ehca_classes.h   2007-02-14 13:52:49.000000000 +0100
+++ infiniband_work/drivers/infiniband/hw/ehca/ehca_classes.h   2007-02-14 13:52:06.000000000 +0100
@@ -52,6 +52,8 @@ struct ehca_mw;
 struct ehca_pd;
 struct ehca_av;

+#include <linux/completion.h>
+
 #include <rdma/ib_verbs.h>
 #include <rdma/ib_user_verbs.h>

@@ -154,6 +156,7 @@ struct ehca_cq {
        struct hlist_head qp_hashtab[QP_HASHTAB_LEN];
        struct list_head entry;
        u32 nr_callbacks;
+       struct completion zero_callbacks;
        spinlock_t task_lock;
        u32 ownpid;
        /* mmap counter for resources mapped into user space */
diff -Nurp infiniband_orig/drivers/infiniband/hw/ehca/ehca_cq.c infiniband_work/drivers/infiniband/hw/ehca/ehca_cq.c
--- infiniband_orig/drivers/infiniband/hw/ehca/ehca_cq.c        2007-02-14 13:52:49.000000000 +0100
+++ infiniband_work/drivers/infiniband/hw/ehca/ehca_cq.c        2007-02-14 13:52:06.000000000 +0100
@@ -147,6 +147,7 @@ struct ib_cq *ehca_create_cq(struct ib_d
        spin_lock_init(&my_cq->spinlock);
        spin_lock_init(&my_cq->cb_lock);
        spin_lock_init(&my_cq->task_lock);
+       init_completion(&my_cq->zero_callbacks);
        my_cq->ownpid = current->tgid;

        cq = &my_cq->ib_cq;
@@ -332,7 +333,7 @@ int ehca_destroy_cq(struct ib_cq *cq)
        spin_lock_irqsave(&ehca_cq_idr_lock, flags);
        while (my_cq->nr_callbacks) {
                spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
-               yield();
+               wait_for_completion(&my_cq->zero_callbacks);
                spin_lock_irqsave(&ehca_cq_idr_lock, flags);
        }

diff -Nurp infiniband_orig/drivers/infiniband/hw/ehca/ehca_irq.c infiniband_work/drivers/infiniband/hw/ehca/ehca_irq.c
--- infiniband_orig/drivers/infiniband/hw/ehca/ehca_irq.c       2007-02-14 13:52:49.000000000 +0100
+++ infiniband_work/drivers/infiniband/hw/ehca/ehca_irq.c       2007-02-14 13:52:06.000000000 +0100
@@ -605,6 +605,7 @@ static void run_comp_task(struct ehca_cp
        spin_lock_irqsave(&cct->task_lock, flags);

        while (!list_empty(&cct->cq_list)) {
+               int is_complete = 0;
                cq = list_entry(cct->cq_list.next, struct ehca_cq, entry);
                spin_unlock_irqrestore(&cct->task_lock, flags);
                comp_event_callback(cq);
@@ -612,11 +613,14 @@ static void run_comp_task(struct ehca_cp

                spin_lock(&cq->task_lock);
                cq->nr_callbacks--;
-               if (cq->nr_callbacks == 0) {
+               is_complete = (cq->nr_callbacks == 0);
+               if (is_complete) {
                        list_del_init(cct->cq_list.next);
                        cct->cq_jobs--;
                }
                spin_unlock(&cq->task_lock);
+               if (is_complete) /* wake up waiting destroy_cq() */
+                       complete(&cq->zero_callbacks);
        }

        spin_unlock_irqrestore(&cct->task_lock, flags);

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Christoph Hellwig]

> @@ -332,7 +333,7 @@ int ehca_destroy_cq(struct ib_cq *cq)
>         spin_lock_irqsave(&ehca_cq_idr_lock, flags);
>         while (my_cq->nr_callbacks) {
>                 spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
> -               yield();
> +               wait_for_completion(&my_cq->zero_callbacks);
>                 spin_lock_irqsave(&ehca_cq_idr_lock, flags);
>         }

A while loop around wait_for_completion doesn't make all that much sense.
I suspect a simple

	if (my_cq->nr_callbacks)
		wait_for_completion(&my_cq->zero_callbacks);

Is what you need.

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Christoph Hellwig]

> @@ -332,7 +333,7 @@ int ehca_destroy_cq(struct ib_cq *cq)
>         spin_lock_irqsave(&ehca_cq_idr_lock, flags);
>         while (my_cq->nr_callbacks) {
>                 spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
> -               yield();
> +               wait_for_completion(&my_cq->zero_callbacks);
>                 spin_lock_irqsave(&ehca_cq_idr_lock, flags);
>         }

A while loop around wait_for_completion doesn't make all that much sense.
I suspect a simple

	if (my_cq->nr_callbacks)
		wait_for_completion(&my_cq->zero_callbacks);

Is what you need.

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Roland Dreier]

I agree with Christoph -- the use of wait_for_completion() in a loop
makes no sense.  When you send a new copy of this patch without
whitespace damage, please fix that up too...

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Roland Dreier]

I agree with Christoph -- the use of wait_for_completion() in a loop
makes no sense.  When you send a new copy of this patch without
whitespace damage, please fix that up too...

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Hoang-Nam Nguyen]

Hi,
> So I'm holding off on applying this for now.  Please think it over and
> either tell me the current patch is OK, or fix it up.  There's not
> really too much urgency because a change like this is something I
> would be comfortable merging between 2.6.21-rc1 and -rc2.
You're absolutely right. Let's target for rc2.
Thanks for this good catch!
Nam

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Hoang-Nam Nguyen]

Hi,
> So I'm holding off on applying this for now.  Please think it over and
> either tell me the current patch is OK, or fix it up.  There's not
> really too much urgency because a change like this is something I
> would be comfortable merging between 2.6.21-rc1 and -rc2.
You're absolutely right. Let's target for rc2.
Thanks for this good catch!
Nam

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Roland Dreier]

Looking at this one more time, I think it actually may be buggy:

 > @@ -147,6 +147,7 @@ struct ib_cq *ehca_create_cq(struct ib_d
 >  	spin_lock_init(&my_cq->spinlock);
 >  	spin_lock_init(&my_cq->cb_lock);
 >  	spin_lock_init(&my_cq->task_lock);
 > +	init_completion(&my_cq->zero_callbacks);

So you initialize the zero_callbacks completion once, at
ehca_create_cq().

But then 

 > @@ -612,11 +613,14 @@ static void run_comp_task(struct ehca_cp
 >  
 >  		spin_lock(&cq->task_lock);
 >  		cq->nr_callbacks--;
 > -		if (cq->nr_callbacks == 0) {
 > +		is_complete = (cq->nr_callbacks == 0);
 > +		if (is_complete) {
 >  			list_del_init(cct->cq_list.next);
 >  			cct->cq_jobs--;
 >  		}
 >  		spin_unlock(&cq->task_lock);
 > +		if (is_complete) /* wake up waiting destroy_cq() */
 > +			complete(&cq->zero_callbacks);
 >  	}

every time nr_callbacks drops to 0, you complete the zero_callbacks
completion.  So the first time a callback runs, you will complete
zero_callbacks, which will let wait_for_completion() finish even if
you later increment nr_callbacks again.

Also this

 > -	while (my_cq->nr_callbacks) {
 > +	if (my_cq->nr_callbacks) {
 >  		spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
 > -		yield();
 > +		wait_for_completion(&my_cq->zero_callbacks);
 >  		spin_lock_irqsave(&ehca_cq_idr_lock, flags);
 >  	}

looks rather unsafe -- I don't see any common locking protecting both
this test of nr_callbacks and the setting of nr_callbacks in the ehca
irq handling... so I don't see anything protecting you from seeing
nr_callbacks==0 and not going into the if() (or while() -- the old
code has the same problem I think) but then doing ++nr_callbacks
somewhere else.  In fact since you do the idr_remove() and
hipz_h_destroy_cq() *after* you make sure no callbacks are running,
this seems like it could happen easily.

So I'm holding off on applying this for now.  Please think it over and
either tell me the current patch is OK, or fix it up.  There's not
really too much urgency because a change like this is something I
would be comfortable merging between 2.6.21-rc1 and -rc2.

 - R.

Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Roland Dreier]

Looking at this one more time, I think it actually may be buggy:

 > @@ -147,6 +147,7 @@ struct ib_cq *ehca_create_cq(struct ib_d
 >  	spin_lock_init(&my_cq->spinlock);
 >  	spin_lock_init(&my_cq->cb_lock);
 >  	spin_lock_init(&my_cq->task_lock);
 > +	init_completion(&my_cq->zero_callbacks);

So you initialize the zero_callbacks completion once, at
ehca_create_cq().

But then 

 > @@ -612,11 +613,14 @@ static void run_comp_task(struct ehca_cp
 >  
 >  		spin_lock(&cq->task_lock);
 >  		cq->nr_callbacks--;
 > -		if (cq->nr_callbacks == 0) {
 > +		is_complete = (cq->nr_callbacks == 0);
 > +		if (is_complete) {
 >  			list_del_init(cct->cq_list.next);
 >  			cct->cq_jobs--;
 >  		}
 >  		spin_unlock(&cq->task_lock);
 > +		if (is_complete) /* wake up waiting destroy_cq() */
 > +			complete(&cq->zero_callbacks);
 >  	}

every time nr_callbacks drops to 0, you complete the zero_callbacks
completion.  So the first time a callback runs, you will complete
zero_callbacks, which will let wait_for_completion() finish even if
you later increment nr_callbacks again.

Also this

 > -	while (my_cq->nr_callbacks) {
 > +	if (my_cq->nr_callbacks) {
 >  		spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
 > -		yield();
 > +		wait_for_completion(&my_cq->zero_callbacks);
 >  		spin_lock_irqsave(&ehca_cq_idr_lock, flags);
 >  	}

looks rather unsafe -- I don't see any common locking protecting both
this test of nr_callbacks and the setting of nr_callbacks in the ehca
irq handling... so I don't see anything protecting you from seeing
nr_callbacks==0 and not going into the if() (or while() -- the old
code has the same problem I think) but then doing ++nr_callbacks
somewhere else.  In fact since you do the idr_remove() and
hipz_h_destroy_cq() *after* you make sure no callbacks are running,
this seems like it could happen easily.

So I'm holding off on applying this for now.  Please think it over and
either tell me the current patch is OK, or fix it up.  There's not
really too much urgency because a change like this is something I
would be comfortable merging between 2.6.21-rc1 and -rc2.

 - R.

[PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()

[Hoang-Nam Nguyen]

remove yield() and use wait_for_completion() in order to wait for running
completion handlers finished before destroying associated completion queue


Signed-off-by: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
---


 ehca_classes.h |    3 +++
 ehca_cq.c      |    5 +++--
 ehca_irq.c     |    6 +++++-
 3 files changed, 11 insertions(+), 3 deletions(-)


diff --git a/drivers/infiniband/hw/ehca/ehca_classes.h b/drivers/infiniband/hw/ehca/ehca_classes.h
index 40404c9..d8ce0c8 100644
--- a/drivers/infiniband/hw/ehca/ehca_classes.h
+++ b/drivers/infiniband/hw/ehca/ehca_classes.h
@@ -52,6 +52,8 @@ struct ehca_mw;
 struct ehca_pd;
 struct ehca_av;
 
+#include <linux/completion.h>
+
 #include <rdma/ib_verbs.h>
 #include <rdma/ib_user_verbs.h>
 
@@ -154,6 +156,7 @@ struct ehca_cq {
 	struct hlist_head qp_hashtab[QP_HASHTAB_LEN];
 	struct list_head entry;
 	u32 nr_callbacks;
+	struct completion zero_callbacks;
 	spinlock_t task_lock;
 	u32 ownpid;
 	/* mmap counter for resources mapped into user space */
diff --git a/drivers/infiniband/hw/ehca/ehca_cq.c b/drivers/infiniband/hw/ehca/ehca_cq.c
index 9291a86..906bd5b 100644
--- a/drivers/infiniband/hw/ehca/ehca_cq.c
+++ b/drivers/infiniband/hw/ehca/ehca_cq.c
@@ -147,6 +147,7 @@ struct ib_cq *ehca_create_cq(struct ib_d
 	spin_lock_init(&my_cq->spinlock);
 	spin_lock_init(&my_cq->cb_lock);
 	spin_lock_init(&my_cq->task_lock);
+	init_completion(&my_cq->zero_callbacks);
 	my_cq->ownpid = current->tgid;
 
 	cq = &my_cq->ib_cq;
@@ -330,9 +331,9 @@ int ehca_destroy_cq(struct ib_cq *cq)
 	}
 
 	spin_lock_irqsave(&ehca_cq_idr_lock, flags);
-	while (my_cq->nr_callbacks) {
+	if (my_cq->nr_callbacks) {
 		spin_unlock_irqrestore(&ehca_cq_idr_lock, flags);
-		yield();
+		wait_for_completion(&my_cq->zero_callbacks);
 		spin_lock_irqsave(&ehca_cq_idr_lock, flags);
 	}
 
diff --git a/drivers/infiniband/hw/ehca/ehca_irq.c b/drivers/infiniband/hw/ehca/ehca_irq.c
index 3ec53c6..7db39b7 100644
--- a/drivers/infiniband/hw/ehca/ehca_irq.c
+++ b/drivers/infiniband/hw/ehca/ehca_irq.c
@@ -605,6 +605,7 @@ static void run_comp_task(struct ehca_cp
 	spin_lock_irqsave(&cct->task_lock, flags);
 
 	while (!list_empty(&cct->cq_list)) {
+		int is_complete = 0;
 		cq = list_entry(cct->cq_list.next, struct ehca_cq, entry);
 		spin_unlock_irqrestore(&cct->task_lock, flags);
 		comp_event_callback(cq);
@@ -612,11 +613,14 @@ static void run_comp_task(struct ehca_cp
 
 		spin_lock(&cq->task_lock);
 		cq->nr_callbacks--;
-		if (cq->nr_callbacks == 0) {
+		is_complete = (cq->nr_callbacks == 0);
+		if (is_complete) {
 			list_del_init(cct->cq_list.next);
 			cct->cq_jobs--;
 		}
 		spin_unlock(&cq->task_lock);
+		if (is_complete) /* wake up waiting destroy_cq() */
+			complete(&cq->zero_callbacks);
 	}
 
 	spin_unlock_irqrestore(&cct->task_lock, flags);