Quoting Hal Rosenstock <[email protected]>:

Sasha,

On Wed, Dec 31, 2008 at 12:04 PM, Sasha Khapyorsky <[email protected]> wrote:

I looked at implementation of safe_*() functions (safe_smp_query,
safe_smp_set and safe_ca_call) and found that they are not actually
"safe" as declared by its names. The only thread-unsafe thing which
is used there is static 'mad_portid' structure (from rpc.c),

I'm not sure that the only thread unsafe thing in the mad rpc
mechanism is the portid.

but modification of this structure is not protected by same mutex (actually
not protected at all).

A first step would be removing the portid as static. If so, portid
would need to be a supplied parameter to various mad routines and the
existing ones relying on madrpc_portid would be deprecated. Does this
make sense to do ? Would you accept such a patch ?


Don't we already have an interface like this with mad_rpc_open_port?

I don't like the void * return but it is "struct ibmadb_port" under the hood. Are those calls which use it not thread safe?

Ira


-- Hal

As far as I know nothing uses those safe_*() primitives right now outside
libibmad, so I think it is better to remove this confused functions from
API (with changing library version, etc.).

The primitives madrpc_lock() and madrpc_unlock() are just wrappers to
hidden static pthread mutex which is not controlled by caller
application. I think that it will be more robust for multithreaded
application to use its own synchronization methods (pthread mutex or any
other) for better control. So let's remove madrpc_lock/unlock() too.

Signed-off-by: Sasha Khapyorsky <[email protected]>
---
libibmad/include/infiniband/mad.h | 41 -------------------------------------
 libibmad/libibmad.ver             |    2 +-
 libibmad/src/libibmad.map         |    2 -
 libibmad/src/rpc.c                |   15 -------------
 libibmad/src/sa.c                 |    5 ++-
 5 files changed, 4 insertions(+), 61 deletions(-)

