RE: creating common ib_types.h for linux and windows
>-Original Message- >From: linux-rdma-ow...@vger.kernel.org >[mailto:linux-rdma-ow...@vger.kernel.org] On Behalf Of Alex Netes >Sent: Wednesday, September 21, 2011 6:30 AM >To: Hefty, Sean >Cc: Bart Van Assche; linux-rdma (linux-rdma@vger.kernel.org); Hal Rosenstock >(hal.rosenst...@gmail.com); o...@lists.openfabrics.org >Subject: Re: creating common ib_types.h for linux and windows > >On 14:23 Tue 20 Sep , Hefty, Sean wrote: >> > Why to test for __WIN__ instead of _WIN32 (defined both when building >> > 32-bit and 64-bit code -- see also >> > http://msdn.microsoft.com/en-us/library/b0084kay%28v=vs.80%29.aspx) ? >> >> I have no idea. This is just what's currently in the code. I can change >> this portion of the code if we want to use #ifdef's. >> > >Digging into the history, I found this patch that added __WIN__ defined for >Windows: http://www.spinics.net/lists/linux-rdma/msg00451.html Hello Alex, The intent was to unify the multiple ifdef tags for Windows; perhaps _WIN32 might have been a better choice. __WIN__ clearly indicates 'all' windows variants, while _WIN32 at 1st glance tends to imply 32bit Windows for those who are not aware of the MS confusing usage of _WIN32 also defined for _WIN64 systems. If you feel a change is needed then OK. Stan. > >> > When creating a common header file, this might be a good start: >> > https://msinttypes.googlecode.com/svn/trunk/stdint.h >> >> There are already shared types defined between linux and windows that opensm >> uses. For the most part, there's no technical reason why >windows can't use 99% of the linux ib_types.h as is. My goal is to be able to >take the file and drop it into the windows build tree without >needing changes. > >What is your end goal? To have one code base for OpenSM that would be able to >be compiled on both Linux and Windows based on __WIN__ definition? > >> >> Btw, as background I tried to pull in the latest ibmad and ibdiags into >> windows. The build broke because of new defines that had been >added to ib_types.h. Eventually we should be able to drop opensm directly >into the windows build as well. >> >> - Sean >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >-- > >-- Alex >-- >To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: dapltest question
>-Original Message- >From: linux-rdma-ow...@vger.kernel.org >[mailto:linux-rdma-ow...@vger.kernel.org] On Behalf Of Steve Wise >Sent: Friday, July 08, 2011 8:23 AM >To: Davis, Arlin R >Cc: linux-rdma >Subject: dapltest question > >Hey Arlin, > >I have a dumb question: Is dapltest designed to support multiple dapltest >client processes running on multiple physical >systems to connect to a single dapltest server process/system? > >I'm running tests where we have 4 client nodes running dapltest transaction >clients connecting to the same dapltest >server on a 5th node and I'm debugging a dapltest server hang. I have more >details if you're interested, but I just >wanna make sure dapltest is designed to support this mode. > Hi Steve, Arlin is out for at least a week. 'dapltest' server is designed to service [1...n] dapltest clients and is routinely tested in this manner. Send me your dapltest client/server invocations and I'll give them a try. Which DAPL provider are you using? Stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)
Hal Rosenstock wrote: > On 12/6/2010 6:55 PM, Stan C. Smith wrote: >> >> Hello, >>Your suggestion of removing extra casts does operate correctly >> under windows x64/x86 variants. Windows compilers have improved. >> >> thanks, >> >> Stan. >> >> Signed-off-by: stan smith >> >> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c >> b/opensm/libvendor/osm_vendor_ibumad_sa.c index 1fdcc47..63728ad >> 100644 --- a/opensm/libvendor/osm_vendor_ibumad_sa.c +++ >> b/opensm/libvendor/osm_vendor_ibumad_sa.c @@ -85,8 +85,7 @@ >> __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw, >> >> /* obtain the sent context since we store it during send in the >> ni_ctx */p_query_req_copy = >> -(osmv_query_req_t *) (long >> *)(long)(p_req_madw->context.ni_context. >> -node_guid); >> +(osmv_query_req_t *) p_req_madw->context.ni_context.node_guid; >> >> /* provide the context of the original request in the result */ >> query_res.query_context = p_query_req_copy->query_context; >> @@ -181,8 +180,7 @@ static void __osmv_sa_mad_err_cb(IN void >> *bind_context, IN osm_madw_t * p_madw) >> >> /* Obtain the sent context etc */ >> p_query_req_copy = >> -(osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context. >> -node_guid); >> +(osmv_query_req_t *) p_madw->context.ni_context.node_guid; > > On an x86 Linux machine, the above 2 instances changed now complain > about warning: cast to pointer from integer of different size > > so should those be cast to (osmv_query_req_t *) (long) or > (osmv_query_req_t *) (uintptr_t) or something else ? > > -- Hal We should return to my original patch submission. remove the (long*) (long) and replace with (uintptr_t) >> -(osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context. >> -node_guid); >> +(osmv_query_req_t *) (uintptr_t) >> p_madw->context.ni_context.node_guid; Sasha, can you take care of this? stan. > >> >> /* provide the context of the original request in the result */ >> query_res.query_context = p_query_req_copy->query_context; >> @@ -433,7 +431,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * >> p_bind, } *p_query_req_copy = *p_query_req; >> p_madw->context.ni_context.node_guid = >> -(ib_net64_t) (long)p_query_req_copy; >> +(ib_net64_t) (uintptr_t)p_query_req_copy; >> >> /* we can support async as well as sync calls */ >> sync = ((p_query_req->flags& OSM_SA_FLAGS_SYNC) == >> OSM_SA_FLAGS_SYNC); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)
Hefty, Sean wrote: >> Wouldn't it be better to remove those additional castings at all? >> Like below? >> > .. >> p_query_req_copy = (osmv_query_req_t *) >> p_req_madw->context.ni_context.node_guid; > > I think the Windows compiler will complain about data loss on a > 32-bit system. Only the case @ line 433 compiler wise, requires the (uintptr_t) to eliminate the x86 compiler warning: conversion from ptr to ib_net64_t is sign-extended which may result in unexpected runtime behaviors. Otherwise, x64 & x86 compilers digest the single cast approach. The real question becomes, are the runtime truncations observed by Mellanox still present with the single cast? > @@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * p_bind, > } > *p_query_req_copy = *p_query_req; > p_madw->context.ni_context.node_guid = > - (ib_net64_t) (long)p_query_req_copy; > + (ib_net64_t) (uintptr_t)p_query_req_copy; p_madw->context.ni_context.node_guid = (ib_net64_t) p_query_req_copy; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] replace (long*)(long) casting with transportable data type (uintptr_t)
Sasha Khapyorsky wrote: > On 14:12 Thu 09 Sep , Stan C. Smith wrote: >> >> Hello, >> In osm_vendor_ibumad_sa.c the casting of a pointer to a long >> causes data truncation problems in 64-bit Windows where sizeof(long) >> != sizeof(void*). 'uintptr_t' is correct in both operating >> environments. > > Wouldn't it be better to remove those additional castings at all? > Like below? Hello, I assume the original casts were there for a reason hence the cumbersome double cast? Mellanox had reported a truncation problem which was fixed with the (uintptr_t) replacement of (long*)(long). It certainly make more sense to use the single cast if it works; no truncation. I'll check with those who discovered the truncation and see if they tried the obvious single cast approach. stan. > >> >> thank you, >> >> stan. >> >> signed-off-by: stan smith >> >> diff --git a/opensm/libvendor/osm_vendor_ibumad_sa.c >> b/opensm/libvendor/osm_vendor_ibumad_sa.c >> index 1fdcc47..9180972 100644 >> --- a/opensm/libvendor/osm_vendor_ibumad_sa.c >> +++ b/opensm/libvendor/osm_vendor_ibumad_sa.c >> @@ -85,7 +85,7 @@ __osmv_sa_mad_rcv_cb(IN osm_madw_t * p_madw, >> >> /* obtain the sent context since we store it during send in the >> ni_ctx */p_query_req_copy = -(osmv_query_req_t *) (long >> *)(long)(p_req_madw->context.ni_context. + (osmv_query_req_t *) >> >> (uintptr_t)(p_req_madw->context.ni_context. node_guid); > > p_query_req_copy = (osmv_query_req_t *) > p_req_madw->context.ni_context.node_guid; > >> >> /* provide the context of the original request in the result */ >> @@ -181,7 +181,7 @@ static void __osmv_sa_mad_err_cb(IN void >> *bind_context, IN osm_madw_t * p_madw) >> >> /* Obtain the sent context etc */ >> p_query_req_copy = >> -(osmv_query_req_t *) (long *)(long)(p_madw->context.ni_context. >> +(osmv_query_req_t *) (uintptr_t)(p_madw->context.ni_context. >> node_guid); > > p_query_req_copy = (osmv_query_req_t *) > p_madw->context.ni_context.node_guid; > >> >> /* provide the context of the original request in the result */ >> @@ -433,7 +433,7 @@ __osmv_send_sa_req(IN osmv_sa_bind_info_t * >> p_bind, } *p_query_req_copy = *p_query_req; >> p_madw->context.ni_context.node_guid = >> -(ib_net64_t) (long)p_query_req_copy; >> +(ib_net64_t) (uintptr_t)p_query_req_copy; > > p_madw->context.ni_context.node_guid = (ib_net64_t) p_query_req_copy; > > ? > > Sasha > >> >> /* we can support async as well as sync calls */ >> sync = ((p_query_req->flags & OSM_SA_FLAGS_SYNC) == >> OSM_SA_FLAGS_SYNC); -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: ib mad definitions
Ira Weiny wrote: > On Tue, 19 Oct 2010 11:50:46 -0700 > "Hefty, Sean" wrote: > >>> ib_types depends on complib at the moment (fixable) >>> ibutils depends on OpenSM (it will anyway -- non-issue) >>> somethings in ib_types are ugly, byteswapping (non-issue; deal with >>> it later) OpenSM may _not_ include umad and therefore miss defines. >>> (fixable?) >>> >>> As for this last item, would it be a big deal to require umad for >>> the header only? Does umad not compile somewhere that other vendor >>> layers are used? I think it is much better for OpenSM to require >>> umad than for other MAD processing software to require OpenSM. >>> Also, would splitting ib_types help this at all? >> >> I'll propose the following: >> >> 1. Add to libibumad/include/infiniband: >> >>umad_types.h - basic mad, rmpp headers >>umad_sa.h- SA attributes >>umad_cm.h- CM messages >> >> 2. Include umad_types.h and umad_sa.h from ib_types.h >> 3. Include umad_cm.h from ib_cm_types.h >> >> We start with a minimal set of definitions to umad and add/move >> other definitions later as needed, creating new header files where >> appropriate (umad_smi.h, umad_pm.h, etc.) >> >> If we can get some basic agreement on this, I'll start on the >> patches immediately. In an ideal world, the new header files would >> work on any platform. > > I agree, > Ira Just to be painfully clear ... A user-mode application would then only need to include ib_types.h + CM flavor of choice .h files ? > >> >> - Sean >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at >> http://BLOCKEDvger.kernel.org/majordomo-info.html >> > > > -- > Ira Weiny > Math Programmer/Computer Scientist > Lawrence Livermore National Lab > 925-423-8008 > wei...@llnl.gov -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: IPoIB CM connection establishment protocol question
Eli Cohen wrote: > Hi stan, > > Here's a short summary: > An IPoIB interface encodes it capability of supporting CM mode in the > hardware address it publishes. Once the stack hands an SKB to IPoIB for > transmission (unicast), IPoIB needs to resolve the hardware address to > IB address information (LID, SL etc.). Once resolution is complete, it > will create a RC connection with the other host using IB CM. This > process is symmetrical - that is, for two communicating hosts there are > two RC connections so that each connection works half duplex. > > Hope that helps. Hello, Thanks for responding. Indeed this clears the air; the original Windows IPoIB CM code assumed a single connection. stan. > > On Thu, Aug 05, 2010 at 10:25:27AM -0700, Smith, Stan wrote: >> >> Could someone who is OFED IPoIB knowledgeable help me understand >> which IPoIB side starts the RC connection establishment (Tx REQ) to >> an ARP unknown remote host? >> >> I tried reading the Linux IPoIB code and realized it's been too many >> years since writing Linux network drivers (2001) and found myself >> lost in the bits w.r.t. CM connection mgmt. and ARP handling. >> >>> From limited observation, it looks like the ACTIVE side (ttcp -t) >>> of an IPoIB network connection broadcasts an IB ARP-REQ which >>> indicates CM capability in the IB ARP flags byte. >> >> When the PASSIVE IPoIB (ttcp -r) receives the IB ARP-REQ, it sends >> back an IB ARP-REPLY which indicates it's CM capability among other >> items. >> >> Now the confusing part: which IPoIB CM side starts the CM-RC >> connection establishment? >> >> Does the PASSIVE IPoIB start the RC connection (Tx REQ) prior to >> sending the IB ARP-REPLY? >> >> Or does the ACTIVE IPoIB side receive the IB ARP-REPLY and then >> start an ACTIVE RC connection (Tx REQ) now that it knows the remote >> side is CM capable? >> >> thanks, >> >> stan. >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-rdma" in >> the body of a message to majord...@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
IPoIB CM connection establishment protocol question
Could someone who is OFED IPoIB knowledgeable help me understand which IPoIB side starts the RC connection establishment (Tx REQ) to an ARP unknown remote host? I tried reading the Linux IPoIB code and realized it's been too many years since writing Linux network drivers (2001) and found myself lost in the bits w.r.t. CM connection mgmt. and ARP handling. >From limited observation, it looks like the ACTIVE side (ttcp -t) of an IPoIB >network connection broadcasts an IB ARP-REQ which indicates CM capability in >the IB ARP flags byte. When the PASSIVE IPoIB (ttcp -r) receives the IB ARP-REQ, it sends back an IB ARP-REPLY which indicates it's CM capability among other items. Now the confusing part: which IPoIB CM side starts the CM-RC connection establishment? Does the PASSIVE IPoIB start the RC connection (Tx REQ) prior to sending the IB ARP-REPLY? Or does the ACTIVE IPoIB side receive the IB ARP-REPLY and then start an ACTIVE RC connection (Tx REQ) now that it knows the remote side is CM capable? thanks, stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method
Smith, Stan wrote: > > > Following discussion is tangential to Hal's patch. > > Last night I came across an curious discovery. > sizeof(ib_inform_info_t)== 36. > sizeof(ib_inform_info_record_t) == 64. > > My debug version of 'osmtest -f v' trips CL_ASSERT() in > ib_get_attr_offset() osmteset.c::osmtest_informinfo_request()for > ib_get_attr_offset(rec), where rec is ib_inform_info_t. > > The winOFED ib_types.h & OFED ib_types.h match w.r.t. definitions for > ib_inform_info_t & ib_inform_info_record_t. > > Question: should ib_inform_info_t be 8-byte aligned with padding in > ib_inform_info_record_t adjusted or should we remove the CL_ASSERT() > from ib_get_attr_offset()? > > stan. According to the IB spec, ib_inform_info_t is not 8-byte aligned, so removing the CL_ASSERT() makes sense. Sorry about extra work incurred, I thought I had tested all cases. stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method
Following discussion is tangential to Hal's patch. Last night I came across an curious discovery. sizeof(ib_inform_info_t)== 36. sizeof(ib_inform_info_record_t) == 64. My debug version of 'osmtest -f v' trips CL_ASSERT() in ib_get_attr_offset() osmteset.c::osmtest_informinfo_request()for ib_get_attr_offset(rec), where rec is ib_inform_info_t. The winOFED ib_types.h & OFED ib_types.h match w.r.t. definitions for ib_inform_info_t & ib_inform_info_record_t. Question: should ib_inform_info_t be 8-bye aligned with padding in ib_inform_info_record_t adjusted or should we remove the CL_ASSERT() from ib_get_attr_offset()? stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_sa.c: In osm_sa_respond, only fill in attr offset if RMPP method
Sasha Khapyorsky wrote: > On 11:27 Wed 09 Jun , Hal Rosenstock wrote: >> On Wed, Jun 9, 2010 at 8:48 AM, Sasha Khapyorsky >> wrote: >>> On 08:40 Wed 09 Jun , Hal Rosenstock wrote: I think the use of ib_get_attr_offset is being "overused". I'd rather see this patch used as it is the simplest way to address that rather than conditionalize ib_get_attr_offset on the SA attribute or would you rather see a patch along those lines ? >>> >>> I just think that if the assertion is incorrect (and >>> ib_get_attr_offset() is generic function) we need to remove this. >>> That is simplest. The ASSERT() in ib_get_attr_offset() is only valid for the debug case; no runtime impact for the 'normal' build. >> >> The downside is that it loses the ability to catch SA records which >> were not properly padded out. > > It is not run-time error, so I don't think that such sort of debugging > is a main function of OpenSM. > >> It seems like a reasonable tradeoff to >> me to not set this field on the transmit side when it's going to be >> ignored on receive (treat it like a reserved field for this case) and >> keep the ability to catch the SA record issue which has bit the >> community a number of times. > > This will cost in extra code. > >> The only other approach I see to maintain this is to add an >> additional calling parameter (allribute ID) to get_attr_offset and >> then inside >> check the attribute ID and return 0 for those SA attributes. > > Which SA attributes? > > Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] use macro for tmp file path
Sasha Khapyorsky wrote:
> On 09:51 Fri 21 May , Smith, Stan wrote:
>>
>> Use defined macro for tmp file path
>>
>> signed-off-by: stan smith
>>
>
> The patch is whitespece-mangled.
>
>> diff --git a/opensm/opensm/st.c b/opensm/opensm/st.c
>> index ea76038..2d39117 100644
>> --- a/opensm/opensm/st.c
>> +++ b/opensm/opensm/st.c
>> @@ -174,7 +174,7 @@ static int init_st = 0;
>>
>> static void stat_col()
>> {
>> - FILE *f = fopen("/var/log/osm_st_col", "w");
>> + FILE *f = fopen( OSM_DEFAULT_TMP_DIR "osm_st_col", "w");
>
> I think it was discussed somehow in the past.
>
> Shouldn't this be:
>
> OSM_DEFAULT_TMP_DIR "/osm_st_col"
>
> , so that trailing '/' will not be mandatory in directory define?
Currently OSM_DEFAULT_TMP_DIR is defined with a trailing directory delimiter.
Having the delimiter in the OSM_DEFAULT_TMP_DIR makes it easier to be
consistent with \ or /.
Will resend this patch as I think I've figured out the mangled email story.
>
> Sasha
>
>> fprintf(f, "collision: %d\n", collision);
>> fclose(f);
>> }
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-rdma" in the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/complib use portable macro syntax
Sasha Khapyorsky wrote: > Hi Stan, > > On 09:45 Fri 21 May , Smith, Stan wrote: >> >> Use portable macro argument syntax. >> >> signed-of-by: stan smith > > Signed-off-by: oops. > >> > > The patch below is malformed - whitespaces are mangled (please check > your mailer). > > Applied by hands. Thanks. Sorry, will investigate. Perhaps I should resend once I've figured out the story. > > Sasha > >> diff --git a/opensm/complib/cl_event_wheel.c >> b/opensm/complib/cl_event_wheel.c index ef6d598..eb894a6 100644 --- >> a/opensm/complib/cl_event_wheel.c +++ >> b/opensm/complib/cl_event_wheel.c @@ -42,7 +42,7 @@ >> #include >> #include >> >> -#define CL_DBG(fmt, arg...) >> +#define CL_DBG(fmt, ...) >> >> static cl_status_t __event_will_age_before(IN const cl_list_item_t * >>const p_list_item, IN >> void *context) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ib_types.h add debug assert
Add a debug assert to catch incorrect MAD attr offset size.
This proved to be useful in catching incorrect struct sizes as MAD attrs need
to be a multiple of 8 bytes.
signed-off-by: stan smith
diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h
index e1bc102..203c319 100644
--- a/opensm/include/iba/ib_types.h
+++ b/opensm/include/iba/ib_types.h
@@ -4395,6 +4395,7 @@ static inline uint32_t OSM_API ib_get_attr_size(IN const
ib_net16_t attr_offset)
static inline ib_net16_t OSM_API ib_get_attr_offset(IN const uint32_t
attr_size)
{
+ CL_ASSERT((attr_size & 0x07) == 0);
return (cl_hton16((uint16_t) (attr_size >> 3)));
}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] opensm/libvendor Reduce stack consumption
Reduce stack consumption by using a union of structs instead of individual
struct allocations.
Code borrowed from osm_vendor_ibumad_sa.c
signed-off-by: stan smith
diff --git a/opensm/libvendor/osm_vendor_mlx_sa.c
b/opensm/libvendor/osm_vendor_mlx_sa.c
index 91f82fa..0cc3f19 100644
--- a/opensm/libvendor/osm_vendor_mlx_sa.c
+++ b/opensm/libvendor/osm_vendor_mlx_sa.c
@@ -576,14 +576,19 @@ ib_api_status_t
osmv_query_sa(IN osm_bind_handle_t h_bind,
IN const osmv_query_req_t * const p_query_req)
{
- osmv_sa_bind_info_t *p_bind = (osmv_sa_bind_info_t *) h_bind;
+ union {
+ ib_service_record_t svc_rec;
+ ib_node_record_t node_rec;
+ ib_portinfo_record_t port_info;
+ ib_path_rec_t path_rec;
+#ifdef DUAL_SIDED_RMPP
+ ib_multipath_rec_t multipath_rec;
+#endif
+ ib_class_port_info_t class_port_info;
+ } u;
osmv_sa_mad_data_t sa_mad_data;
+ osmv_sa_bind_info_t *p_bind = (osmv_sa_bind_info_t *) h_bind;
osmv_user_query_t *p_user_query;
- ib_service_record_t svc_rec;
- ib_node_record_t node_rec;
- ib_portinfo_record_t port_info;
- ib_path_rec_t path_rec;
- ib_class_port_info_t class_port_info;
osm_log_t *p_log = p_bind->p_log;
ib_api_status_t status;
@@ -617,7 +622,7 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_service_record_t));
sa_mad_data.comp_mask = 0;
- sa_mad_data.p_attr = &svc_rec;
+ sa_mad_data.p_attr = &u.svc_rec;
break;
case OSMV_QUERY_SVC_REC_BY_NAME:
@@ -628,8 +633,8 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.comp_mask = IB_SR_COMPMASK_SNAME;
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_service_record_t));
- sa_mad_data.p_attr = &svc_rec;
- memcpy(svc_rec.service_name, p_query_req->p_query_input,
+ sa_mad_data.p_attr = &u.svc_rec;
+ memcpy(u.svc_rec.service_name, p_query_req->p_query_input,
sizeof(ib_svc_name_t));
break;
@@ -640,8 +645,8 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.comp_mask = IB_SR_COMPMASK_SID;
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_service_record_t));
- sa_mad_data.p_attr = &svc_rec;
- svc_rec.service_id =
+ sa_mad_data.p_attr = &u.svc_rec;
+ u.svc_rec.service_id =
*(ib_net64_t *) (p_query_req->p_query_input);
break;
@@ -653,7 +658,7 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_class_port_info_t));
sa_mad_data.comp_mask = 0;
- sa_mad_data.p_attr = &class_port_info;
+ sa_mad_data.p_attr = &u.class_port_info;
break;
@@ -665,8 +670,8 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_node_record_t));
sa_mad_data.comp_mask = IB_NR_COMPMASK_NODEGUID;
- sa_mad_data.p_attr = &node_rec;
- node_rec.node_info.node_guid =
+ sa_mad_data.p_attr = &u.node_rec;
+ u.node_rec.node_info.node_guid =
*(ib_net64_t *) (p_query_req->p_query_input);
break;
@@ -678,8 +683,8 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_portinfo_record_t));
sa_mad_data.comp_mask = IB_PIR_COMPMASK_LID;
- sa_mad_data.p_attr = &port_info;
- port_info.lid = *(ib_net16_t *) (p_query_req->p_query_input);
+ sa_mad_data.p_attr = &u.port_info;
+ u.port_info.lid = *(ib_net16_t *) (p_query_req->p_query_input);
break;
case OSMV_QUERY_PORT_REC_BY_LID_AND_NUM:
@@ -729,19 +734,19 @@ osmv_query_sa(IN osm_bind_handle_t h_bind,
case OSMV_QUERY_PATH_REC_BY_PORT_GUIDS:
osm_log(p_log, OSM_LOG_DEBUG,
"osmv_query_sa DBG:001 %s", "PATH_REC_BY_PORT_GUIDS\n");
- memset(&path_rec, 0, sizeof(ib_path_rec_t));
+ memset(&u.path_rec, 0, sizeof(ib_path_rec_t));
sa_mad_data.attr_id = IB_MAD_ATTR_PATH_RECORD;
sa_mad_data.attr_offset =
ib_get_attr_offset(sizeof(ib_path_rec_t));
sa_mad_data.comp_mask =
(IB_PR_COMPMASK_DGID | IB_PR_COMPMASK_SGID |
IB_PR_COMPMASK_NUMBPATH);
- path_rec.num_path = 0x7f;
- sa_mad_data.p_attr = &path_rec;
- ib_gid_set_defa
[PATCH] osmtest - use helper function
Use defined inline helper function instead of actual implementation.
Separate implementation from use.
signed-off-by: stan smith
diff --git a/opensm/osmtest/osmtest.c b/opensm/osmtest/osmtest.c
index 50f94db..5d9c54c 100644
--- a/opensm/osmtest/osmtest.c
+++ b/opensm/osmtest/osmtest.c
@@ -719,7 +719,7 @@ osmtest_get_node_rec(IN osmtest_t * const p_osmt,
p_context->p_osmt = p_osmt;
user.comp_mask = IB_NR_COMPMASK_NODEGUID;
user.attr_id = IB_MAD_ATTR_NODE_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -793,7 +793,7 @@ osmtest_get_node_rec_by_lid(IN osmtest_t * const p_osmt,
p_context->p_osmt = p_osmt;
user.comp_mask = IB_NR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_NODE_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -1057,7 +1057,7 @@ osmtest_get_port_rec(IN osmtest_t * const p_osmt,
p_context->p_osmt = p_osmt;
user.comp_mask = IB_PIR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_PORTINFO_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4171,7 +4171,7 @@ osmtest_get_link_rec_by_lid(IN osmtest_t * const p_osmt,
if (to_lid)
user.comp_mask |= IB_LR_COMPMASK_TO_LID;
user.attr_id = IB_MAD_ATTR_LINK_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4249,7 +4249,7 @@ osmtest_get_guidinfo_rec_by_lid(IN osmtest_t * const
p_osmt,
p_context->p_osmt = p_osmt;
user.comp_mask = IB_GIR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_GUIDINFO_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4328,7 +4328,7 @@ osmtest_get_pkeytbl_rec_by_lid(IN osmtest_t * const
p_osmt,
p_context->p_osmt = p_osmt;
user.comp_mask = IB_PKEY_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_PKEY_TBL_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4407,7 +4407,7 @@ osmtest_get_sw_info_rec_by_lid(IN osmtest_t * const
p_osmt,
if (lid)
user.comp_mask = IB_SWIR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_SWITCH_INFO_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4486,7 +4486,7 @@ osmtest_get_lft_rec_by_lid(IN osmtest_t * const p_osmt,
if (lid)
user.comp_mask = IB_LFTR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_LFT_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4565,7 +4565,7 @@ osmtest_get_mft_rec_by_lid(IN osmtest_t * const p_osmt,
if (lid)
user.comp_mask = IB_MFTR_COMPMASK_LID;
user.attr_id = IB_MAD_ATTR_MFT_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
user.p_attr = &record;
req.query_type = OSMV_QUERY_USER_DEFINED;
@@ -4637,7 +4637,7 @@ osmtest_sminfo_record_request(IN osmtest_t * const p_osmt,
p_context->p_osmt = p_osmt;
user.attr_id = IB_MAD_ATTR_SMINFO_RECORD;
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_get_attr_offset((uint16_t) (sizeof(record)));
p_sm_info_opt = p_options;
if (p_sm_info_opt->sm_guid != 0) {
record.sm_info.guid = p_sm_info_opt->sm_guid;
@@ -4737,7 +4737,7 @@ osmtest_informinfo_request(IN osmtest_t * const p_osmt,
p_context->p_osmt = p_osmt;
user.attr_id = attr_id;
if (attr_id == IB_MAD_ATTR_INFORM_INFO_RECORD) {
- user.attr_offset = cl_ntoh16((uint16_t) (sizeof(record) >> 3));
+ user.attr_offset = ib_ge
[PATCH] use macro for tmp file path
Use defined macro for tmp file path
signed-off-by: stan smith
diff --git a/opensm/opensm/st.c b/opensm/opensm/st.c
index ea76038..2d39117 100644
--- a/opensm/opensm/st.c
+++ b/opensm/opensm/st.c
@@ -174,7 +174,7 @@ static int init_st = 0;
static void stat_col()
{
- FILE *f = fopen("/var/log/osm_st_col", "w");
+ FILE *f = fopen( OSM_DEFAULT_TMP_DIR "osm_st_col", "w");
fprintf(f, "collision: %d\n", collision);
fclose(f);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] opensm - use portable macro definition for tmp file path
Use defined macro for tmp file path.
signed-off-by: stan smith
diff --git a/opensm/opensm/osm_db_files.c b/opensm/opensm/osm_db_files.c
index dd9f772..5771e6e 100644
--- a/opensm/opensm/osm_db_files.c
+++ b/opensm/opensm/osm_db_files.c
@@ -615,7 +615,7 @@ int main(int argc, char **argv)
cl_list_construct(&keys);
cl_list_init(&keys, 10);
- osm_log_init_v2(&log, TRUE, 0xff, "/var/log/osm_db_test.log", 0, FALSE);
+ osm_log_init_v2(&log, TRUE, 0xff, OSM_DEFAULT_TMP_DIR
"osm_db_test.log", 0, FALSE);
osm_db_construct(&db);
if (osm_db_init(&db, &log)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] opensm/complib use portable macro syntax
Use portable macro argument syntax. signed-of-by: stan smith diff --git a/opensm/complib/cl_event_wheel.c b/opensm/complib/cl_event_wheel.c index ef6d598..eb894a6 100644 --- a/opensm/complib/cl_event_wheel.c +++ b/opensm/complib/cl_event_wheel.c @@ -42,7 +42,7 @@ #include #include -#define CL_DBG(fmt, arg...) +#define CL_DBG(fmt, ...) static cl_status_t __event_will_age_before(IN const cl_list_item_t * const p_list_item, IN void *context) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Ok to edit the OpenFabrics wiki?
Justin Clift wrote: > Jeff Becker wrote: > >> >> Hi. I'm aware of the problem, and have verified it. Although I spent >> some time looking at it, I haven't been able to fix it yet. Part of >> the issue is that we have a new server ready to go, and I am awaiting >> instructions from an outside review panel (discussed at Sonoma) to >> proceed. If I get some free cycles, I'll take a look, but I expect >> the server (and wiki) move will happen soon, so the problem may be >> moot. >> >> Thanks. > > No worries. I'll take a look again after the server + wiki move I > guess. Seems like the wiki could do with some freshening up, so I'll > see what spare time I have then. > > Regards and best wishes, > > Justin Clift > > -- > Salasaga - Open Source eLearning IDE >http://www.salasaga.org FYI - OFA Windows Wiki pages have been in this inaccessible state for over 2 weeks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_db_files.c: add '/' path delimited
Bart Van Assche wrote: > On Sat, Nov 28, 2009 at 5:35 PM, Smith, Stan > wrote: >> >> Sasha Khapyorsky wrote: >>> Add '/' path delimiter unconditionally on file path generation. >> >> Any chance you could make the path delimiter a #define so there is >> not an explicit Linux filesystem assumption? > > As far as I know the Windows kernel recognizes both forward and > backward slashes as a path separator. > > Bart. Who would of thought...windows _open() & fopen() can now tolerate a mix-n-match of path delimiters; Implementations do change. Sasha, skip the #define thought. Thanks Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_db_files.c: add '/' path delimited
Sasha Khapyorsky wrote:
> Add '/' path delimiter unconditionally on file path generation.
Any chance you could make the path delimiter a #define so there is not an
explicit Linux filesystem assumption?
>
> Signed-off-by: Sasha Khapyorsky
> ---
> opensm/opensm/osm_db_files.c | 14 ++
> 1 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/opensm/opensm/osm_db_files.c
> b/opensm/opensm/osm_db_files.c
> index ec7436e..5fd7c83 100644
> --- a/opensm/opensm/osm_db_files.c
> +++ b/opensm/opensm/osm_db_files.c
> @@ -195,7 +195,7 @@ osm_db_domain_t *osm_db_domain_init(IN osm_db_t *
> p_db, IN char *domain_name) {
> osm_db_domain_t *p_domain;
> osm_db_domain_imp_t *p_domain_imp;
> - int dir_name_len;
> + size_t path_len;
> osm_log_t *p_log = p_db->p_log;
> FILE *p_file;
>
> @@ -209,16 +209,14 @@ osm_db_domain_t *osm_db_domain_init(IN osm_db_t
> * p_db, IN char *domain_name) (osm_db_domain_imp_t *)
> malloc(sizeof(osm_db_domain_imp_t)); CL_ASSERT(p_domain_imp !=
> NULL);
>
> - dir_name_len = strlen(((osm_db_imp_t *)
> p_db->p_db_imp)->db_dir_name); + path_len = strlen(((osm_db_imp_t *)
> p_db->p_db_imp)->db_dir_name) + + strlen(domain_name) + 2;
>
> /* set the domain file name */
> - p_domain_imp->file_name =
> - (char *)malloc(sizeof(char) * (dir_name_len) +
> strlen(domain_name) +
> -2);
> + p_domain_imp->file_name = malloc(path_len);
> CL_ASSERT(p_domain_imp->file_name != NULL);
> - strcpy(p_domain_imp->file_name,
> -((osm_db_imp_t *) p_db->p_db_imp)->db_dir_name);
> - strcat(p_domain_imp->file_name, domain_name);
> + snprintf(p_domain_imp->file_name, path_len, "%s/%s",
> + ((osm_db_imp_t *) p_db->p_db_imp)->db_dir_name, domain_name);
>
> /* make sure the file exists - or exit if not writable */
> p_file = fopen(p_domain_imp->file_name, "a+");
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] remove unnecessary leading '/' from filename
Sasha Khapyorsky wrote: > Hi Stan, > > On 15:18 Mon 12 Oct , Smith, Stan wrote: >> >> I believe removing the starting '/' from guid2lid filename is the >> correct fix for a couple of reasons: 1) prefixing a / to a filename >> seems strange when the path name is spec'ed as having a trailing / ? > > I agree. We need to care about making path properly rather than > introduce a restrictions for directory names definitions. > > Sasha Hello, OK on proper paths. What about the excellent point you raised in previous discussions about those pathnames which are input from environment vars which could be improperly formed? For badly formed paths from ENV vars (lacking trailing '/'), is it sufficient to let open() fail and output path/filename to osm log file? Stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] RE: [PATCH] opensm - use C99 transportable data type forpointer storage
Hefty, Sean wrote:
>> Did you send it before morning coffee or after ? :)
>>
>> Maybe, because it seems obvious.
>> All are agree with the elimination of unnecessary #ifndef's.
>> As to me, go ahead, commit it.
>
> It needs to be merged into the mgmt.git tree.
>
#include
+#include
#ifdef __cplusplus
# define BEGIN_C_DECLS extern "C" {
@@ -49,7 +50,7 @@
#endif /* __cplusplus */
BEGIN_C_DECLS
-#define st_ptr_t unsigned long
+#define st_ptr_t uintptr_t
>
> Does anyone know the reason for st_ptr_t at all? Why not use
> uintptr_t everywhere and avoid the define?
This is a good question. I avoided the issue due to the fact this patch has
been submitted before with no feedback so KIS is the game plan for now.
Once the uintptr_t concept has been accepted, then a 2nd step would be to
replace st_ptr_t with uintptr_t and remove the #define.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm - use C99 transportable data type for pointer storage
Hello,
Can you help me understand why this patch always seems to fall into a black
hole with no feedback?
Thank you,
Stan.
Stan C. Smith wrote:
> In order to skip the #ifndef __WIN__ around #include ,
> inttypes.h was used. 'inttypes.h' includes stdint.h and exists in the
> WinOF svn tree.
> OFED opensm builds without problems on EL 5.3.
>
> Signed-off-by: stan smith
>
> diff --git a/opensm/include/opensm/st.h b/opensm/include/opensm/st.h
> index 30cc308..ad6c289 100644
> --- a/opensm/include/opensm/st.h
> +++ b/opensm/include/opensm/st.h
> @@ -39,6 +39,7 @@
> #define ST_INCLUDED
>
> #include
> +#include
>
> #ifdef __cplusplus
> # define BEGIN_C_DECLS extern "C" {
> @@ -49,7 +50,7 @@
> #endif /* __cplusplus */
>
> BEGIN_C_DECLS
> -#define st_ptr_t unsigned long
> +#define st_ptr_t uintptr_t
> typedef st_ptr_t st_data_t;
>
> #define ST_DATA_T_DEFINED
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] contain pthreads defs with ifdef HAVE_LIBPTHREAD
Hefty, Sean wrote:
>> Contain pthread definitions with ifdef HAVE_LIBPTHREAD
>>
>> Signed-off-by: stan smith
>>
>> diff --git a/opensm/include/vendor/osm_vendor_ibumad.h
>> b/opensm/include/vendor/osm_vendor_ibumad.h
>> index 0a4692d..d523412 100644
>> --- a/opensm/include/vendor/osm_vendor_ibumad.h
>> +++ b/opensm/include/vendor/osm_vendor_ibumad.h
>> @@ -160,8 +160,13 @@ typedef struct _osm_vendor {
>> char ca_names[OSM_UMAD_MAX_CAS][UMAD_CA_NAME_LEN];
>> vendor_match_tbl_t mtbl; umad_port_t umad_port;
>> +#ifdef HAVE_LIBPTHREAD
>> pthread_mutex_t cb_mutex;
>> pthread_mutex_t match_tbl_mutex;
>> +#else
>> +cl_mutex_t cb_mutex;
>> +cl_mutex_t match_tbl_mutex;
>> +#endif
>
> complib is available on both platforms, why not just use it?
>
> Alternately, #define cl_mutex_t pthread_mutex_t and avoid the #ifdef's
Pthreads have been workin just fine, no need fix something that is not broken.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] Re: [PATCH] osmtest - Add OSM_CDECL to main() declaration
Hefty, Sean wrote: >>> Sean maintains a separate set of patches applied to IB diags in >>> order to >> address issues like the x86 requirement for __cdecl on main(). >>> Since OSM_CDECL was already defined in OFED opensm, it did not seem >>> to be a >> major concession to prefix main() with it. >>> Options: >>> 1) utilize OSM_CDECL >>> 2) throw an ugly ifdef __WIN__ or ifdef __linux__ around main() >>> 3) Create more Windows-only patches required for the Windows >>> implementation of OFED opensm & tools. >> >> Any options for windows compiler about call conventions? > > To clarify, I maintain a single patch to handle this for the signal() > callback handler, not the ib-diags main() functions. > > Here's what I use in the sources file for building the diags: > > MSC_WARNING_LEVEL = /W3 /WX /wd4007 > > - Sean Thanks for sharing the info at your earliest convenience. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] opensm - standardize on a single Windows #define
Smith, Stan wrote: > Sasha Khapyorsky wrote: >> On 10:16 Thu 08 Oct , Smith, Stan wrote: >>> Hefty, Sean wrote: >>>>> Code cleanup, standardize on a single Windows #define '__WIN__'; >>>>> WIN32 --> __WIN__. Flip usage of ifndef WIN32 --> ifdef __GNUC__ >>>> >>>> GNUC indicates a specific compiler, not a platform, which is what >>>> the check is for. >>> >>> To the best of my understanding OpenSM is built for Linux/OFED, >>> Solaris and Windows. >> >> Also some embedded platforms and we don't know what in a future. >> >> '#ifdef __WIN__' and '#ifndef __GNUC__' are not equivalent. >> >> Sasha > > You wish to live with the negative logic of ifndef __WIN__ ? > > How about ifdef __linux__ ? If __linux__ doesn't work for you, then please create a Linux Platform define I can use. > > > > ___ > ofw mailing list > o...@lists.openfabrics.org > http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osmtest - Add OSM_CDECL to main() declaration
Sasha Khapyorsky wrote:
> On 14:58 Mon 12 Oct , Smith, Stan wrote:
>>>
>>>> {
>>>> static osmtest_t osm_test;
>>>> osmtest_opt_t opt = { 0 };
>>
>> Sean maintains a separate set of patches applied to IB diags in
>> order to address issues like the x86 requirement for __cdecl on
>> main(). Since OSM_CDECL was already defined in OFED opensm, it did
>> not seem to be a major concession to prefix main() with it.
>> Options:
>> 1) utilize OSM_CDECL
>> 2) throw an ugly ifdef __WIN__ or ifdef __linux__ around main()
>> 3) Create more Windows-only patches required for the Windows
>> implementation of OFED opensm & tools.
>
> Any options for windows compiler about call conventions?
>
> Sasha
Sadly not for x86 architecture on Windows. Both Sean and I have danced around
this and come up empty.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osmtest/osmtest.c: remove strings.h inclusion
Sasha Khapyorsky wrote: > Remove redundant strings.h inclusion (and associated ifndef __WIN__). > > Signed-off-by: Sasha Khapyorsky > --- > opensm/osmtest/osmtest.c |3 --- > 1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/opensm/osmtest/osmtest.c b/opensm/osmtest/osmtest.c > index 3618ea3..f0f23c7 100644 > --- a/opensm/osmtest/osmtest.c > +++ b/opensm/osmtest/osmtest.c > @@ -46,9 +46,6 @@ > #include > #include > #include > -#ifndef __WIN__ > -#include > -#endif > #include > #include > #include "osmtest.h" Looking better all the time... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] opensm - standardize on a single Windows #define
Sasha Khapyorsky wrote: > On 10:16 Thu 08 Oct , Smith, Stan wrote: >> Hefty, Sean wrote: >>>> Code cleanup, standardize on a single Windows #define '__WIN__'; >>>> WIN32 --> __WIN__. Flip usage of ifndef WIN32 --> ifdef __GNUC__ >>> >>> GNUC indicates a specific compiler, not a platform, which is what >>> the check is for. >> >> To the best of my understanding OpenSM is built for Linux/OFED, >> Solaris and Windows. > > Also some embedded platforms and we don't know what in a future. > > '#ifdef __WIN__' and '#ifndef __GNUC__' are not equivalent. > > Sasha You wish to live with the negative logic of ifndef __WIN__ ? How about ifdef __linux__ ? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] remove unnecessary leading '/' from filename
Sasha Khapyorsky wrote:
> On 16:46 Wed 07 Oct , Stan C. Smith wrote:
>>
>> In osm_db_domain_init() the filename is appended to a path string
>> 'db_dir_name' which contains a trailing '/'. Remove extra '/'.
>>
>> Signed-off-by: stan smith
>>
>> diff --git a/opensm/opensm/osm_lid_mgr.c
>> b/opensm/opensm/osm_lid_mgr.c
>> index 947fe7a..1704054 100644
>> --- a/opensm/opensm/osm_lid_mgr.c
>> +++ b/opensm/opensm/osm_lid_mgr.c
>> @@ -239,7 +239,7 @@ ib_api_status_t osm_lid_mgr_init(IN
>> osm_lid_mgr_t * p_mgr, IN osm_sm_t * sm) p_mgr->p_lock =
>> sm->p_lock;
>>
>> /* we initialize and restore the db domain of guid to lid map */
>> -p_mgr->p_g2l = osm_db_domain_init(p_mgr->p_db, "/guid2lid");
>> +p_mgr->p_g2l = osm_db_domain_init(p_mgr->p_db, "guid2lid");
>
> I think that this can break when OSM_CACHE_DIR is supplied as
> environment variable and its value doesn't have '/' at the end. The
> patch below is from 2007 and it fixed this issue.
>
> Sasha
I believe removing the starting '/' from guid2lid filename is the correct fix
for a couple of reasons:
1) prefixing a / to a filename seems strange when the path name is spec'ed as
having a trailing / ?
2) / in a windows filename is a problem.
Due to comments in opensm/include/opensm/osm_base.h which explicitly speak to
the requirement for a trailing directory delimiter, the missing functionality
is the check for the trailing directory delimiter when using an environment var
to build a path. One could be a 'good' guy and if the env is missing the
trailing / then add one.
Do want me to resubmit with the env var check/fix or do you want to do it?
Stan.
>
>
> commit 907b95ec02d6a794ee8f6d6996cc6350a476bd15
> Author: Albert L. Chu
> Date: Wed Mar 14 07:08:58 2007 -0500
>
> OpenSM: Fix cache filename corner case
>
> Signed-off-by: Albert L. Chu
> Signed-off-by: Hal Rosenstock
>
> diff --git a/osm/opensm/osm_lid_mgr.c b/osm/opensm/osm_lid_mgr.c
> index 89adc48..bd5945f 100644
> --- a/osm/opensm/osm_lid_mgr.c
> +++ b/osm/opensm/osm_lid_mgr.c
> @@ -282,7 +282,7 @@ osm_lid_mgr_init(
>p_mgr->p_req = p_req;
>
>/* we initialize and restore the db domain of guid to lid map */
> - p_mgr->p_g2l = osm_db_domain_init(p_mgr->p_db, "guid2lid");
> + p_mgr->p_g2l = osm_db_domain_init(p_mgr->p_db, "/guid2lid");
>if (! p_mgr->p_g2l)
>{
> osm_log( p_mgr->p_log, OSM_LOG_ERROR,
> diff --git a/osm/opensm/osm_subnet.c b/osm/opensm/osm_subnet.c
> index 99e39aa..cbb3549 100644
> --- a/osm/opensm/osm_subnet.c
> +++ b/osm/opensm/osm_subnet.c
> @@ -745,7 +745,7 @@ osm_subn_rescan_conf_file(
> p_cache_dir = OSM_DEFAULT_CACHE_DIR;
>
>strcpy(file_name, p_cache_dir);
> - strcat(file_name, "opensm.opts");
> + strcat(file_name, "/opensm.opts");
>
>opts_file = fopen(file_name, "r");
>if (!opts_file)
> @@ -838,7 +838,7 @@ osm_subn_parse_conf_file(
> p_cache_dir = OSM_DEFAULT_CACHE_DIR;
>
>strcpy(file_name, p_cache_dir);
> - strcat(file_name, "opensm.opts");
> + strcat(file_name, "/opensm.opts");
>
>opts_file = fopen(file_name, "r");
>if (!opts_file) return;
> @@ -1096,7 +1096,7 @@ osm_subn_write_conf_file(
> p_cache_dir = OSM_DEFAULT_CACHE_DIR;
>
>strcpy(file_name, p_cache_dir);
> - strcat(file_name, "opensm.opts");
> + strcat(file_name, "/opensm.opts");
>
>opts_file = fopen(file_name, "w");
>if (!opts_file) return;
>
>> if (!p_mgr->p_g2l) {
>> OSM_LOG(p_mgr->p_log, OSM_LOG_ERROR, "ERR 0316: "
>> "Error initializing Guid-to-Lid persistent database\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osmtest - Add OSM_CDECL to main() declaration
Sasha Khapyorsky wrote:
> On 10:10 Mon 12 Oct , Stan C. Smith wrote:
>>
>>
>> Signed-off-by: stan smith
>>
>> diff --git a/opensm/osmtest/main.c b/opensm/osmtest/main.c
>> index 4bb9f82..287baf3 100644
>> --- a/opensm/osmtest/main.c
>> +++ b/opensm/osmtest/main.c
>> @@ -285,7 +285,7 @@ ib_net64_t get_port_guid(IN osmtest_t * p_osmt,
>> uint64_t port_guid)
>>
>> /**
>>
>> **/
>> -int main(int argc, char *argv[]) +int OSM_CDECL main(int argc, char
>> *argv[])
>
> Hmm, why is this needed? AFAIK most of infiniband-diags tools were
> ported to windows without adding such special declaration for main()
> functions.
>
> Sasha
>
>> {
>> static osmtest_t osm_test;
>> osmtest_opt_t opt = { 0 };
Sean maintains a separate set of patches applied to IB diags in order to
address issues like the x86 requirement for __cdecl on main().
Since OSM_CDECL was already defined in OFED opensm, it did not seem to be a
major concession to prefix main() with it.
Options:
1) utilize OSM_CDECL
2) throw an ugly ifdef __WIN__ or ifdef __linux__ around main()
3) Create more Windows-only patches required for the Windows implementation of
OFED opensm & tools.
Your call.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osmtest - code cleanup
Sasha Khapyorsky wrote: > On 11:57 Mon 12 Oct , Stan C. Smith wrote: >> >> Remove unused ifdef __WIN__ & redundant include. >> >> Signed-off-by: stan smith > > Applied. Thanks. The question is below. > >> >> diff --git a/opensm/osmtest/osmtest.c b/opensm/osmtest/osmtest.c >> index c6ec955..82a814e 100644 --- a/opensm/osmtest/osmtest.c >> +++ b/opensm/osmtest/osmtest.c >> @@ -43,19 +43,13 @@ >> * >> */ >> >> -#ifdef __WIN__ >> -#pragma warning(disable : 4996) >> -#endif >> - >> #include >> #include >> #include >> -#ifdef __WIN__ >> -#include >> -#else >> +#ifndef __WIN__ >> #include >> -#include >> #endif > > I suppose that windows should have string.h, right? Assuming so could > we remove '#ifndef __WIN__' completely here? Yes - I missed this in the linux env. According to the strings.h file, if string.h is included prior, then strings.h is a nop. Seems the include of strings.h could be removed along with the ifndef __WIN__. Good catch. How do you feel about replacing ifndef __WIN__ with ifdef __linux__ in other situations? > > Sasha > >> +#include >> #include >> #include "osmtest.h" -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] opensm - standardize on a single Windows #define
Hefty, Sean wrote: >> What would you suggest, given you did not like the ifndef __WIN__ in >> terms of readability? > > I was saying that it is slightly easier for me to read: > > #ifdef __WIN__ > // windows stuff > #else // <- not __WIN__ > // other stuff > #endif > > versus > > #ifndef __WIN_ > // other stuff > #else // <- not not __WIN__ > // windows stuff > #endif > > I don't have a problem with the use of __WIN__ or > > #ifndef __WIN__ > // other stuff > #endif There is a fair amount of code hoisting to flip the ifndef __WIN__ --> ifdef __WIN__ for readability reasons. There are bigger fish to fry at this juncture. Perhaps into the future. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] opensm - standardize on a single Windows #define
Hefty, Sean wrote: >> Code cleanup, standardize on a single Windows #define '__WIN__'; >> WIN32 --> __WIN__. Flip usage of ifndef WIN32 --> ifdef __GNUC__ > > GNUC indicates a specific compiler, not a platform, which is what the > check is for. To the best of my understanding OpenSM is built for Linux/OFED, Solaris and Windows. What would you suggest, given you did not like the ifndef __WIN__ in terms of readability? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osm_subnet.c
Sasha Khapyorsky wrote:
> Hi Stan,
>
> On 14:35 Thu 01 Oct , Smith, Stan wrote:
>>
>> Set function return type to match that of cl_pfn_fmap_cmp_t to align
>> with how the function is invoked.
>>
>> Signed-off-by: stan smith
>>
>> --- a/opensm/opensm/osm_subnet.c2009-10-01
>> 12:45:52.0 -0700 +++ b/opensm/opensm/osm_subnet.c
>> 2009-10-01 14:24:18.0 -0700 @@ -397,7 +397,7 @@
>>
>> /**
>>
>> **/
>> -static long compar_mgids(const void *m1, const void *m2) +static
>> intn_t compar_mgids(const void *m1, const void *m2)
>
> Any disagreement about changing a prototype of this method
> (cl_pfn_fmap_cmp_t) in complib to use standard type (long) instead of
> "homemade" and less clear one ('intn_t')?
>
> Sasha
>
>> {
>> return memcmp(m1, m2, sizeof(ib_gid_t));
>> }
How is the processor architecture 'natural' size intn_t less clear than 'long'?
In some worlds sizeof(long) != sizeof(void*).
I would prefer leaving the definition as intn_t as that's the way it has been
defined for 'years' (think application code not necessarily OFED) which is
coded to the intn_t definition.
In the Windows world, cl_fleximap.h is not just an OpenSM utilized file, more
like something that would live in /usr/include.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] typeof() not supported in Windows WDK compiler
Hello Sasha,
Please skip the patch titled 'typeof() not supported in Windows WDK compiler'
as Jason's fix does the job thus requiring no src code changes.
Thank you Jason.
Stan.
Jason Gunthorpe wrote:
> On Thu, Oct 01, 2009 at 03:59:00PM -0700, Sean Hefty wrote:
>>> diff --git a/opensm/opensm/osm_drop_mgr.c
>>> b/opensm/opensm/osm_drop_mgr.c index 4f98cc9..8fe5129 100644 +++
>>> b/opensm/opensm/osm_drop_mgr.c @@ -209,8 +209,13 @@ static void
>>> drop_mgr_remove_port(osm_sm_t * sm, IN osm_port_t * p_port)
>>> drop_mgr_clean_physp(sm, p_port->p_physp);
>>>
>>> while (!cl_is_qlist_empty(&p_port->mcm_list)) {
>>> +#ifndef __WIN__
>>> mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>>>mcm_port, list_item);
>>> +#else
>>> + mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>>> + mcm_port, list_item, (osm_mcm_port_t*) );
>>> +#endif
>>
>> I'd find this more readable if it were #ifdef rather than #ifndef.
>>
>> That said, I've got to believe that there's a better way to handle
>> these changes. I just don't know what it is.
>
> Hmm
>
> How about
>
> #ifdef __GCC_
> #define cl_item_obj(item_ptr, obj_ptr, item_field) (typeof(obj_ptr)) \
> ((void *)item_ptr - (unsigned
> long)&((typeof(obj_ptr))0)->item_field) #else
> #define cl_item_obj(item_ptr, obj_ptr, item_field) (typeof(obj_ptr)) \
> (void *)((uint8_t*)item_ptr - ((uintptr_t)(&(obj_ptr)->item_field) -
> ((uintptr_t)&(obj_ptr
> #endif
>
> And rely on the C implicit cast from void* to an object type, and rely
> on compiles with gcc to do the extra type checking.
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] typeof() not supported in Windows WDK compiler
Jason Gunthorpe wrote:
> On Thu, Oct 01, 2009 at 03:59:00PM -0700, Sean Hefty wrote:
>>> diff --git a/opensm/opensm/osm_drop_mgr.c
>>> b/opensm/opensm/osm_drop_mgr.c index 4f98cc9..8fe5129 100644 +++
>>> b/opensm/opensm/osm_drop_mgr.c @@ -209,8 +209,13 @@ static void
>>> drop_mgr_remove_port(osm_sm_t * sm, IN osm_port_t * p_port)
>>> drop_mgr_clean_physp(sm, p_port->p_physp);
>>>
>>> while (!cl_is_qlist_empty(&p_port->mcm_list)) {
>>> +#ifndef __WIN__
>>> mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>>>mcm_port, list_item);
>>> +#else
>>> + mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>>> + mcm_port, list_item, (osm_mcm_port_t*) );
>>> +#endif
>>
>> I'd find this more readable if it were #ifdef rather than #ifndef.
>>
>> That said, I've got to believe that there's a better way to handle
>> these changes. I just don't know what it is.
>
> Hmm
>
> How about
Do the Sun guys use gcc?
>
> #ifdef __GCC_
> #define cl_item_obj(item_ptr, obj_ptr, item_field) (typeof(obj_ptr)) \
> ((void *)item_ptr - (unsigned
> long)&((typeof(obj_ptr))0)->item_field) #else
> #define cl_item_obj(item_ptr, obj_ptr, item_field) (typeof(obj_ptr)) \
> (void *)((uint8_t*)item_ptr - ((uintptr_t)(&(obj_ptr)->item_field) -
> ((uintptr_t)&(obj_ptr
> #endif
Interesting, worth a try.
>
> And rely on the C implicit cast from void* to an object type, and rely
> on compiles with gcc to do the extra type checking.
'compiles with gcc' ?
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [ofw] [PATCH] typeof() not supported in Windows WDK compiler
Hefty, Sean wrote:
>> diff --git a/opensm/opensm/osm_drop_mgr.c
>> b/opensm/opensm/osm_drop_mgr.c index 4f98cc9..8fe5129 100644 ---
>> a/opensm/opensm/osm_drop_mgr.c +++ b/opensm/opensm/osm_drop_mgr.c
>> @@ -209,8 +209,13 @@ static void drop_mgr_remove_port(osm_sm_t * sm,
>> IN osm_port_t * p_port) drop_mgr_clean_physp(sm, p_port->p_physp);
>>
>> while (!cl_is_qlist_empty(&p_port->mcm_list)) {
>> +#ifndef __WIN__
>> mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>> mcm_port, list_item);
>> +#else
>> +mcm_port = cl_item_obj(cl_qlist_head(&p_port->mcm_list),
>> + mcm_port, list_item, (osm_mcm_port_t*) );
>> +#endif
>
> I'd find this more readable if it were #ifdef rather than #ifndef.
So would I, although the convention is to only define something extra for
Windows.
Perhaps a gcc/cpp defined item could be recommended and agreed upon?
>
> That said, I've got to believe that there's a better way to handle
> these changes. I just don't know what it is.
Agreed, I'm listening.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] C++ style coding does not compile
Move C++ in-stream var declarations to top of {} block as in C style; Var
assignments left in place.
MSFT compiler errors out with Missing ';' before 'type', 'Illegal use of this
type osm_prefix_route_t as an expression'?
Signed-off-by: stan smith
diff --git a/opensm/opensm/osm_sa_path_record.c
b/opensm/opensm/osm_sa_path_record.c
index 2247ebe..1166def 100644
--- a/opensm/opensm/osm_sa_path_record.c
+++ b/opensm/opensm/osm_sa_path_record.c
@@ -1214,6 +1214,10 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
if (!ib_gid_is_multicast(&p_pr->dgid) &&
ib_gid_get_subnet_prefix(&p_pr->dgid) !=
sa->p_subn->opt.subnet_prefix) {
+
+ osm_prefix_route_t *route;
+ osm_prefix_route_t *r;
+
OSM_LOG(sa->p_log, OSM_LOG_VERBOSE,
"Non local DGID subnet prefix 0x%016"
PRIx64 "\n",
@@ -1221,11 +1225,9 @@ static ib_net16_t pr_rcv_get_end_points(IN osm_sa_t * sa,
/* Find the router port that is configured to
handle this prefix, if any */
- osm_prefix_route_t *route = NULL;
- osm_prefix_route_t *r = (osm_prefix_route_t *)
- cl_qlist_head(&sa->p_subn->
- prefix_routes_list);
-
+ route = NULL;
+ r = (osm_prefix_route_t *) cl_qlist_head(
+ &sa->p_subn->prefix_routes_list);
while (r != (osm_prefix_route_t *)
cl_qlist_end(&sa->p_subn->
prefix_routes_list)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] osm_helper.c match func to proto
Sasha Khapyorsky wrote: > On 13:16 Thu 01 Oct , Smith, Stan wrote: >> >> Removed inet_ntop() (void*) cast per discussions; only prototype >> updates. >> >> Signed-off-by: stan smith > > Just pushed out "'const' removing" patch few minutes before. Maybe it > will just work for you. > > Sasha Yes it did just work/compile for the changes in osm_helper.c & osm_helper.h. Please advise when other .h files & friends are adjusted. Stan. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] osm_subnet.c change void* arithmetic to char*
MS compiler doesn't Grok void* arithmetic, use char* Signed-off-by: stan smith --- a/opensm/opensm/osm_subnet.c2009-10-01 12:45:52.0 -0700 +++ b/opensm/opensm/osm_subnet.c2009-10-01 14:24:18.0 -0700 @@ -1193,8 +1193,8 @@ if (strcmp(r->name, p_key)) continue; - p_field1 = (void *)p_opts->file_opts + r->opt_offset; - p_field2 = (void *)p_opts + r->opt_offset; + p_field1 = (char *)p_opts->file_opts + r->opt_offset; + p_field2 = (char *)p_opts + r->opt_offset; /* don't call setup function first time */ r->parse_fn(NULL, p_key, p_val, p_field1, p_field2, NULL); @@ -1253,8 +1253,8 @@ if (!r->can_update || strcmp(r->name, p_key)) continue; - p_field1 = (void *)p_opts->file_opts + r->opt_offset; - p_field2 = (void *)p_opts + r->opt_offset; + p_field1 = (char *)p_opts->file_opts + r->opt_offset; + p_field2 = (char *)p_opts + r->opt_offset; r->parse_fn(p_subn, p_key, p_val, p_field1, p_field2, r->setup_fn); break; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] osm_subnet.c
Set function return type to match that of cl_pfn_fmap_cmp_t to align with how
the function is invoked.
Signed-off-by: stan smith
--- a/opensm/opensm/osm_subnet.c2009-10-01 12:45:52.0 -0700
+++ b/opensm/opensm/osm_subnet.c2009-10-01 14:24:18.0 -0700
@@ -397,7 +397,7 @@
/**
**/
-static long compar_mgids(const void *m1, const void *m2)
+static intn_t compar_mgids(const void *m1, const void *m2)
{
return memcmp(m1, m2, sizeof(ib_gid_t));
}
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] osm_helper.c match func to proto
Removed inet_ntop() (void*) cast per discussions; only prototype updates.
Signed-off-by: stan smith
diff --git a/opensm/opensm/osm_helper.c b/opensm/opensm/osm_helper.c
index ea9e21f..f4e365b 100644
--- a/opensm/opensm/osm_helper.c
+++ b/opensm/opensm/osm_helper.c
@@ -780,10 +780,11 @@ static void dbg_get_capabilities_str(IN char *p_buf, IN
const uint32_t buf_size,
/**
**/
-void osm_dump_port_info(IN osm_log_t * p_log, IN const ib_net64_t node_guid,
+void osm_dump_port_info(IN osm_log_t * const p_log,
+ IN const ib_net64_t node_guid,
IN const ib_net64_t port_guid,
IN const uint8_t port_num,
- IN const ib_port_info_t * p_pi,
+ IN const ib_port_info_t * const p_pi,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -870,8 +871,8 @@ void osm_dump_port_info(IN osm_log_t * p_log, IN const
ib_net64_t node_guid,
/**
**/
-void osm_dump_portinfo_record(IN osm_log_t * p_log,
- IN const ib_portinfo_record_t * p_pir,
+void osm_dump_portinfo_record(IN osm_log_t * const p_log,
+ IN const ib_portinfo_record_t * const p_pir,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -956,8 +957,8 @@ void osm_dump_portinfo_record(IN osm_log_t * p_log,
/**
**/
-void osm_dump_guidinfo_record(IN osm_log_t * p_log,
- IN const ib_guidinfo_record_t * p_gir,
+void osm_dump_guidinfo_record(IN osm_log_t * const p_log,
+ IN const ib_guidinfo_record_t * const p_gir,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -995,7 +996,8 @@ void osm_dump_guidinfo_record(IN osm_log_t * p_log,
/**
**/
-void osm_dump_node_info(IN osm_log_t * p_log, IN const ib_node_info_t * p_ni,
+void osm_dump_node_info(IN osm_log_t * const p_log,
+ IN const ib_node_info_t * const p_ni,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -1030,8 +1032,8 @@ void osm_dump_node_info(IN osm_log_t * p_log, IN const
ib_node_info_t * p_ni,
/**
**/
-void osm_dump_node_record(IN osm_log_t * p_log,
- IN const ib_node_record_t * p_nr,
+void osm_dump_node_record(IN osm_log_t * const p_log,
+ IN const ib_node_record_t * const p_nr,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -1080,7 +1082,8 @@ void osm_dump_node_record(IN osm_log_t * p_log,
/**
**/
-void osm_dump_path_record(IN osm_log_t * p_log, IN const ib_path_rec_t * p_pr,
+void osm_dump_path_record(IN osm_log_t * const p_log,
+ IN const ib_path_rec_t * const p_pr,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -1129,8 +1132,8 @@ void osm_dump_path_record(IN osm_log_t * p_log, IN const
ib_path_rec_t * p_pr,
/**
**/
-void osm_dump_multipath_record(IN osm_log_t * p_log,
- IN const ib_multipath_rec_t * p_mpr,
+void osm_dump_multipath_record(IN osm_log_t * const p_log,
+ IN const ib_multipath_rec_t * const p_mpr,
IN const osm_log_level_t log_level)
{
if (osm_log_is_active(p_log, log_level)) {
@@ -1193,7 +1196,8 @@ void osm_dump_multipath_record(IN osm_log_t * p_log,
/**
**/
-void osm_dump_mc_record(IN osm_log_t * p_log, IN const ib_member_rec_t *
p_mcmr,
+void osm_dump_mc_record(IN osm_log_t * const p_log,
+ IN const ib_member_rec_t * const p_mcmr,
IN con
RE: [OPENSM] match functions to prototypes in header file
Sasha Khapyorsky wrote:
>> @@ -1080,7 +1082,8 @@ void osm_dump_node_record(IN osm_log_t * p_log,
>>
>> /**
>>
>> **/
>> -void osm_dump_path_record(IN osm_log_t * p_log, IN const
>> ib_path_rec_t * p_pr, +void osm_dump_path_record(IN osm_log_t *
>>const p_log, + IN const
>> ib_path_rec_t * const p_pr, IN
>> const osm_log_level_t log_level) {
>> if (osm_log_is_active(p_log, log_level)) {
>> @@ -1106,9 +1109,9 @@ void osm_dump_path_record(IN osm_log_t *
>> p_log, IN const ib_path_rec_t * p_pr,
>> "\t\t\t\tresv2...0x%X\n"
>> "\t\t\t\tresv3...0x%X\n",
>> cl_ntoh64(p_pr->service_id), - inet_ntop(AF_INET6,
>> p_pr->dgid.raw,
>> gid_str, + inet_ntop(AF_INET6, (void*)p_pr->dgid.raw,
>> gid_str,
>
> And why is such casting(s) needed?
The '(void *)' on inet_ntop() was the only way I could get the MS compiler
warnings about differing const types to go away;
(const void *) failed.
Since suppression of information is the preferred modus operandi, I'll see if I
can figure out a way to suppress the warning.
>
> Also wouldn't it be simpler to remove 'const' in "type * const var"
> function parameter definitions? This restricts only value of a pointer
> (not structure content), and since function parameters are passed by
> values such restriction is only related to a function implementation
> and actually meanless in sense of API. Thoughts?
I'm just the messenger here, making the function match the prototype.
Fine by me if you choose to change both.
Stan.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [OPENSM] cast to remove warnings about signed vs. unsigned comparisons
Jason Gunthorpe wrote: > On Thu, Oct 01, 2009 at 09:14:50AM -0700, Smith, Stan wrote: > >> Upcoming patches do contain a fair amount of casting for those >> assignment cases where IB defined fields uint8_t/uint16_t are >> assiged from larger sized variable. Will examine code in further >> detail favoring var declaration changes for signed/unsigned >> comparisions. W.r.t. opensm/* files, the end is in sight. There are >> 18 more patch files coming, will reinspect for less casting. > > I've always felt the warning on implicit cast from larger to smaller > type to be somewhat useless - that is an unavoidable operation when > working with networking. Adding casts does nothing to actually improve > the code, and trying to change to smaller types results in worse code > gen and no improvement in function. > > Maybe OFW would be better off just turning that warning off? :) Perhaps I'm old-school, in that I'd rather see all the details/warnings which forces a closer examination of the situation. Over the years I've discovered that ignoring information leads to long debug sessions... The thing about casting is you can read the code and see what's going on 'in context' without having to look someplace else (.h file) or perhaps make erroneous asumptions. That said, dealing with overzealous hand-holding compilers is never pleasant. Will consider suppression of warning information. > >>> In general to simplify a detection of such cases we can consider to >>> use -Wsign-compare gcc flag in linux environment. > > And -Wsign-conversion > > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] opensm/osm_helper.c: fix compiler warning
Sasha Khapyorsky wrote:
> Eliminate compiler warning:
>
> osm_helper.c:551: warning: missing braces around initializer
> osm_helper.c:551: warning: (near initialization for 'ib_zero_gid.raw')
>
> Signed-off-by: Sasha Khapyorsky
> ---
> opensm/opensm/osm_helper.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/opensm/opensm/osm_helper.c b/opensm/opensm/osm_helper.c
> index 35da131..ea9e21f 100644
> --- a/opensm/opensm/osm_helper.c
> +++ b/opensm/opensm/osm_helper.c
> @@ -548,7 +548,7 @@ const char *ib_get_trap_str(ib_net16_t trap_num)
> return "Unknown";
> }
>
> -const ib_gid_t ib_zero_gid = { 0 };
> +const ib_gid_t ib_zero_gid = { { 0 } };
>
> /**
>
> **/
OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [OPENSM] cast to remove warnings about signed vs. unsigned comparisons
Sasha Khapyorsky wrote:
> On 13:30 Wed 30 Sep , Stan C. Smith wrote:
>>
>> Use (unsigned) cast to remove compiler warnings for signed component
>> in comparison (for loops) . In a couple of cases use unsigned
>> instead of int for the variable declaration.
>>
>> Signed-off-by: Stan Smith
>
> Applied. Thanks.
>
> Basically I prefer to avoid castings when possible - much better is to
> use proper types. So I'm adding the patch below, hope that it is fine
> for you.
Hello,
I agree, avoiding casting when possible is a much better approach. Currently
I have been attempting to minimize code changes and leaning towards cosmetic
casting in lieu of potentially breaking working code.
You know the code base far better than I, these patches are for review from
eyes with more OpenSM experience.
Upcoming patches do contain a fair amount of casting for those assignment cases
where IB defined fields uint8_t/uint16_t are assiged from larger sized
variable. Will examine code in further detail favoring var declaration changes
for signed/unsigned comparisions.
W.r.t. opensm/* files, the end is in sight. There are 18 more patch files
coming, will reinspect for less casting.
Stan.
>
> In general to simplify a detection of such cases we can consider to
> use -Wsign-compare gcc flag in linux environment.
>
> Sasha
>
>
> commit 3b30f3b1311b44f3e2042ea2c4e180ffa8291532
> Author: Sasha Khapyorsky
> Date: Thu Oct 1 17:50:04 2009 +0200
>
> opensm/osm_mesh.c: remove some castings
>
> Instead of casting for preventing different sign comparison
> warnings use unsigned types for affected variables.
>
> Signed-off-by: Sasha Khapyorsky
>
> diff --git a/opensm/opensm/osm_mesh.c b/opensm/opensm/osm_mesh.c
> index 8235e55..e5c53d9 100644
> --- a/opensm/opensm/osm_mesh.c
> +++ b/opensm/opensm/osm_mesh.c
> @@ -58,7 +58,7 @@
> static const struct mesh_info {
> int dimension; /* dimension of the torus */
> int size[MAX_DIMENSION];/* size of the torus */
> - int degree; /* degree of polynomial */
> + unsigned int degree;/* degree of polynomial */
> int poly[MAX_DEGREE+1]; /* polynomial */
> } mesh_info[] = {
> {0, {0}, 0, {0}, },
> @@ -263,7 +263,7 @@ static char *poly_print(int n, int *coeff)
> *
> * return a nonzero value if polynomials differ else 0
> */
> -static int poly_diff(int n, const int *p, switch_t *s)
> +static int poly_diff(unsigned int n, const int *p, switch_t *s)
> {
> if (s->node->num_links != n)
> return 1;
> @@ -591,13 +591,13 @@ static int get_switch_metric(lash_t *p_lash,
> int sw) {
> osm_log_t *p_log = &p_lash->p_osm->log;
> int ret = -1;
> - int i, j, change;
> + unsigned int i, j, change;
> int sw1, sw2, sw3;
> switch_t *s = p_lash->switches[sw];
> switch_t *s1, *s2, *s3;
> int **m;
> mesh_node_t *node = s->node;
> - int num_links = node->num_links;
> + unsigned int num_links = node->num_links;
>
> OSM_LOG_ENTER(p_log);
>
> @@ -622,7 +622,7 @@ static int get_switch_metric(lash_t *p_lash, int
> sw) s2 = p_lash->switches[sw2];
> if (s2->node->temp == LARGE)
> continue;
> - for (j = 0; (unsigned)j <
> s2->node->num_links; j++) {
> + for (j = 0; j < s2->node->num_links;
> j++) {
> sw3 =
> s2->node->links[j]->switch_id;
> s3 = p_lash->switches[sw3];
>
> @@ -925,8 +925,8 @@ static void make_geometry(lash_t *p_lash, int sw)
> int num_switches = p_lash->num_switches;
> int sw1, sw2;
> switch_t *s, *s1, *s2, *seed;
> - int i, j, k, l, n, m;
> - int change;
> + unsigned int i, j, k, l, n, m;
> + unsigned int change;
>
> OSM_LOG_ENTER(p_log);
>
> @@ -956,7 +956,7 @@ static void make_geometry(lash_t *p_lash, int sw)
> /*
>* ignore chain fragments
>*/
> - if ((unsigned)n < seed->node->num_links && n <= 2)
> + if (n < seed->node->num_links && n <= 2)
> continue;
>
> /*
> @@ -1068,11 +1068,11 @@ static void make_geometry(lash_t *p_lash, int
> sw)
>* find switch (other than s1) that
> neighbors i and j
>* have in common
>*/
> - for (k = 0; (unsigned)k <
> s1->node->num_links; k++) {
> + for (k = 0; k < s1->node->num_links;
> k++) {
> if
> (s1->node->links
