[openib-general][PATCH][kdapl]: adding DAT_MEM_TYPE_IA support
James Lentini
jlentini at netapp.com
Thu Jul 28 10:59:45 PDT 2005
Thanks for working on this Guy.
I received these errors when I tried to apply this patch:
patching file ib/dapl_openib_util.c
Hunk #1 FAILED at 234.
Hunk #2 FAILED at 281.
Hunk #3 FAILED at 300.
Hunk #4 FAILED at 309.
Hunk #5 FAILED at 318.
5 out of 5 hunks FAILED -- saving rejects to file
ib/dapl_openib_util.c.rej
patching file ib/dapl_openib_util.h
Hunk #1 FAILED at 92.
1 out of 1 hunk FAILED -- saving rejects to file
ib/dapl_openib_util.h.rej
patching file ib/dapl_lmr.c
Hunk #1 FAILED at 125.
Hunk #2 succeeded at 147 with fuzz 2.
Hunk #3 FAILED at 164.
Hunk #4 FAILED at 240.
Hunk #5 FAILED at 257.
Hunk #6 FAILED at 284.
Hunk #7 FAILED at 307.
Hunk #8 FAILED at 324.
7 out of 8 hunks FAILED -- saving rejects to file ib/dapl_lmr.c.rej
Due to the errors I had some trouble looking over your changes. Can
you resend?
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.
A few comments/questions below:
On Wed, 27 Jul 2005, Guy German wrote:
> [kdapl]: this patch adds the option of using DAT_MEM_TYPE_IA in
> dat_lmr_kcreate
>
> Signed-off-by: Guy German <guyg at voltaire.com>
>
> Index: infiniband/ulp/kdapl/ib/dapl_openib_util.c
> ===================================================================
> --- infiniband/ulp/kdapl/ib/dapl_openib_util.c (revision 2914)
> +++ infiniband/ulp/kdapl/ib/dapl_openib_util.c (working copy)
> @@ -234,8 +234,43 @@ int dapl_ib_mr_register(struct dapl_ia *
> return -ENOSYS;
> }
>
> +int dapl_ib_mr_register_ia(struct dapl_ia *ia, struct dapl_lmr *lmr,
> + union dat_region_description phys_addr, u64
> length,
> + enum dat_mem_priv_flags privileges)
> +{
> + int status;
> + int acl;
> + struct ib_mr *mr;
> + struct ib_phys_buf buf_list;
> + u64 iova = 0;
> + buf_list.addr = phys_addr.for_pa;
> + buf_list.size = length;
> + + iova = buf_list.addr;
> + acl = dapl_ib_convert_mem_privileges(privileges);
> + acl |= IB_ACCESS_MW_BIND;
> + mr = ib_reg_phys_mr(((struct dapl_pz *)lmr->param.pz)->pd,
> + &buf_list, 1, acl, &iova); + if
> (IS_ERR(mr)) {
> + status = PTR_ERR(mr);
> + dapl_dbg_log(DAPL_DBG_TYPE_ERR,
> + "%s: ib_reg_phys_mr error code return = %d\n",
> + __FUNCTION__, status);
> + return status;
> + }
> + dapl_dbg_log(DAPL_DBG_TYPE_UTIL, "%s: got handle mr=%p\n",
> + __FUNCTION__, mr);
> +
> + lmr->param.lmr_context = mr->lkey;
> + lmr->param.rmr_context = mr->rkey;
> + lmr->param.registered_size = length;
> + lmr->param.registered_address = phys_addr.for_pa;
> + lmr->mr = mr;
> + return 0;
> +}
> +
> int dapl_ib_mr_register_physical(struct dapl_ia *ia, struct dapl_lmr *lmr,
> - void *phys_addr, u64 length,
> + union dat_region_description phys_addr, u64
> length,
> enum dat_mem_priv_flags privileges)
> {
> int status;
> @@ -246,7 +281,7 @@ int dapl_ib_mr_register_physical(struct
> u64 iova = 0;
> u64 *array;
>
> - array = (u64 *) phys_addr;
> + array = (u64 *)phys_addr.for_array; /* need to add for_u64_array to
> union */
What does this comment mean?
> buf_list = kmalloc(length * sizeof *buf_list, GFP_ATOMIC);
> if (!buf_list)
> return -ENOMEM;
> @@ -265,8 +300,8 @@ int dapl_ib_mr_register_physical(struct
> if (IS_ERR(mr)) {
> status = PTR_ERR(mr);
> dapl_dbg_log(DAPL_DBG_TYPE_ERR,
> - " ib_reg_phys_mr error code return = %d\n",
> - status);
> + "%s: ib_reg_phys_mr error code return = %d\n",
> + __FUNCTION__, status);
> return status;
> }
> #if 0
> @@ -274,8 +309,8 @@ int dapl_ib_mr_register_physical(struct
> status = ib_query_mr(mr, &attr);
> if (status < 0) {
> dapl_dbg_log(DAPL_DBG_TYPE_ERR,
> - " 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.
> ib_dereg_mr(mr);
> return status;
> }
> @@ -283,10 +318,12 @@ int dapl_ib_mr_register_physical(struct
>
> lmr->param.lmr_context = mr->lkey;
> lmr->param.rmr_context = mr->rkey;
> + lmr->param.registered_size = length * PAGE_SIZE;
> + lmr->param.registered_address = array[0];
> lmr->mr = mr;
>
> dapl_dbg_log(DAPL_DBG_TYPE_UTIL,
> - "dapl_ib_mr_register_physical(%p %d) got lkey 0x%x \n",
> + "%s: (%p %d) got lkey 0x%x \n", __FUNCTION__,
> buf_list, length, lmr->param.lmr_context);
> return 0;
> }
> Index: infiniband/ulp/kdapl/ib/dapl_openib_util.h
> ===================================================================
> --- infiniband/ulp/kdapl/ib/dapl_openib_util.h (revision 2914)
> +++ infiniband/ulp/kdapl/ib/dapl_openib_util.h (working copy)
> @@ -92,8 +92,13 @@ int dapl_ib_mr_register(struct dapl_ia *
> void *virt_addr, u64 length,
> enum dat_mem_priv_flags privileges);
>
> +int dapl_ib_mr_register_ia(struct dapl_ia *ia, struct dapl_lmr *lmr,
> + union dat_region_description phys_addr, u64
> length,
> + enum dat_mem_priv_flags privileges);
> +
> int dapl_ib_mr_register_physical(struct dapl_ia *ia, struct dapl_lmr *lmr,
> - void *phys_addr, u64 length,
> + union dat_region_description phys_addr, +
> u64 length,
> enum dat_mem_priv_flags privileges);
>
> int dapl_ib_mr_deregister(struct dapl_lmr *lmr);
> Index: infiniband/ulp/kdapl/ib/dapl_lmr.c
> ===================================================================
> --- infiniband/ulp/kdapl/ib/dapl_lmr.c (revision 2914)
> +++ infiniband/ulp/kdapl/ib/dapl_lmr.c (working copy)
> @@ -125,20 +125,21 @@ error1:
> }
>
> static inline int dapl_lmr_create_physical(struct dapl_ia *ia,
> - DAT_REGION_DESCRIPTION phys_addr,
> - u64 page_count, struct dapl_pz
> *pz,
> + union dat_region_description
> phys_addr,
> + u64 page_count, +
> enum dat_mem_type mem_type,
> + struct dapl_pz *pz,
> enum dat_mem_priv_flags
> privileges,
> struct dat_lmr **lmr,
> - DAT_LMR_CONTEXT *lmr_context,
> - DAT_RMR_CONTEXT *rmr_context,
> + 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.
> u64 *registered_length,
> u64 *registered_address)
> {
> struct dapl_lmr *new_lmr;
> - u64 *array = phys_addr.for_array;
> int status;
>
> - new_lmr = dapl_lmr_alloc(ia, DAT_MEM_TYPE_PHYSICAL, phys_addr,
> + new_lmr = dapl_lmr_alloc(ia, mem_type, phys_addr,
> page_count, (struct dat_pz *) pz,
> privileges);
>
> if (NULL == new_lmr) {
> @@ -146,8 +147,14 @@ static inline int dapl_lmr_create_physic
> goto error1;
> }
>
> - status = dapl_ib_mr_register_physical(ia, new_lmr,
> phys_addr.for_array,
> - page_count, privileges);
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);
> + }
>
> if (0 != status)
> goto error2;
> @@ -157,13 +164,13 @@ static inline int dapl_lmr_create_physic
> if (lmr)
> *lmr = (struct dat_lmr *)new_lmr;
> if (lmr_context)
> - *lmr_context = (DAT_LMR_CONTEXT)new_lmr->param.lmr_context;
> + *lmr_context = (u32)new_lmr->param.lmr_context;
> if (rmr_context)
> - *rmr_context = (DAT_LMR_CONTEXT)new_lmr->param.rmr_context;
> + *rmr_context = (u32)new_lmr->param.rmr_context;
> if (registered_address)
> - *registered_address = array[0];
> + *registered_address = new_lmr->param.registered_address;
> if (registered_length)
> - *registered_length = page_count * PAGE_SIZE;
> + *registered_length = new_lmr->param.registered_size;
>
> return 0;
>
> @@ -233,7 +240,7 @@ int dapl_lmr_kcreate(struct dat_ia *ia,
> struct dapl_ia *dapl_ia;
> struct dapl_pz *dapl_pz;
> int status;
> -
> +
> dapl_dbg_log(DAPL_DBG_TYPE_API,
> "dapl_lmr_kcreate(ia:%p, mem_type:%x, ...)\n",
> ia, mem_type);
> @@ -250,10 +257,12 @@ int dapl_lmr_kcreate(struct dat_ia *ia,
> rmr_context,
> registered_length,
> registered_address);
> break;
> + case DAT_MEM_TYPE_PLATFORM: // used as proprietary Tavor-FMR
> case DAT_MEM_TYPE_PHYSICAL:
> + case DAT_MEM_TYPE_IA:
> status = dapl_lmr_create_physical(dapl_ia,
> region_description,
> - length, dapl_pz,
> privileges,
> - lmr, lmr_context,
> + length, mem_type, dapl_pz,
> + privileges, lmr,
> lmr_context,
> rmr_context,
> registered_length,
> registered_address);
> @@ -275,8 +284,6 @@ int dapl_lmr_kcreate(struct dat_ia *ia,
> registered_address);
> break;
> }
> - case DAT_MEM_TYPE_PLATFORM:
> - case DAT_MEM_TYPE_IA:
> case DAT_MEM_TYPE_BYPASS:
> status = -ENOSYS;
> break;
> @@ -300,7 +307,8 @@ int dapl_lmr_free(struct dat_lmr *lmr)
>
> switch (dapl_lmr->param.mem_type) {
> case DAT_MEM_TYPE_PHYSICAL:
> - case DAT_MEM_TYPE_VIRTUAL:
> + case DAT_MEM_TYPE_PLATFORM:
> + case DAT_MEM_TYPE_IA:
> case DAT_MEM_TYPE_LMR:
> {
> struct dapl_pz *pz;
> @@ -316,8 +324,7 @@ int dapl_lmr_free(struct dat_lmr *lmr)
> }
> break;
> }
> - case DAT_MEM_TYPE_PLATFORM:
> - case DAT_MEM_TYPE_IA:
> + case DAT_MEM_TYPE_VIRTUAL:
> case DAT_MEM_TYPE_BYPASS:
> status = -ENOSYS;
> break;
>
More information about the general
mailing list