Re: [patch 2.6.15-mm4] sem2mutex: infiniband, #2

[Roland Dreier <rdreier@cisco.com>]

Thanks, I finally got around to looking at this, and I applied the
infiniband/ulp/srp part of this to my tree.  However, I noticed some
fishy things in other parts of the patch.  First of all, the entire
user_mad.c diff you sent is:

 > --- linux.orig/drivers/infiniband/core/user_mad.c
 > +++ linux/drivers/infiniband/core/user_mad.c
 > @@ -47,7 +47,7 @@
 >  #include <linux/kref.h>
 >  
 >  #include <asm/uaccess.h>
 > -#include <asm/semaphore.h>
 > +#include <linux/mutex.h>
 >  
 >  #include <rdma/ib_mad.h>
 >  #include <rdma/ib_user_mad.h>

it seems that you're just getting lucky that struct semaphore is
getting defined implicitly, since you didn't remove the actual
semaphore use.

The semaphore usage in user_mad.c seems a little tricky to convert to
a mutex, if I understand things correctly.  The idea is that we create
a special file that turns on the "IsSM" bit on the InfiniBand port
when it's open (don't worry about what that means, really).  We use a
semaphore to prevent anyone else from opening the file and trying to
be a SM until the first file is closed -- but of course the last
reference to a file might be dropped by a different process than the
one that opened it, so the up() might be called from a different
process than the original down().

Second, in the mthca changes, you have

 > --- linux.orig/drivers/infiniband/hw/mthca/mthca_dev.h
 > +++ linux/drivers/infiniband/hw/mthca/mthca_dev.h
 > @@ -44,7 +44,9 @@
 >  #include <linux/pci.h>
 >  #include <linux/dma-mapping.h>
 >  #include <linux/timer.h>
 > -#include <asm/semaphore.h>
 > +#include <linux/mutex.h>

but again not all semaphore usages have been removed.

For example, a few lines later in the same file, we have:

 > @@ -111,7 +113,7 @@ enum {
 >  struct mthca_cmd {
 >  	struct pci_pool          *pool;
 >  	int                       use_events;
 > -	struct semaphore          hcr_sem;
 > +	struct mutex              hcr_mutex;
 >  	struct semaphore 	  poll_sem;
 >  	struct semaphore 	  event_sem;

so poll_sem and event_sem remain.

Again, these seem hard to convert to mutexes, especially event_sem.
The device firmware allows some number of commands to be queued up (64
is a typical number) and event_sem is used to keep track of when a
command slot is available.  I guess I could rewrite things so that I
use something like wait_event() to block until there is a free command
slot, but I'm not convinced that's an improvement.

Thanks,
  Roland
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/