Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
Quoting r. Zach Brown [EMAIL PROTECTED]:
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 [EMAIL PROTECTED]
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.c2006-07-03
08:41:16.0 -0400
+++ 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c 2006-07-03
10:05:52.0 -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
___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
* Michael S. Tsirkin [EMAIL PROTECTED] 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 [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.
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
===
--- 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)
___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
Quoting r. Ingo Molnar [EMAIL PROTECTED]:
Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately
* Michael S. Tsirkin [EMAIL PROTECTED] 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 [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)
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 [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
===
--- 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
___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
* Michael S. Tsirkin [EMAIL PROTECTED] 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 ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
Quoting r. Ingo Molnar [EMAIL PROTECTED]: Subject: Re: [PATCH] mthca: initialize send and receive queue locks separately * Michael S. Tsirkin [EMAIL PROTECTED] 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 ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
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 ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
Quoting r. Zach Brown [EMAIL PROTECTED]: 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 ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
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. ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
Re: [openib-general] [PATCH] mthca: initialize send and receive queue locks separately
Quoting r. Roland Dreier [EMAIL PROTECTED]: 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 ___ openib-general mailing list openib-general@openib.org http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
[openib-general] [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]
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]
---
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.0 -0400
+++ 2.6.17-mm6/drivers/infiniband/hw/mthca/mthca_qp.c 2006-07-03
10:05:52.0 -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)
___
openib-general mailing list
openib-general@openib.org
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
