***SPAM*** Re: [ofa-general] [PATCH] libibmad: remove functions which use pthread
Ira Weiny
weiny2 at llnl.gov
Tue Feb 17 14:28:59 PST 2009
On Tue, 17 Feb 2009 16:12:12 -0500
Hal Rosenstock <hal.rosenstock at gmail.com> wrote:
> On Tue, Feb 17, 2009 at 12:19 PM, <weiny2 at llnl.gov> wrote:
> > Quoting Hal Rosenstock <hal.rosenstock at gmail.com>:
> >
> >> Sasha,
> >>
> >> On Wed, Dec 31, 2008 at 12:04 PM, Sasha Khapyorsky <sashak at voltaire.com>
> >> 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.
However, it is pretty confusing which interface to use... [or even that there
are 2 interfaces, but I digress] (see below)
>
> > 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 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 <sashak at voltaire.com>
> >>> ---
> >>> 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
> >>> general at lists.openfabrics.org
> >>> http:// lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> >>>
> >>> To unsubscribe, please visit http://
> >>> openib.org/mailman/listinfo/openib-general
> >>>
> >> _______________________________________________
> >> general mailing list
> >> general at lists.openfabrics.org
> >> http:// lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> >>
> >> To unsubscribe, please visit http://
> >> openib.org/mailman/listinfo/openib-general
> >>
> >>
> >
> >
> >
> >
>
--
Ira Weiny <weiny2 at llnl.gov>
More information about the general
mailing list