Hi James,
>Due to the errors I had some trouble looking over your changes. Can
>you resend?
Will do.
>One high level comment. I would prefer to see the DAT_MEM_TYPE_IA
>support added in new function called dapl_lmr_create_ia rather than
>overloading the dapl_lmr_create_physical function. Adding a
>dapl_lmr_create_ia function would be more in keeping with the existing
>structure.
I think that we can save duplicated copy-pased-code if we stick to
one function.
> + array = (u64 *)phys_addr.for_array; /* need to add for_u64_array to
> union */
>What does this comment mean?
I think the right way to do it is :
array = phys_addr.for_u64_array
(Givven the union consists of a new type u64* called "for_u64_array")
> - " ib_query_mr error code return from ib_query_mr
> = %d\n",
> - status);
> + "%s: ib_query_mr error code return from
> ib_query_mr = %d\n",
> + __FUNCTION__, status);
>We try to keep lines less than 80 characters. Is there a more concise
>way to convey the message above? In this case the original message
>appears to be poorly written.
I will try to fix it to 80 chars long.
> + u32 *lmr_context,
> + u32 *rmr_context,
>Why remove the DAT_LMR_CONTEXT and DAT_RMR_CONTEXT types? I would
>prefer to remove everyone of them at once.
Ok, no problem.
seemed like a good idea while I was there already .. :)
>Something is strange here. The call to dapl_ib_mr_register_physical is
>inside an else clause in the current code.
> + if (DAT_MEM_TYPE_IA == mem_type) {
> + status = dapl_ib_mr_register_ia(ia, new_lmr, phys_addr,
> + page_count, privileges);
> + }
> + else {
> + status = dapl_ib_mr_register_physical(ia, new_lmr, phys_addr,
> + page_count,
> privileges);
> + }
This was done to allow DAT_MEM_TYPE_PLATFORM behave as PHYSICAL as well.
The reason is only temporary and propriatery. I don't mind changing
the else with an explicit "if"
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
