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 ? -- 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
