[openib-general] Re: [openib-commits] r2426 - gen2/users/jlentini/linux-kernel/dat-provider
James Lentini
jlentini at netapp.com
Mon May 23 07:32:12 PDT 2005
Bernhard,
Thank you for pointing these items out. Comments below:
On Sat, 21 May 2005, Bernhard Fischer wrote:
> On Fri, May 20, 2005 at 03:05:08PM -0700, jlentini at openib.org wrote:
>> Author: jlentini
>> Date: 2005-05-20 15:05:07 -0700 (Fri, 20 May 2005)
>> New Revision: 2426
>
> Just tersely parsing, no mind to delve into any API implications
> whatsoever. [Late, no motivation to think, nor even read, not even
> speaking of writing anything publically. I'm over quota with my
> telephone-IP provider since 11th, which nag's our scanty pocket-money
> crucially, etc]
>
>> Modified: gen2/users/jlentini/linux-kernel/dat-provider/dapl.h
>> ===================================================================
>> --- gen2/users/jlentini/linux-kernel/dat-provider/dapl.h 2005-05-20 17:22:17 UTC (rev 2425)
>> +++ gen2/users/jlentini/linux-kernel/dat-provider/dapl.h 2005-05-20 22:05:07 UTC (rev 2426)
>> @@ -144,6 +140,24 @@
>> * *
>> *********************************************************************/
>>
>> +typedef void (*ib_async_handler_t) (struct ib_event *, void *);
>> +
>> +/*
>> + * ib_hca_transport_t structure. Define fields specific to this
>> + * provider implementation necessary to maintain the HCA
>> + *
>> + * NOTE: null_ib_hca_handle is strictly internal to IB. DAPL allows a
>> + * NULL DTO EVD handle, but IB does not. Create a CQ under the
>> + * hood and make sure an error is generated if the user every
> "every tries to post"? -enoparse; please be more specific -- simpler.
> 'ever' would still be obfuscated. IOW please rephrase and put
> abbreviations into the wiki.
Agreed. I'll rephrase that.
I can update the WIKI with DAPL acronyms.
>> + * tries to post, by setting the WQ length to 0 in ep_create
>> + * and/or ep_modify.
>> + */
>> @@ -394,7 +403,7 @@
>>
>> /* maintenence fields */
> maintenance.
> also in:
> users/jlentini/userspace/dapl/include/dapl.h
>> boolean_t listening; /* PSP is registered & active */
>> - ib_cm_srvc_handle_t cm_srvc_handle; /* Used by CM */
>> + struct ib_cm_id *cm_srvc_handle; /* Used by CM */
>> DAPL_LLIST_HEAD cr_list_head; /* CR pending queue */
>> int cr_list_count; /* count of CRs on queue */
>> };
>> --- gen2/users/jlentini/linux-kernel/dat-provider/dapl_evd_util.c 2005-05-20 17:22:17 UTC (rev 2425)
>> +++ gen2/users/jlentini/linux-kernel/dat-provider/dapl_evd_util.c 2005-05-20 22:05:07 UTC (rev 2426)
>> @@ -331,22 +322,22 @@
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> "\t dapl_evd_dto_callback : CQE \n");
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> - "\t\t work_req_id %lli\n", DAPL_GET_CQE_WRID(cqe_ptr));
>> - if (DAPL_GET_CQE_STATUS(cqe_ptr) == 0) {
>> - if (DAPL_GET_CQE_OPTYPE(cqe_ptr) == OP_RECEIVE) {
>> + "\t\t work_req_id %lli\n", cqe_ptr->wr_id);
> [[:space:]] --------------^ ?
Which space are you referring to? The one between \t\t and
work_req_id?
>> + if (cqe_ptr->status == 0) {
>> + if (cqe_ptr->opcode == IB_WC_RECV) {
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> - "\t\t op_type: OP_RECEIVE\n");
>> + "\t\t op_type: IB_WC_RECV\n");
> ditto
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> "\t\t bytes_num %d\n",
> ditto
>> - DAPL_GET_CQE_BYTESNUM(cqe_ptr));
>> + cqe_ptr->byte_len);
>> } else {
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> "\t\t op_type: %s\n",
> ditto
>> - optable[DAPL_GET_CQE_OPTYPE(cqe_ptr)]);
>> + optable[cqe_ptr->opcode]);
>> }
>> }
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> - "\t\t status %d\n", DAPL_GET_CQE_STATUS(cqe_ptr));
>> + "\t\t status %d\n", cqe_ptr->status);
> ditto
>> dapl_dbg_log(DAPL_DBG_TYPE_CALLBACK,
>> "\t >>>>>>>>>>>>>>>>>>>>>>><<<<<<<<<<<<<<<<<<<\n");
> fooooooooooooooooooooooooooooooooooooooooooobar!
> merge errors?
No that text, ">>>>>>>>>>>>>>>>>>>>>>><<<<<<<<<<<<<<<<<<<" is actually
part of a debugging print out. I agree that these debug statements are
of debatable value. If a high volume of events are being generated,
the amount of data produced by these is difficult to use.
> also in:
> gen2/branches/shaharf-ibat/src/userspace/management/libibmad/src/rpc.c
> gen2/ulps/iser/make.conf
> gen2/users/jlentini/linux-kernel/dat-provider/dapl_evd_util.c (multiple
> times)
> gen2/users/jlentini/linux-kernel/dat-provider/dapl_rsp_free.c (multiple
> gen2/users/jlentini/linux-kernel/dat-provider/dapl_rsp_create.c
> times)
> gen2/users/jlentini/linux-kernel/dat-provider/dapl_openib_cm.c (multiple
> times)
> gen2/users/jlentini/userspace/dapl/common/dapl_evd_util.c (ditto)
> gen2/users/jlentini/userspace/dapl/common/dapl_rsp_free.c (ditto)
> gen2/users/jlentini/userspace/dapl/common/dapl_rsp_create.c (ditto)
> gen2/trunk/src/userspace/management/libibmad/src/rpc.c
>
>> #endif
>> @@ -667,21 +657,21 @@
>>
>> #ifdef DAPL_DBG
>> /* Current gen2 mthca is not setting the opcode in seccesful cqe */
>> - /* The opcode will be OP_SEND or OP_RECEIVE acording the is_send bit */
>> + /* The opcode will be IB_WC_SEND or IB_WC_RECV acording the is_send bit */
> "acording" -enoparse: s/acord/accord/g
>
> also in:
> gen2/branches/shaharf-ibat/src/userspace/management/osm/opensm/osm_state_mgr.c
> gen2/branches/roland-uverbs/src/userspace/management/osm/opensm/osm_sa_mcmember_record.c
> gen2/utils/src/linux-user/ibdm/datamodel/ibdm.i
> gen2/utils/src/linux-user/IBMgtSim/src/ibdm.i
> gen2/utils/src/linux-user/IBMgtSim/utils/RunSimTest
> there: callabcks: -enoparse;
> continue with acord:
> gen2/users/jlentini/linux-kernel/dat-provider/dapl_evd_util.c
> gen2/trunk/src/userspace/management/osm/opensm/osm_state_mgr.c
I've fixed my error:
gen2/users/jlentini/linux-kernel/dat-provider/dapl_evd_util.c
The others belong to Roland, Shahar, and Eitan.
>
> "calla" -enoparse: s/calla/call
> gen2/utils/src/linux-user/IBMgtSim/utils/RunSimTest
> gen2/ulps/iser/include/iser_api.h
> gen2/trunk/src/userspace/management/osm/libvendor/osm_vendor_mlx_ts.c
> gen2/trunk/src/userspace/management/osm/libvendor/osm_vendor_ts.c
> gen2/trunk/src/userspace/management/osm/libvendor/osm_vendor_mlx_ts_anafa.c
>
> [If needed (i.e. if i'm aware of any request to do so) i'll expand those..]
Ditto.
>
>> /* We can not check the following assert for now */
> [skip]
>> @@ -184,19 +184,8 @@
>> }
>> }
>> /* force the EP into error state to force flush all posted DTOs. */
>> - {
>> - struct dat_ep_attr ep_attr;
>> - struct dat_named_attr ep_state;
>> + (void) dapl_modify_qp_state_to_error(ep_ptr->qp_handle);
>
> sun will hopefuly fix all of
> egrep -ri "(^[[:space:]]*\(void\))"
> openib.gen2/upstream/gen2/trunk/|egrep -v "svn-(text|base)"
> so i won't comment on that single occurance above ;)
I didn't realize the convention was to not have spaces before a cast.
This isn't in Documentation/CodingStyle. I know I've been adding these
here and there. I'll fix these as I see them.
More information about the general
mailing list