On 2/17/09, Ira Weiny <[email protected]> wrote: > On Tue, 17 Feb 2009 16:12:12 -0500 > Hal Rosenstock <[email protected]> wrote: > >> On Tue, Feb 17, 2009 at 12:19 PM, <[email protected]> wrote: >> > 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'm not sure this was carried all the way through (The basic building >> blocks are there but I think some additional routines are needed). >> >> Shouldn't the in tree clients be converted over and the old routines >> deprecated ? > > For utilities which run once through I think the old functions work just > fine.
Well, sort of... Aren't mad_portid "collisions" possible when multiple programs are run concurrently ? > However, it is pretty confusing which interface to use... [or even that > there > are 2 interfaces, but I digress] (see below) I don't think the newer improved interfaces were ever documented. >> > I don't like the void * return but it is "struct ibmadb_port" under the >> > hood. >> >> Is access into that currently opaque struct needed for something by >> the clients of the library ? > > There is nothing the clients need to access but it would be much better to > return some named data type. This along with some documentation would > clarify > what the difference between madrpc and mad_rpc really is. Furthermore, a > named type will help to "self document" other functions like "mad_rpc". For > example: > > void *mad_rpc(const ibmad_port_t *ibmad_port, ib_rpc_t * rpc, ib_portid_t > * dport, > void *payload, void *rcvdata); > > Oh now I found it... Check out smp_[query|set]_via... Here the interface > changes the parameter name and one has no idea what the type is (without > looking at the code that is! ;-) > > uint8_t *smp_query_via(void *buf, ib_portid_t * id, unsigned attrid, > unsigned mod, unsigned timeout, const void *srcport); > ^^^^ > > uint8_t *smp_set_via(void *buf, ib_portid_t * id, unsigned attrid, > unsigned mod, > unsigned timeout, const void *srcport); > ^^^^ > And here is one more... > int ib_path_query_via(const void *srcport, ibmad_gid_t srcgid, > ibmad_gid_t destgid, ib_portid_t * sm_id, void *buf); Are you referring to how srcport is used to call either the old madrpc or the newer mad_rpc API and if the newer one srcport is really a pointer to a struct ibmad_port ? -- Hal >> > Are those calls which use it not thread safe? >> >> They look OK but I'm not 100% sure yet. > > Yea, they look thread safe but I am not sure either. :-( > > I would be in favor of making all the utils use mad_rpc_open_port but it is > up > to Shasha if we go down this path. > > Ira > >> >> -- Hal >> >> > 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 >> >> >> >> >> > >> > >> > >> > >> > > > -- > Ira Weiny <[email protected]> > _______________________________________________ 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
