Re: [openib-general] [PATCH] Fix potential deadlock in mthca
On Fri, 2006-08-11 at 09:15 -0700, Arjan van de Ven wrote: Roland Dreier wrote: Here's a long-standing bug that lockdep found very nicely. Ingo/Arjan, can you confirm that the fix looks OK and I am using spin_lock_nested() properly? I couldn't find much documentation or many examples of it, so I'm not positive this is the right way to handle this fix. looks correct to me; Acked-by: Arjan van de Ven [EMAIL PROTECTED] looks good to me too. Acked-by: Ingo Molnar [EMAIL PROTECTED] btw., we could introduce a new spin-lock op: spin_lock_double(l1, l2); ... spin_unlock_double(l1, l2); because some other code, like kernel/sched.c, fs/dcache.c and kernel/futex.c uses quite similar locking. 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] ipoib lockdep warning
* Roland Dreier [EMAIL PROTECTED] wrote: Hmm, good point. It sort of seems to me like the idr interfaces are broken by design. [...] So, ugh... maybe the best thing to do is change lib/idr.c to use spin_lock_irqsave() internally? i agree that the IDR subsystem should be irq-safe if GFP_ATOMIC is passed in. So the _irqsave()/_irqrestore() fix should be done. But i also think that you should avoid using GFP_ATOMIC for any sort of reliable IO path and push as much work into process context as possible. Is it acceptable for your infiniband IO model to fail with -ENOMEM if GFP_ATOMIC happens to fail, and is the IO retried transparently? 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] Convert idr's internal locking to _irqsave variant
* Roland Dreier [EMAIL PROTECTED] wrote: Currently, the code in lib/idr.c uses a bare spin_lock(idp-lock) to do internal locking. This is a nasty trap for code that might call idr functions from different contexts; for example, it seems perfectly reasonable to call idr_get_new() from process context and idr_remove() from interrupt context -- but with the current locking this would lead to a potential deadlock. The simplest fix for this is to just convert the idr locking to use spin_lock_irqsave(). In particular, this fixes a very complicated locking issue detected by lockdep, involving the ib_ipoib driver's priv-lock and dev-_xmit_lock, which get involved with the ib_sa module's query_idr.lock. Cc: Arjan van de Ven [EMAIL PROTECTED] Cc: Ingo Molnar [EMAIL PROTECTED] Acked-by: Ingo Molnar [EMAIL PROTECTED] 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
* 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
* 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
