[PATCH] ib-release-locks-in-the-proper-order

[PATCH] ib-release-locks-in-the-proper-order

[John Kacur]

Please consider the following patch - originally from Steven Rostedt. 
It solves a problem for rt which is very sensitive to the lock ordering. 
It should have a no impact on non-rt.

The patch applies to current tip/master - but it is fine with me if it 
would be more appropriate for one of the infiniband people to take it.

Thanks

>From e533f2b9ee9b0bd95aaa4c3369e79b350c9895d3 Mon Sep 17 00:00:00 2001
From: Steven Rostedt <srostedt@redhat.com>
Date: Mon, 21 Sep 2009 21:23:46 +0200
Subject: [PATCH] ib: release locks in the proper order

RT is very sensitive to the order locks are taken and released
wrt read write locks. We must do

  lock(a);
  lock(b);
  lock(c);

  [...]

  unlock(c);
  unlock(b);
  unlock(a);

otherwise bad things can happen.

Signed-off-by: Ken Cox <jkc@redhat.com>
Signed-off-by: Clark Williams <williams@redhat.com>
Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/infiniband/core/uverbs_cmd.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 56feab6..5ea66e1 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -1116,12 +1116,12 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 		goto err_copy;
 	}
 
-	put_pd_read(pd);
-	put_cq_read(scq);
-	if (rcq != scq)
-		put_cq_read(rcq);
 	if (srq)
 		put_srq_read(srq);
+	if (rcq != scq)
+		put_cq_read(rcq);
+	put_cq_read(scq);
+	put_pd_read(pd);
 
 	mutex_lock(&file->mutex);
 	list_add_tail(&obj->uevent.uobject.list, &file->ucontext->qp_list);
@@ -1140,14 +1140,14 @@ err_destroy:
 	ib_destroy_qp(qp);
 
 err_put:
-	if (pd)
-		put_pd_read(pd);
-	if (scq)
-		put_cq_read(scq);
-	if (rcq && rcq != scq)
-		put_cq_read(rcq);
 	if (srq)
 		put_srq_read(srq);
+	if (rcq && rcq != scq)
+		put_cq_read(rcq);
+	if (scq)
+		put_cq_read(scq);
+	if (pd)
+		put_pd_read(pd);
 
 	put_uobj_write(&obj->uevent.uobject);
 	return ret;
-- 
1.6.0.6

Re: [PATCH] ib-release-locks-in-the-proper-order

[Steven Rostedt]

On Mon, 2009-09-21 at 21:35 +0200, John Kacur wrote:
> Please consider the following patch - originally from Steven Rostedt. 
> It solves a problem for rt which is very sensitive to the lock ordering. 
> It should have a no impact on non-rt.
> 
> The patch applies to current tip/master - but it is fine with me if it 
> would be more appropriate for one of the infiniband people to take it.
> 
> Thanks
> 
> >From e533f2b9ee9b0bd95aaa4c3369e79b350c9895d3 Mon Sep 17 00:00:00 2001
> From: Steven Rostedt <srostedt@redhat.com>
> Date: Mon, 21 Sep 2009 21:23:46 +0200
> Subject: [PATCH] ib: release locks in the proper order
> 
> RT is very sensitive to the order locks are taken and released
> wrt read write locks. We must do
> 
>   lock(a);
>   lock(b);
>   lock(c);
> 
>   [...]
> 
>   unlock(c);
>   unlock(b);
>   unlock(a);
> 
> otherwise bad things can happen.
> 
> Signed-off-by: Ken Cox <jkc@redhat.com>
> Signed-off-by: Clark Williams <williams@redhat.com>
> Signed-off-by: John Kacur <jkacur@redhat.com>

The -rt patch doesn't use the multi rwlock code anymore (the reason for
the first patch), and the last revision of that code was able to handle
that too.

Linus totally ripped into this idea. A lock must be able to handle any
order of unlocking. There should be no technical reason a lock must be
unlocked in reverse order they were locked.

What exactly is sensitive about this?

-- Steve

Re: [PATCH] ib-release-locks-in-the-proper-order

[John Kacur]

----- "Steven Rostedt" <rostedt@goodmis.org> wrote:

> On Mon, 2009-09-21 at 21:35 +0200, John Kacur wrote:
> > Please consider the following patch - originally from Steven
> Rostedt. 
> > It solves a problem for rt which is very sensitive to the lock
> ordering. 
> > It should have a no impact on non-rt.
> > 
> > The patch applies to current tip/master - but it is fine with me if
> it 
> > would be more appropriate for one of the infiniband people to take
> it.
> > 
> > Thanks
> > 
> > >From e533f2b9ee9b0bd95aaa4c3369e79b350c9895d3 Mon Sep 17 00:00:00
> 2001
> > From: Steven Rostedt <srostedt@redhat.com>
> > Date: Mon, 21 Sep 2009 21:23:46 +0200
> > Subject: [PATCH] ib: release locks in the proper order
> > 
> > RT is very sensitive to the order locks are taken and released
> > wrt read write locks. We must do
> > 
> >   lock(a);
> >   lock(b);
> >   lock(c);
> > 
> >   [...]
> > 
> >   unlock(c);
> >   unlock(b);
> >   unlock(a);
> > 
> > otherwise bad things can happen.
> > 
> > Signed-off-by: Ken Cox <jkc@redhat.com>
> > Signed-off-by: Clark Williams <williams@redhat.com>
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> 
> The -rt patch doesn't use the multi rwlock code anymore (the reason
> for
> the first patch), and the last revision of that code was able to
> handle
> that too.
> 
> Linus totally ripped into this idea. A lock must be able to handle
> any
> order of unlocking. There should be no technical reason a lock must
> be
> unlocked in reverse order they were locked.
> 
> What exactly is sensitive about this?
> 

Thanks Steve!
I hereby withdraw this patch!!!!