diff --git a/libibmad/include/infiniband/mad.h b/libibmad/include/infiniband/mad.h
index eff6738..89b4be5 100644
--- a/libibmad/include/infiniband/mad.h
+++ b/libibmad/include/infiniband/mad.h
@@ -703,8 +703,6 @@ void * madrpc_rmpp(ib_rpc_t *rpc, ib_portid_t *dport, ib_rmpp_hdr_t *rmpp,
 void   madrpc_init(char *dev_name, int dev_port, int *mgmt_classes,
                   int num_classes);
 void   madrpc_save_mad(void *madbuf, int len);
-void   madrpc_lock(void);
-void   madrpc_unlock(void);
 void   madrpc_show_errors(int set);

 void * mad_rpc_open_port(char *dev_name, int dev_port, int *mgmt_classes,
@@ -725,32 +723,6 @@ uint8_t * smp_query_via(void *buf, ib_portid_t *id, unsigned attrid, uint8_t * smp_set_via(void *buf, ib_portid_t *id, unsigned attrid, unsigned mod,
                     unsigned timeout, const void *srcport);

-inline static uint8_t *
-safe_smp_query(void *rcvbuf, ib_portid_t *portid, unsigned attrid, unsigned mod,
-              unsigned timeout)
-{
-       uint8_t *p;
-
-       madrpc_lock();
-       p = smp_query(rcvbuf, portid, attrid, mod, timeout);
-       madrpc_unlock();
-
-       return p;
-}
-
-inline static uint8_t *
-safe_smp_set(void *rcvbuf, ib_portid_t *portid, unsigned attrid, unsigned mod,
-            unsigned timeout)
-{
-       uint8_t *p;
-
-       madrpc_lock();
-       p = smp_set(rcvbuf, portid, attrid, mod, timeout);
-       madrpc_unlock();
-
-       return p;
-}
-
 /* sa.c */
 uint8_t * sa_call(void *rcvbuf, ib_portid_t *portid, ib_sa_call_t *sa,
                 unsigned timeout);
@@ -761,19 +733,6 @@ int ib_path_query(ibmad_gid_t srcgid, ibmad_gid_t destgid, ib_portid_t *sm_id,
 int    ib_path_query_via(const void *srcport, ibmad_gid_t srcgid,
ibmad_gid_t destgid, ib_portid_t *sm_id, void *buf);

-inline static uint8_t *
-safe_sa_call(void *rcvbuf, ib_portid_t *portid, ib_sa_call_t *sa,
-            unsigned timeout)
-{
-       uint8_t *p;
-
-       madrpc_lock();
-       p = sa_call(rcvbuf, portid, sa, timeout);
-       madrpc_unlock();
-
-       return p;
-}
-
 /* resolve.c */
 int    ib_resolve_smlid(ib_portid_t *sm_id, int timeout);
 int    ib_resolve_guid(ib_portid_t *portid, uint64_t *guid,
diff --git a/libibmad/libibmad.ver b/libibmad/libibmad.ver
index 7e93c16..23d2dc2 100644
--- a/libibmad/libibmad.ver
+++ b/libibmad/libibmad.ver
@@ -6,4 +6,4 @@
 # API_REV - advance on any added API
 # RUNNING_REV - advance any change to the vendor files
 # AGE - number of backward versions the API still supports
-LIBVERSION=5:0:4
+LIBVERSION=2:0:0
diff --git a/libibmad/src/libibmad.map b/libibmad/src/libibmad.map
index 927e51c..f944d86 100644
--- a/libibmad/src/libibmad.map
+++ b/libibmad/src/libibmad.map
@@ -72,14 +72,12 @@ IBMAD_1.3 {
               madrpc;
               madrpc_def_timeout;
               madrpc_init;
-               madrpc_lock;
               madrpc_portid;
               madrpc_rmpp;
               madrpc_save_mad;
               madrpc_set_retries;
               madrpc_set_timeout;
               madrpc_show_errors;
-               madrpc_unlock;
               ib_path_query;
               sa_call;
               sa_rpc_call;
diff --git a/libibmad/src/rpc.c b/libibmad/src/rpc.c
index 5226540..670a936 100644
--- a/libibmad/src/rpc.c
+++ b/libibmad/src/rpc.c
@@ -38,7 +38,6 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
-#include <pthread.h>
 #include <string.h>
 #include <errno.h>

@@ -286,20 +285,6 @@ madrpc_rmpp(ib_rpc_t *rpc, ib_portid_t *dport, ib_rmpp_hdr_t *rmpp, void *data)
       return mad_rpc_rmpp(&port, rpc, dport, rmpp, data);
 }

-static pthread_mutex_t rpclock = PTHREAD_MUTEX_INITIALIZER;
-
-void
-madrpc_lock(void)
-{
-       pthread_mutex_lock(&rpclock);
-}
-
-void
-madrpc_unlock(void)
-{
-       pthread_mutex_unlock(&rpclock);
-}
-
 void
madrpc_init(char *dev_name, int dev_port, int *mgmt_classes, int num_classes)
 {
diff --git a/libibmad/src/sa.c b/libibmad/src/sa.c
index 27b9d52..c601254 100644
--- a/libibmad/src/sa.c
+++ b/libibmad/src/sa.c
@@ -132,7 +132,7 @@ ib_path_query_via(const void *srcport, ibmad_gid_t srcgid, ibmad_gid_t destgid,
       if (srcport) {
               p = sa_rpc_call (srcport, buf, sm_id, &sa, 0);
       } else {
-               p = safe_sa_call(buf, sm_id, &sa, 0);
+               p = sa_call(buf, sm_id, &sa, 0);
       }
       if (!p) {
               IBWARN("sa call path_query failed");
@@ -142,8 +142,9 @@ ib_path_query_via(const void *srcport, ibmad_gid_t srcgid, ibmad_gid_t destgid,
       mad_decode_field(p, IB_SA_PR_DLID_F, &dlid);
       return dlid;
 }
+
 int
ib_path_query(ibmad_gid_t srcgid, ibmad_gid_t destgid, ib_portid_t *sm_id, void *buf)
 {
-       return ib_path_query_via (NULL, srcgid, destgid, sm_id, buf);
+       return ib_path_query_via(NULL, srcgid, destgid, sm_id, buf);
 }
--
1.6.0.4.766.g6fc4a

_______________________________________________
general mailing list
[email protected]
http:// lists.openfabrics.org/cgi-bin/mailman/listinfo/general

To unsubscribe, please visit http:// openib.org/mailman/listinfo/openib-general

_______________________________________________
general mailing list
[email protected]
http:// lists.openfabrics.org/cgi-bin/mailman/listinfo/general

To unsubscribe, please visit http:// openib.org/mailman/listinfo/openib-general





_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general

To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general

Reply via email to