| Message ID | 1549560811-8655-4-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce new API for T10-PI offload | expand |
> From: Israel Rukshin <israelr@mellanox.com> > > This is a preparation for signature verbs API re-design. In the new > design a single MR with IB_MR_TYPE_PI type will be used to perform > the needed mapping for PI (protection information) operations. One nit, lets keep mr type and allocation api consistent: IB_MR_TYPE_INTEGRITY ib_alloc_mr_integrity Other than that, Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> @@ -1982,6 +1982,9 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, > if (!pd->device->ops.alloc_mr) > return ERR_PTR(-EOPNOTSUPP); > > + if (WARN_ON_ONCE(mr_type == IB_MR_TYPE_PI)) > + return ERR_PTR(-EINVAL); > + So why is IB_MR_TYPE_PI a separate function, but IB_MR_TYPE_SG_GAPS is not? I think we either want one alloc/free helper per type, or we have to extend ib_alloc_mr with an arguments structure or something similar.
On 2/12/2019 5:59 PM, Christoph Hellwig wrote: >> @@ -1982,6 +1982,9 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, >> if (!pd->device->ops.alloc_mr) >> return ERR_PTR(-EOPNOTSUPP); >> >> + if (WARN_ON_ONCE(mr_type == IB_MR_TYPE_PI)) >> + return ERR_PTR(-EINVAL); >> + > So why is IB_MR_TYPE_PI a separate function, but IB_MR_TYPE_SG_GAPS > is not? > > I think we either want one alloc/free helper per type, or we have > to extend ib_alloc_mr with an arguments structure or something similar. my original commit modified the ib_alloc_mr to: struct ib_mr *ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type, - u32 max_num_sg) + u32 max_num_sg, u32 max_num_meta_sg) and I also prefer to have 1 allocation function but I guess we should use the style that the RDMA maintainers prefer and this is the review I got. So Jason/Leon/Doug in case you prefer to extend ib_alloc_mr as I originally did, I can do it for V3.
On Wed, Feb 13, 2019 at 02:41:49AM +0200, Max Gurtovoy wrote: > > On 2/12/2019 5:59 PM, Christoph Hellwig wrote: > > > @@ -1982,6 +1982,9 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, > > > if (!pd->device->ops.alloc_mr) > > > return ERR_PTR(-EOPNOTSUPP); > > > + if (WARN_ON_ONCE(mr_type == IB_MR_TYPE_PI)) > > > + return ERR_PTR(-EINVAL); > > > + > > So why is IB_MR_TYPE_PI a separate function, but IB_MR_TYPE_SG_GAPS > > is not? > > > > I think we either want one alloc/free helper per type, or we have > > to extend ib_alloc_mr with an arguments structure or something similar. > > my original commit modified the ib_alloc_mr to: > > struct ib_mr *ib_alloc_mr(struct ib_pd *pd, > enum ib_mr_type mr_type, > - u32 max_num_sg) > + u32 max_num_sg, u32 max_num_meta_sg) > > > and I also prefer to have 1 allocation function but I guess we should use > the style that the RDMA maintainers prefer and this is the review I got. > > So Jason/Leon/Doug in case you prefer to extend ib_alloc_mr as I originally > did, I can do it for V3. These monster 'do everything' multiplexor functions have turned out to be a maintenance pain in the past. I'm not sure MR will turn out any better.. So, CH's 'one alloc/free helper per type' seems like a better direction. At least the API should be understandable in that configuration. Jason
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 238ec42778ef..bc4c7c5c305b 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -1244,6 +1244,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops) SET_DEVICE_OP(dev_ops, alloc_fmr); SET_DEVICE_OP(dev_ops, alloc_hw_stats); SET_DEVICE_OP(dev_ops, alloc_mr); + SET_DEVICE_OP(dev_ops, alloc_mr_integrity); SET_DEVICE_OP(dev_ops, alloc_mw); SET_DEVICE_OP(dev_ops, alloc_pd); SET_DEVICE_OP(dev_ops, alloc_rdma_netdev); diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 4588b933d4b4..047a51bd3e79 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -1982,6 +1982,9 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, if (!pd->device->ops.alloc_mr) return ERR_PTR(-EOPNOTSUPP); + if (WARN_ON_ONCE(mr_type == IB_MR_TYPE_PI)) + return ERR_PTR(-EINVAL); + mr = pd->device->ops.alloc_mr(pd, mr_type, max_num_sg); if (!IS_ERR(mr)) { mr->device = pd->device; @@ -1999,6 +2002,49 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, } EXPORT_SYMBOL(ib_alloc_mr); +/** + * ib_alloc_mr_integrity() - Allocates an integrity memory region + * @pd: protection domain associated with the region + * @max_num_data_sg: maximum data sg entries available for registration + * @max_num_meta_sg: maximum metadata sg entries available for + * registration + * + * Notes: + * Memory registration page/sg lists must not exceed max_num_sg, + * also the integrity page/sg lists must not exceed max_num_meta_sg. + * + */ +struct ib_mr *ib_alloc_mr_integrity(struct ib_pd *pd, + u32 max_num_data_sg, + u32 max_num_meta_sg) +{ + struct ib_mr *mr; + + if (!pd->device->ops.alloc_mr_integrity) + return ERR_PTR(-EOPNOTSUPP); + + if (!max_num_meta_sg) + return ERR_PTR(-EINVAL); + + mr = pd->device->ops.alloc_mr_integrity(pd, max_num_data_sg, + max_num_meta_sg); + if (IS_ERR(mr)) + return mr; + + mr->device = pd->device; + mr->pd = pd; + mr->dm = NULL; + mr->uobject = NULL; + atomic_inc(&pd->usecnt); + mr->need_inval = false; + mr->res.type = RDMA_RESTRACK_MR; + rdma_restrack_kadd(&mr->res); + mr->type = IB_MR_TYPE_PI; + + return mr; +} +EXPORT_SYMBOL(ib_alloc_mr_integrity); + /* "Fast" memory regions */ struct ib_fmr *ib_alloc_fmr(struct ib_pd *pd, diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index 42a9e297c21f..8edba47f272e 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -756,11 +756,15 @@ __attribute_const__ int ib_rate_to_mbps(enum ib_rate rate); * register any arbitrary sg lists (without * the normal mr constraints - see * ib_map_mr_sg) + * @IB_MR_TYPE_PI: memory region that is used for + * PI (protection information) operations + */ enum ib_mr_type { IB_MR_TYPE_MEM_REG, IB_MR_TYPE_SIGNATURE, IB_MR_TYPE_SG_GAPS, + IB_MR_TYPE_PI, }; enum ib_mr_status_check { @@ -2306,6 +2310,9 @@ struct ib_device_ops { int (*dereg_mr)(struct ib_mr *mr); struct ib_mr *(*alloc_mr)(struct ib_pd *pd, enum ib_mr_type mr_type, u32 max_num_sg); + struct ib_mr *(*alloc_mr_integrity)(struct ib_pd *pd, + u32 max_num_data_sg, + u32 max_num_meta_sg); int (*advise_mr)(struct ib_pd *pd, enum ib_uverbs_advise_mr_advice advice, u32 flags, struct ib_sge *sg_list, u32 num_sge, @@ -3677,6 +3684,10 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd, enum ib_mr_type mr_type, u32 max_num_sg); +struct ib_mr *ib_alloc_mr_integrity(struct ib_pd *pd, + u32 max_num_data_sg, + u32 max_num_meta_sg); + /** * ib_update_fast_reg_key - updates the key portion of the fast_reg MR * R_Key and L_Key.