***SPAM*** Re: ***SPAM*** Re: [ofa-general] [PATCH] libibmad: remove functions which use pthread
Hal Rosenstock
hal.rosenstock at gmail.com
Tue Feb 17 13:12:12 PST 2009
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 ?
> 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 ?
> Are those calls which use it not thread safe?
They look OK but I'm not 100% sure yet.
-- 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
>>
>>
>
>
>
>
More information about the general
mailing list