***SPAM*** Re: [ofa-general] [PATCH] libibmad: remove functions which use pthread
Hal Rosenstock
hal.rosenstock at gmail.com
Mon Feb 16 06:52:37 PST 2009
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 ?
-- 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
>
More information about the general
mailing list