[openib-general][PATCH][kdapl]: adding DAT_MEM_TYPE_IA support

Guy German guyg at voltaire.com
Thu Jul 28 16:21:47 PDT 2005


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"




More information about the general mailing list