[PATCH] mthca: initialize send and receive queue locks separately

[PATCH] mthca: initialize send and receive queue locks separately

[Zach Brown]

mthca: initialize send and receive queue locks separately

lockdep identifies a lock by the call site of its initialization.  By
initializing the send and receive queue locks in mthca_wq_init() we confuse
lockdep.  It warns that that the ordered acquiry of both locks in
mthca_modify_qp() is recursive acquiry of one lock:

=============================================
[ INFO: possible recursive locking detected ]
---------------------------------------------
modprobe/1192 is trying to acquire lock:
 (&wq->lock){....}, at: [<f892b4db>] mthca_modify_qp+0x60/0xa7b [ib_mthca]
but task is already holding lock:
 (&wq->lock){....}, at: [<f892b4ce>] mthca_modify_qp+0x53/0xa7b [ib_mthca]

Initializing the locks separately in mthca_alloc_qp_common() stops the warning
and will let lockdep enforce proper ordering on paths that acquire both locks.

Signed-off-by: Zach Brown <zach.brown@oracle.com>
---

 drivers/infiniband/hw/mthca/mthca_qp.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c
===================================================================
--- 2.6.17-mm6.orig/drivers/infiniband/hw/mthca/mthca_qp.c	2006-07-03 08:41:16.000000000 -0400
+++ 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c	2006-07-03 10:05:52.000000000 -0400
@@ -224,7 +224,7 @@
 
 static void mthca_wq_init(struct mthca_wq *wq)
 {
-	spin_lock_init(&wq->lock);
+	/* mthca_alloc_qp_common() initializes the locks */
 	wq->next_ind  = 0;
 	wq->last_comp = wq->max - 1;
 	wq->head      = 0;
@@ -1114,6 +1114,9 @@
 	qp->sq_policy    = send_policy;
 	mthca_wq_init(&qp->sq);
 	mthca_wq_init(&qp->rq);
+	/* these are initialized separately so lockdep can tell them apart */
+	spin_lock_init(&qp->sq.lock);
+	spin_lock_init(&qp->rq.lock);
 
 	ret = mthca_map_memfree(dev, qp);
 	if (ret)

Re: [PATCH] mthca: initialize send and receive queue locks separately

[Michael S. Tsirkin]

Quoting r. Zach Brown <zach.brown@oracle.com>:
> Subject: [PATCH] mthca: initialize send and receive queue locks separately
> 
> mthca: initialize send and receive queue locks separately
> 
> lockdep identifies a lock by the call site of its initialization.  By
> initializing the send and receive queue locks in mthca_wq_init() we confuse
> lockdep.  It warns that that the ordered acquiry of both locks in
> mthca_modify_qp() is recursive acquiry of one lock:
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> ---------------------------------------------
> modprobe/1192 is trying to acquire lock:
>  (&wq->lock){....}, at: [<f892b4db>] mthca_modify_qp+0x60/0xa7b [ib_mthca]
> but task is already holding lock:
>  (&wq->lock){....}, at: [<f892b4ce>] mthca_modify_qp+0x53/0xa7b [ib_mthca]

Is this mthca code unique?
Would not it be better to teach lockdep about this scenario somehow?

> Initializing the locks separately in mthca_alloc_qp_common() stops the warning
> and will let lockdep enforce proper ordering on paths that acquire both locks.
> 
> Signed-off-by: Zach Brown <zach.brown@oracle.com>

This moves code out of a common function and so results in code duplication
and has memory cost.

> ---
> 
>  drivers/infiniband/hw/mthca/mthca_qp.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> Index: 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c
> ===================================================================
> --- 2.6.17-mm6.orig/drivers/infiniband/hw/mthca/mthca_qp.c	2006-07-03 08:41:16.000000000 -0400
> +++ 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c	2006-07-03 10:05:52.000000000 -0400
> @@ -224,7 +224,7 @@
>  
>  static void mthca_wq_init(struct mthca_wq *wq)
>  {
> -	spin_lock_init(&wq->lock);
> +	/* mthca_alloc_qp_common() initializes the locks */
>  	wq->next_ind  = 0;
>  	wq->last_comp = wq->max - 1;
>  	wq->head      = 0;

And then we'll have to remember to update this comment when lock
is moved to another place?

> @@ -1114,6 +1114,9 @@
>  	qp->sq_policy    = send_policy;
>  	mthca_wq_init(&qp->sq);
>  	mthca_wq_init(&qp->rq);
> +	/* these are initialized separately so lockdep can tell them apart */
> +	spin_lock_init(&qp->sq.lock);
> +	spin_lock_init(&qp->rq.lock);
>  
>  	ret = mthca_map_memfree(dev, qp);
>  	if (ret)
> 

Looks wrong, to me. Is it a good idea to fix correct code?
Assuming its important, can we maybe add some annotations to make lockdep shut
up, instead?

-- 
MST

Re: [PATCH] mthca: initialize send and receive queue locks separately

[Ingo Molnar]

* Michael S. Tsirkin <mst@mellanox.co.il> wrote:

> > Initializing the locks separately in mthca_alloc_qp_common() stops the warning
> > and will let lockdep enforce proper ordering on paths that acquire both locks.
> > 
> > Signed-off-by: Zach Brown <zach.brown@oracle.com>
> 
> This moves code out of a common function and so results in code 
> duplication and has memory cost.

the patch below does the same via the lockdep_set_class() method, which 
has no cost on non-lockdep kernels.

	Ingo

---------------->
Subject: lockdep: annotate drivers/infiniband/hw/mthca/mthca_qp.c
From: Ingo Molnar <mingo@elte.hu>

annotate mthca queue locks: split them into send and receive locks.

(both can be held at once, but there is ordering between them which
lockdep enforces)

Has no effect on non-lockdep kernels.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/infiniband/hw/mthca/mthca_qp.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Index: linux/drivers/infiniband/hw/mthca/mthca_qp.c
===================================================================
--- linux.orig/drivers/infiniband/hw/mthca/mthca_qp.c
+++ linux/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -222,9 +222,15 @@ static void *get_send_wqe(struct mthca_q
 			 (PAGE_SIZE - 1));
 }
 
-static void mthca_wq_init(struct mthca_wq *wq)
+/*
+ * Send and receive queues for two different lock classes:
+ */
+static struct lock_class_key mthca_wq_send_lock_class, mthca_wq_recv_lock_class;
+
+static void mthca_wq_init(struct mthca_wq *wq, struct lock_class_key *key)
 {
 	spin_lock_init(&wq->lock);
+	lockdep_set_class(&wq->lock, key);
 	wq->next_ind  = 0;
 	wq->last_comp = wq->max - 1;
 	wq->head      = 0;
@@ -845,10 +851,10 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
 				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 
-		mthca_wq_init(&qp->sq);
+		mthca_wq_init(&qp->sq, &mthca_wq_send_lock_class);
 		qp->sq.last = get_send_wqe(qp, qp->sq.max - 1);
 
-		mthca_wq_init(&qp->rq);
+		mthca_wq_init(&qp->rq, &mthca_wq_recv_lock_class);
 		qp->rq.last = get_recv_wqe(qp, qp->rq.max - 1);
 
 		if (mthca_is_memfree(dev)) {
@@ -1112,8 +1118,8 @@ static int mthca_alloc_qp_common(struct 
 	qp->atomic_rd_en = 0;
 	qp->resp_depth   = 0;
 	qp->sq_policy    = send_policy;
-	mthca_wq_init(&qp->sq);
-	mthca_wq_init(&qp->rq);
+	mthca_wq_init(&qp->sq, &mthca_wq_send_lock_class);
+	mthca_wq_init(&qp->rq, &mthca_wq_recv_lock_class);
 
 	ret = mthca_map_memfree(dev, qp);
 	if (ret)

Re: [PATCH] mthca: initialize send and receive queue locks separately

[Michael S. Tsirkin]

Quoting r. Ingo Molnar <mingo@elte.hu>:
> Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately
> 
> 
> * Michael S. Tsirkin <mst@mellanox.co.il> wrote:
> 
> > > Initializing the locks separately in mthca_alloc_qp_common() stops the warning
> > > and will let lockdep enforce proper ordering on paths that acquire both locks.
> > > 
> > > Signed-off-by: Zach Brown <zach.brown@oracle.com>
> > 
> > This moves code out of a common function and so results in code 
> > duplication and has memory cost.
> 
> the patch below does the same via the lockdep_set_class() method, which 
> has no cost on non-lockdep kernels.
> 
> 	Ingo
> 
> ---------------->
> Subject: lockdep: annotate drivers/infiniband/hw/mthca/mthca_qp.c
> From: Ingo Molnar <mingo@elte.hu>
> 
> annotate mthca queue locks: split them into send and receive locks.
> 
> (both can be held at once, but there is ordering between them which
> lockdep enforces)

I find this capability of lockdep very useful.

> Has no effect on non-lockdep kernels.

Hmm ... adding parameters to function still has text cost, I think. No?

> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  drivers/infiniband/hw/mthca/mthca_qp.c |   16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> Index: linux/drivers/infiniband/hw/mthca/mthca_qp.c
> ===================================================================
> --- linux.orig/drivers/infiniband/hw/mthca/mthca_qp.c
> +++ linux/drivers/infiniband/hw/mthca/mthca_qp.c
> @@ -222,9 +222,15 @@ static void *get_send_wqe(struct mthca_q
>  			 (PAGE_SIZE - 1));
>  }
>  
> -static void mthca_wq_init(struct mthca_wq *wq)
> +/*
> + * Send and receive queues for two different lock classes:
> + */
> +static struct lock_class_key mthca_wq_send_lock_class, mthca_wq_recv_lock_class;
> +

Does this still have a small cost in data size on non-lockdep kernels, as well?
If yes, maybe some typedef/macro magic can be used to put this struct in an
unused elf section for such kernels?

> +static void mthca_wq_init(struct mthca_wq *wq, struct lock_class_key *key)
>  {
>  	spin_lock_init(&wq->lock);
> +	lockdep_set_class(&wq->lock, key);
>  	wq->next_ind  = 0;
>  	wq->last_comp = wq->max - 1;
>  	wq->head      = 0;
> @@ -845,10 +851,10 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
>  			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
>  				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
>  
> -		mthca_wq_init(&qp->sq);
> +		mthca_wq_init(&qp->sq, &mthca_wq_send_lock_class);
>  		qp->sq.last = get_send_wqe(qp, qp->sq.max - 1);
>  
> -		mthca_wq_init(&qp->rq);
> +		mthca_wq_init(&qp->rq, &mthca_wq_recv_lock_class);
>  		qp->rq.last = get_recv_wqe(qp, qp->rq.max - 1);
>  
>  		if (mthca_is_memfree(dev)) {
> @@ -1112,8 +1118,8 @@ static int mthca_alloc_qp_common(struct 
>  	qp->atomic_rd_en = 0;
>  	qp->resp_depth   = 0;
>  	qp->sq_policy    = send_policy;
> -	mthca_wq_init(&qp->sq);
> -	mthca_wq_init(&qp->rq);
> +	mthca_wq_init(&qp->sq, &mthca_wq_send_lock_class);
> +	mthca_wq_init(&qp->rq, &mthca_wq_recv_lock_class);
>  
>  	ret = mthca_map_memfree(dev, qp);
>  	if (ret)
> 
> 

I'm pretty sure this still adds to code footprint due to extra function
parameters even on non-lockdep kernels. Will the following work?

@@ -1112,8 +1118,8 @@ static int mthca_alloc_qp_common(struct 
 	qp->atomic_rd_en = 0;
 	qp->resp_depth   = 0;
 	qp->sq_policy    = send_policy;
	mthca_wq_init(&qp->sq);
+	lockdep_set_class(&qp->sq.lock, &mthca_wq_send_lock_class);
	mthca_wq_init(&qp->rq);
+	lockdep_set_class(&qp->rq.lock, &mthca_wq_recv_lock_class);

-- 
MST

Re: [PATCH] mthca: initialize send and receive queue locks separately

[Ingo Molnar]

* Michael S. Tsirkin <mst@mellanox.co.il> wrote:

> > Has no effect on non-lockdep kernels.
> 
> Hmm ... adding parameters to function still has text cost, I think. No?

it shouldnt - it's a static function and the parameter is unused _and_ 
is of a type that is zero-size [on non-lockdep kernels] - gcc ought to 
be able to optimize it out.

	Ingo

Re: [PATCH] mthca: initialize send and receive queue locks separately

[Michael S. Tsirkin]

Quoting r. Ingo Molnar <mingo@elte.hu>:
> Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately
> 
> 
> * Michael S. Tsirkin <mst@mellanox.co.il> wrote:
> 
> > > Has no effect on non-lockdep kernels.
> > 
> > Hmm ... adding parameters to function still has text cost, I think. No?
> 
> it shouldnt - it's a static function and the parameter is unused _and_ 
> is of a type that is zero-size [on non-lockdep kernels] - gcc ought to 
> be able to optimize it out.

Ingo, you are right, and I just checked this with several gcc versions.
More power to you :)

-- 
MST

[PATCH] lockdep: don't pull in includes when lockdep disabled

[Michael S. Tsirkin]

Ingo, does the following look good to you?

Do not pull in various includes through lockdep.h if lockdep is disabled.

Signed-off-by: Michael S. Tsirkin <mst@mellanox.co.il>

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 316e0fb..39d50c4 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -8,13 +8,13 @@
 #ifndef __LINUX_LOCKDEP_H
 #define __LINUX_LOCKDEP_H
 
+#ifdef CONFIG_LOCKDEP
+
 #include <linux/linkage.h>
 #include <linux/list.h>
 #include <linux/debug_locks.h>
 #include <linux/stacktrace.h>
 
-#ifdef CONFIG_LOCKDEP
-
 /*
  * Lock-class usage-state bits:
  */
-- 
MST

Re: [PATCH] lockdep: don't pull in includes when lockdep disabled

[Arjan van de Ven]

On Wed, 2006-07-26 at 09:26 +0300, Michael S. Tsirkin wrote:
> Ingo, does the following look good to you?
> 
> Do not pull in various includes through lockdep.h if lockdep is disabled.

Hi,

can you tell us what this fixes? Eg is there a specific problem?
I mean... we're adding ifdefs so there better be a real good reason for
them.... fixing something real would be such a reason ;-)

Greetings,
    Arjan van de Ven

Re: [PATCH] lockdep: don't pull in includes when lockdep disabled

[Michael S. Tsirkin]

Quoting r. Arjan van de Ven <arjan@infradead.org>:
> Subject: Re: [PATCH] lockdep: don't pull in includes when lockdep disabled
> 
> On Wed, 2006-07-26 at 09:26 +0300, Michael S. Tsirkin wrote:
> > Ingo, does the following look good to you?
> > 
> > Do not pull in various includes through lockdep.h if lockdep is disabled.
> 
> Hi,
> 
> can you tell us what this fixes? Eg is there a specific problem?

Er ... it's a cosmetic change - there's no serious problem, it is just that even
if I disable lockdep, linux/lockdep.h will pull in several headers even
though they are not needed -> more useless work for compiler to do.

> I mean... we're adding ifdefs

Note this doesn't add ifdefs, just moves them around.

> so there better be a real good reason for
> them.... fixing something real would be such a reason ;-)

Well, I don't expect this specific bit to speed compilation up in any measurable
way, but unnecessary includes do have the tendency to accumulate and lead to
slower builds ...

Is that a reason?

-- 
MST

Re: [PATCH] lockdep: don't pull in includes when lockdep disabled

[Alexey Dobriyan]

On Wed, Jul 26, 2006 at 08:33:19AM +0200, Arjan van de Ven wrote:
> On Wed, 2006-07-26 at 09:26 +0300, Michael S. Tsirkin wrote:
> > Ingo, does the following look good to you?
> >
> > Do not pull in various includes through lockdep.h if lockdep is disabled.
> 
> Hi,
> 
> can you tell us what this fixes? Eg is there a specific problem?

[raises hand]
Zillions of warnings on m68k allmodconfig. And, yes, patch removes them.

In file included from ...
		 from ...
include/linux/list.h: In function `__list_add_rcu':
include/linux/list.h:89: warning: implicit declaration of function `smp_wmb'

> I mean... we're adding ifdefs so there better be a real good reason for
> them.... fixing something real would be such a reason ;-)

Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately

[Zach Brown]

> This moves code out of a common function and so results in code duplication
> and has memory cost.

Of course.  I don't care which trade-offs you prefer to maintain as long
as the driver stops yelling at me as the machine boots.  That patch was
what Arjan suggested as the simplest.

Also, while looking at this I saw that the locks are being
re-initialized from mthca_modify_qp().  Is that just a side-effect of
relying on mthca_wq_init() to reset the non-lock members?  If you're
concerned about microoptimization it seems like this could be avoided.

- z

Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately

[Michael S. Tsirkin]

Quoting r. Zach Brown <zach.brown@oracle.com>:
> Is that just a side-effect of
> relying on mthca_wq_init() to reset the non-lock members?

Yes.


> If you're
> concerned about microoptimization it seems like this could be avoided.

This is off the fast path so I think Roland's idea was we should only care
about code size. But might be worth thinking this over, anyway. Thanks!

-- 
MST

Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately

[Roland Dreier]

    Zach> Also, while looking at this I saw that the locks are being
    Zach> re-initialized from mthca_modify_qp().  Is that just a
    Zach> side-effect of relying on mthca_wq_init() to reset the
    Zach> non-lock members?  If you're concerned about
    Zach> microoptimization it seems like this could be avoided.

I think that is actually a very minor bug you've found.  If someone
were posting a work request at the same time as they transitioned a QP
to reset (which is a legitimate if not sensible thing to do), then the
spinlock could get reinitialized while it was held.  Which would be
bad.  So I think I like your original patch the best.

 - R.

Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier@cisco.com>:
> I think that is actually a very minor bug you've found.  If someone
> were posting a work request at the same time as they transitioned a QP
> to reset (which is a legitimate if not sensible thing to do), then the
> spinlock could get reinitialized while it was held.  Which would be
> bad.

I think you can't post work requests to a QP in reset state, since IB spec
forbids this. If you do, it seems bad things will happen anyway, for example
head/tail pointers may get out of sync as mthca_wq_init clears these.

-- 
MST