***SPAM*** Re: ***SPAM*** Re: [ofa-general] [PATCH] libibmad: remove functions which use pthread
Hal Rosenstock
hal.rosenstock at gmail.com
Tue Feb 17 15:21:02 PST 2009
On 2/17/09, Ira Weiny <weiny2 at llnl.gov> wrote:
> 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.
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 <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