mthca: initialize send and receive queue locks separately
Subsystems (2)
PATCH 1/1 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]
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.
---
drivers/infiniband/hw/mthca/mthca_qp.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/infiniband/hw/mthca/mthca_qp.c +4 -1
@@ -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)
Quoting r. Zach Brown <[email protected]>:
Quoted 9 lines
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 inmthca_modify_qp()is recursive acquiry of one lock:
Quoted 7 lines
=============================================
[ 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?
Quoted 4 lines
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 <[email protected]>
This moves code out of a common function and so results in code duplication and has memory cost.
Quoted 9 lines
---
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
===================================================================
drivers/infiniband/hw/mthca/mthca_qp.c line 230
Quoted 8 lines
drivers/infiniband/hw/mthca/mthca_qp.c +1
-1@@ -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?
drivers/infiniband/hw/mthca/mthca_qp.c line 1122
Quoted 11 lines
drivers/infiniband/hw/mthca/mthca_qp.c +3
-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)
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?
Signature · 1 line
MST
Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines
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 <[email protected]>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 <[email protected]>
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.
--- drivers/infiniband/hw/mthca/mthca_qp.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)
drivers/infiniband/hw/mthca/mthca_qp.c +11 -5
@@ -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)
Quoting r. Ingo Molnar <[email protected]>:
Quoted 18 lines
Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately
Quoted 7 lines · Michael S. Tsirkin
Quoted 4 lines
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 <[email protected]>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
Quoted 8 lines
---------------->
Subject: lockdep: annotatedrivers/infiniband/hw/mthca/mthca_qp.c
From: Ingo Molnar <[email protected]>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.
Quoted 1 line
Has no effect on non-lockdep kernels.
Hmm ... adding parameters to function still has text cost, I think. No?
Quoted 10 lines
Signed-off-by Ingo Molnar <[email protected]>--- drivers/infiniband/hw/mthca/mthca_qp.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)Index:
linux/drivers/infiniband/hw/mthca/mthca_qp.c
===================================================================
drivers/infiniband/hw/mthca/mthca_qp.c static void *get_send_wqe(struct mthca_q · line 229
Quoted 9 lines
drivers/infiniband/hw/mthca/mthca_qp.c +5
-1@@ -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?
Quoted 23 lines
drivers/infiniband/hw/mthca/mthca_qp.c +4
-2+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)) {
drivers/infiniband/hw/mthca/mthca_qp.c static int mthca_alloc_qp_common(struct · line 1125
Quoted 10 lines
drivers/infiniband/hw/mthca/mthca_qp.c +2
-2@@ -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);
Signature · 1 line
MST
Quoted 3 lines · Michael S. Tsirkin
Quoted 1 line
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
Quoting r. Ingo Molnar <[email protected]>:
Quoted 5 lines
Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately
* Michael S. Tsirkin <[email protected]> wrote:
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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 :)
Signature · 1 line
MST
PATCH 1/1 lockdep: don't pull in includes when lockdep disabled
In reply to Ingo MolnarIngo, does the following look good to you?
Do not pull in various includes through lockdep.h if lockdep is disabled.
include/linux/lockdep.h +2 -2
@@ -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: */
Signature · 1 line
MST
Re: [PATCH] lockdep: don't pull in includes when lockdep disabled
Quoted 3 lines · Michael S. Tsirkin
Ingo, does the following look good to you?
Do not pull in various includes through
lockdep.hif 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
Quoting r. Arjan van de Ven <[email protected]>:
Quoted 10 lines
Subject: Re: [PATCH] lockdep: don't pull in includes when lockdep disabled
Quoted 3 lines · Michael S. Tsirkin
Ingo, does the following look good to you?
Do not pull in various includes through
lockdep.hif 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.
Quoted 1 line
I mean... we're adding ifdefs
Note this doesn't add ifdefs, just moves them around.
Quoted 2 lines
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?
Signature · 1 line
MST
Re: [PATCH] lockdep: don't pull in includes when lockdep disabled
In reply to Arjan van de VenQuoted 8 lines · Arjan van de Ven
Quoted 3 lines · Michael S. Tsirkin
Ingo, does the following look good to you?
Do not pull in various includes through
lockdep.hif 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'
Quoted 2 lines
I mean... we're adding ifdefs so there better be a real good reason for them.... fixing something real would be such a reason ;-)
Quoted 3 lines
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
Quoting r. Zach Brown <[email protected]>:
Quoted 2 lines
Is that just a side-effect of
relying onmthca_wq_init()to reset the non-lock members?
Yes.
Quoted 2 lines
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!
Signature · 1 line
MST
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.
Quoting r. Roland Dreier <[email protected]>:
Quoted 5 lines
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.
Signature · 1 line
MST