diff mbox series

[03/17] RDMA/core: Introduce IB_MR_TYPE_PI and ib_alloc_mr_integrity API

Message ID 1549302646-18446-4-git-send-email-maxg@mellanox.com (mailing list archive)
State Superseded
Headers show
Series Introduce new API for T10-PI offload | expand

Commit Message

Max Gurtovoy Feb. 4, 2019, 5:50 p.m. UTC 
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.

Signed-off-by: Israel Rukshin <israelr@mellanox.com>
Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/infiniband/core/device.c |  1 +
 drivers/infiniband/core/verbs.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 include/rdma/ib_verbs.h          | 11 ++++++++++
 3 files changed, 58 insertions(+)

Comments

Bart Van Assche Feb. 5, 2019, 4:26 a.m. UTC | #1 
On 2/4/19 9:50 AM, Max Gurtovoy wrote:
> +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)) {
> +		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;
> +}

Please use the traditional Linux kernel coding style, namely if 
(IS_ERR(mr)) return mr; instead of the above.

Thanks,

Bart.
Jason Gunthorpe Feb. 5, 2019, 4:28 a.m. UTC | #2 
On Mon, Feb 04, 2019 at 08:26:48PM -0800, Bart Van Assche wrote:
> On 2/4/19 9:50 AM, Max Gurtovoy wrote:
> > +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)) {
> > +		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;
> > +}
> 
> Please use the traditional Linux kernel coding style, namely if (IS_ERR(mr))
> return mr; instead of the above.

And don't randomly indent some ='s but not others. Just forget about
horizontal alignment :(

Jason
Leon Romanovsky Feb. 5, 2019, 9:57 a.m. UTC | #3 
On Mon, Feb 04, 2019 at 07:50:32PM +0200, Max Gurtovoy wrote:
> 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.
>
> Signed-off-by: Israel Rukshin <israelr@mellanox.com>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>  drivers/infiniband/core/device.c |  1 +
>  drivers/infiniband/core/verbs.c  | 46 ++++++++++++++++++++++++++++++++++++++++
>  include/rdma/ib_verbs.h          | 11 ++++++++++
>  3 files changed, 58 insertions(+)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 8872453e26c0..6242665f4b5c 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1237,6 +1237,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..4381bf4d98be 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)) {
> +		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 47e5c70b1352..9044356e3bb7 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,
>  };

While you are adding MR types, it will be nice to have it exposed
through rdmatool.

[leonro@server ~]$ rdma resource show mr
dev mlx5_0 mrlen 4096 pid 561 comm ibv_rc_pingpong

Thanks
Max Gurtovoy Feb. 5, 2019, 11:23 a.m. UTC | #4 
On 2/5/2019 6:28 AM, Jason Gunthorpe wrote:
> On Mon, Feb 04, 2019 at 08:26:48PM -0800, Bart Van Assche wrote:
>> On 2/4/19 9:50 AM, Max Gurtovoy wrote:
>>> +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)) {
>>> +		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;
>>> +}
>> Please use the traditional Linux kernel coding style, namely if (IS_ERR(mr))
>> return mr; instead of the above.
> And don't randomly indent some ='s but not others. Just forget about
> horizontal alignment :(
>
> Jason

Sure no problem doing that. JFYI, this coding style was taken from 
ib_alloc_mr.

I guess we can fix that too in a separate commit.
Jason Gunthorpe Feb. 5, 2019, 3:46 p.m. UTC | #5 
On Tue, Feb 05, 2019 at 01:23:03PM +0200, Max Gurtovoy wrote:
> 
> On 2/5/2019 6:28 AM, Jason Gunthorpe wrote:
> > On Mon, Feb 04, 2019 at 08:26:48PM -0800, Bart Van Assche wrote:
> > > On 2/4/19 9:50 AM, Max Gurtovoy wrote:
> > > > +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)) {
> > > > +		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;
> > > > +}
> > > Please use the traditional Linux kernel coding style, namely if (IS_ERR(mr))
> > > return mr; instead of the above.
> > And don't randomly indent some ='s but not others. Just forget about
> > horizontal alignment :(
> > 
> > Jason
> 
> Sure no problem doing that. JFYI, this coding style was taken from
> ib_alloc_mr.

Sure, but when copying the code you have to fix the indenting to match
the new location.

If you are doing the stupid line up the ='s style then you have to
manually go and fix every nearby = sign too.

IHMO just forget about horizontal white space, run your code through
clang-format and just move on.

> I guess we can fix that too in a separate commit.

There is too much of it to fix, just don't make it worse.

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 8872453e26c0..6242665f4b5c 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -1237,6 +1237,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..4381bf4d98be 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)) {
+		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 47e5c70b1352..9044356e3bb7 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,
@@ -3676,6 +3683,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.