LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Roland Dreier <rdreier@cisco.com>
To: Hoang-Nam Nguyen <hnguyen@linux.vnet.ibm.com>
Cc: linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org,
	openib-general@openib.org, hch@infradead.org, raisch@de.ibm.com,
	bos@patchscale.com, heiko.carstens@de.ibm.com
Subject: Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()
Date: Thu, 15 Feb 2007 09:57:48 -0800	[thread overview]
Message-ID: <adazm7fs3qb.fsf@cisco.com> (raw)
In-Reply-To: <200702151709.45323.hnguyen@linux.vnet.ibm.com> (Hoang-Nam Nguyen's message of "Thu, 15 Feb 2007 17:09:44 +0100")

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.

  reply	other threads:[~2007-02-15 17:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-15 16:09 Hoang-Nam Nguyen
2007-02-15 17:57 ` Roland Dreier [this message]
2007-02-15 19:54   ` Hoang-Nam Nguyen
  -- strict thread matches above, loose matches on Subject: below --
2007-02-14 16:41 Hoang-Nam Nguyen
2007-02-14 22:28 ` Christoph Hellwig
2007-02-15  0:50 ` Roland Dreier

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=adazm7fs3qb.fsf@cisco.com \
    --to=rdreier@cisco.com \
    --cc=bos@patchscale.com \
    --cc=hch@infradead.org \
    --cc=heiko.carstens@de.ibm.com \
    --cc=hnguyen@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=openib-general@openib.org \
    --cc=raisch@de.ibm.com \
    --subject='Re: [PATCH 2.6.21-rc1 4/5] ehca: replace yield() by wait_for_completion()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).