[PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Junxian Huang]

ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as
the sg_offset/data_sg_offset/meta_sg_offset arguments. Drivers who
need to derefernce these arguments have to add NULL pointer checks
to avoid crashing the kernel.

This can be optimized by adding dummy sg_offset pointer to these
two APIs. When the sg_offset arguments are NULL, pass the pointer
of dummy to drivers. Drivers can always get a valid pointer, so no
need to add NULL pointer checks.

Junxian Huang (2):
  RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and
    ib_map_mr_sg_pi()
  RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops

 drivers/infiniband/core/verbs.c         | 12 +++++++++---
 drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
 drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

--
2.33.0

[PATCH for-next 1/2] RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Junxian Huang]

ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as the
sg_offset/data_sg_offset/meta_sg_offset arguments. In these cases,
pass a dummy pointer to drivers so they don't have to add NULL pointer
checks.

Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/infiniband/core/verbs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 473ee0831307..27060554dde2 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -2670,6 +2670,9 @@ int ib_map_mr_sg_pi(struct ib_mr *mr, struct scatterlist *data_sg,
 		    struct scatterlist *meta_sg, int meta_sg_nents,
 		    unsigned int *meta_sg_offset, unsigned int page_size)
 {
+	unsigned int data_dummy = 0;
+	unsigned int meta_dummy = 0;
+
 	if (unlikely(!mr->device->ops.map_mr_sg_pi ||
 		     WARN_ON_ONCE(mr->type != IB_MR_TYPE_INTEGRITY)))
 		return -EOPNOTSUPP;
@@ -2677,8 +2680,9 @@ int ib_map_mr_sg_pi(struct ib_mr *mr, struct scatterlist *data_sg,
 	mr->page_size = page_size;
 
 	return mr->device->ops.map_mr_sg_pi(mr, data_sg, data_sg_nents,
-					    data_sg_offset, meta_sg,
-					    meta_sg_nents, meta_sg_offset);
+					    data_sg_offset ? : &data_dummy,
+					    meta_sg, meta_sg_nents,
+					    meta_sg_offset ? : &meta_dummy);
 }
 EXPORT_SYMBOL(ib_map_mr_sg_pi);
 
@@ -2711,12 +2715,14 @@ EXPORT_SYMBOL(ib_map_mr_sg_pi);
 int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
 		 unsigned int *sg_offset, unsigned int page_size)
 {
+	unsigned int dummy = 0;
+
 	if (unlikely(!mr->device->ops.map_mr_sg))
 		return -EOPNOTSUPP;
 
 	mr->page_size = page_size;
 
-	return mr->device->ops.map_mr_sg(mr, sg, sg_nents, sg_offset);
+	return mr->device->ops.map_mr_sg(mr, sg, sg_nents, sg_offset ? : &dummy);
 }
 EXPORT_SYMBOL(ib_map_mr_sg);
 
-- 
2.33.0

[PATCH for-next 2/2] RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops

[Junxian Huang]

Since we can now ensure that the sg_offset/data_sg_offset/meta_sg_offset
arguments are valid pointer, NULL pointer checks in drivers are no longer
needed.

Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
---
 drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
 drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/mr.c b/drivers/infiniband/hw/mlx5/mr.c
index 45d9dc9c6c8f..f119078ca671 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -2545,17 +2545,13 @@ mlx5_ib_map_pa_mr_sg_pi(struct ib_mr *ibmr, struct scatterlist *data_sg,
 	if (data_sg_nents == 1) {
 		n++;
 		mr->mmkey.ndescs = 1;
-		if (data_sg_offset)
-			sg_offset = *data_sg_offset;
+		sg_offset = *data_sg_offset;
 		mr->data_length = sg_dma_len(data_sg) - sg_offset;
 		mr->data_iova = sg_dma_address(data_sg) + sg_offset;
 		if (meta_sg_nents == 1) {
 			n++;
 			mr->meta_ndescs = 1;
-			if (meta_sg_offset)
-				sg_offset = *meta_sg_offset;
-			else
-				sg_offset = 0;
+			sg_offset = *meta_sg_offset;
 			mr->meta_length = sg_dma_len(meta_sg) - sg_offset;
 			mr->pi_iova = sg_dma_address(meta_sg) + sg_offset;
 		}
@@ -2576,7 +2572,7 @@ mlx5_ib_sg_to_klms(struct mlx5_ib_mr *mr,
 {
 	struct scatterlist *sg = sgl;
 	struct mlx5_klm *klms = mr->descs;
-	unsigned int sg_offset = sg_offset_p ? *sg_offset_p : 0;
+	unsigned int sg_offset = *sg_offset_p;
 	u32 lkey = mr->ibmr.pd->local_dma_lkey;
 	int i, j = 0;
 
@@ -2594,15 +2590,14 @@ mlx5_ib_sg_to_klms(struct mlx5_ib_mr *mr,
 		sg_offset = 0;
 	}
 
-	if (sg_offset_p)
-		*sg_offset_p = sg_offset;
+	*sg_offset_p = sg_offset;
 
 	mr->mmkey.ndescs = i;
 	mr->data_length = mr->ibmr.length;
 
 	if (meta_sg_nents) {
 		sg = meta_sgl;
-		sg_offset = meta_sg_offset_p ? *meta_sg_offset_p : 0;
+		sg_offset = *meta_sg_offset_p;
 		for_each_sg(meta_sgl, sg, meta_sg_nents, j) {
 			if (unlikely(i + j >= mr->max_descs))
 				break;
@@ -2615,8 +2610,7 @@ mlx5_ib_sg_to_klms(struct mlx5_ib_mr *mr,
 
 			sg_offset = 0;
 		}
-		if (meta_sg_offset_p)
-			*meta_sg_offset_p = sg_offset;
+		*meta_sg_offset_p = sg_offset;
 
 		mr->meta_ndescs = j;
 		mr->meta_length = mr->ibmr.length - mr->data_length;
diff --git a/drivers/infiniband/sw/rdmavt/trace_mr.h b/drivers/infiniband/sw/rdmavt/trace_mr.h
index 0cb8e0a0565e..6f4c70480d34 100644
--- a/drivers/infiniband/sw/rdmavt/trace_mr.h
+++ b/drivers/infiniband/sw/rdmavt/trace_mr.h
@@ -159,7 +159,7 @@ TRACE_EVENT(
 		__entry->user_base = to_imr(ibmr)->mr.user_base;
 		__entry->ibmr_length = to_imr(ibmr)->mr.length;
 		__entry->sg_nents = sg_nents;
-		__entry->sg_offset = sg_offset ? *sg_offset : 0;
+		__entry->sg_offset = *sg_offset;
 	),
 	TP_printk(
 		"[%s] ibmr_iova %llx iova %llx user_base %llx length %llx sg_nents %d sg_offset %u",
-- 
2.33.0

Re: [PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Leon Romanovsky]

On Tue, Nov 05, 2024 at 08:08:39PM +0800, Junxian Huang wrote:
> ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as
> the sg_offset/data_sg_offset/meta_sg_offset arguments. Drivers who
> need to derefernce these arguments have to add NULL pointer checks
> to avoid crashing the kernel.
> 
> This can be optimized by adding dummy sg_offset pointer to these
> two APIs. When the sg_offset arguments are NULL, pass the pointer
> of dummy to drivers. Drivers can always get a valid pointer, so no
> need to add NULL pointer checks.
> 
> Junxian Huang (2):
>   RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and
>     ib_map_mr_sg_pi()
>   RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops
> 
>  drivers/infiniband/core/verbs.c         | 12 +++++++++---
>  drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
>  drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
>  3 files changed, 16 insertions(+), 16 deletions(-)

So what does this change give us?
We have same functionality, same number of lines, same everything ...

Thanks

> 
> --
> 2.33.0
> 
> 

Re: [PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Junxian Huang]

On 2024/11/6 20:08, Leon Romanovsky wrote:
> On Tue, Nov 05, 2024 at 08:08:39PM +0800, Junxian Huang wrote:
>> ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as
>> the sg_offset/data_sg_offset/meta_sg_offset arguments. Drivers who
>> need to derefernce these arguments have to add NULL pointer checks
>> to avoid crashing the kernel.
>>
>> This can be optimized by adding dummy sg_offset pointer to these
>> two APIs. When the sg_offset arguments are NULL, pass the pointer
>> of dummy to drivers. Drivers can always get a valid pointer, so no
>> need to add NULL pointer checks.
>>
>> Junxian Huang (2):
>>   RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and
>>     ib_map_mr_sg_pi()
>>   RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops
>>
>>  drivers/infiniband/core/verbs.c         | 12 +++++++++---
>>  drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
>>  drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
>>  3 files changed, 16 insertions(+), 16 deletions(-)
> 
> So what does this change give us?
> We have same functionality, same number of lines, same everything ...
> 

Actually this is inspired by an hns bug. When ib_map_mr_sg() passes a NULL
sg_offset pointer to hns_roce_map_mr_sg(), we dereference this pointer
without a NULL check.

Of course we can fix it by adding NULL check in hns, but I think this
patch may be a better solution since the sg_offset is guaranteed to be
a valid pointer. This could benefit future drivers who also want to
dereference sg_offset, they won't need to care about NULL checks.

Junxian

> Thanks
> 
>>
>> --
>> 2.33.0
>>
>>

Re: [PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Leon Romanovsky]

On Wed, Nov 06, 2024 at 09:12:47PM +0800, Junxian Huang wrote:
> 
> 
> On 2024/11/6 20:08, Leon Romanovsky wrote:
> > On Tue, Nov 05, 2024 at 08:08:39PM +0800, Junxian Huang wrote:
> >> ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as
> >> the sg_offset/data_sg_offset/meta_sg_offset arguments. Drivers who
> >> need to derefernce these arguments have to add NULL pointer checks
> >> to avoid crashing the kernel.
> >>
> >> This can be optimized by adding dummy sg_offset pointer to these
> >> two APIs. When the sg_offset arguments are NULL, pass the pointer
> >> of dummy to drivers. Drivers can always get a valid pointer, so no
> >> need to add NULL pointer checks.
> >>
> >> Junxian Huang (2):
> >>   RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and
> >>     ib_map_mr_sg_pi()
> >>   RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops
> >>
> >>  drivers/infiniband/core/verbs.c         | 12 +++++++++---
> >>  drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
> >>  drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
> >>  3 files changed, 16 insertions(+), 16 deletions(-)
> > 
> > So what does this change give us?
> > We have same functionality, same number of lines, same everything ...
> > 
> 
> Actually this is inspired by an hns bug. When ib_map_mr_sg() passes a NULL
> sg_offset pointer to hns_roce_map_mr_sg(), we dereference this pointer
> without a NULL check.
> 
> Of course we can fix it by adding NULL check in hns, but I think this
> patch may be a better solution since the sg_offset is guaranteed to be
> a valid pointer. This could benefit future drivers who also want to
> dereference sg_offset, they won't need to care about NULL checks.

Let's fix hns please. We are moving away from SG in RDMA.

> 
> Junxian
> 
> > Thanks
> > 
> >>
> >> --
> >> 2.33.0
> >>
> >>

Re: [PATCH for-next 0/2] Small optimization for ib_map_mr_sg() and ib_map_mr_sg_pi()

[Junxian Huang]

On 2024/11/6 21:36, Leon Romanovsky wrote:
> On Wed, Nov 06, 2024 at 09:12:47PM +0800, Junxian Huang wrote:
>>
>>
>> On 2024/11/6 20:08, Leon Romanovsky wrote:
>>> On Tue, Nov 05, 2024 at 08:08:39PM +0800, Junxian Huang wrote:
>>>> ib_map_mr_sg() and ib_map_mr_sg_pi() allow ULPs to specify NULL as
>>>> the sg_offset/data_sg_offset/meta_sg_offset arguments. Drivers who
>>>> need to derefernce these arguments have to add NULL pointer checks
>>>> to avoid crashing the kernel.
>>>>
>>>> This can be optimized by adding dummy sg_offset pointer to these
>>>> two APIs. When the sg_offset arguments are NULL, pass the pointer
>>>> of dummy to drivers. Drivers can always get a valid pointer, so no
>>>> need to add NULL pointer checks.
>>>>
>>>> Junxian Huang (2):
>>>>   RDMA/core: Add dummy sg_offset pointer for ib_map_mr_sg() and
>>>>     ib_map_mr_sg_pi()
>>>>   RDMA: Delete NULL pointer checks for sg_offset in .map_mr_sg ops
>>>>
>>>>  drivers/infiniband/core/verbs.c         | 12 +++++++++---
>>>>  drivers/infiniband/hw/mlx5/mr.c         | 18 ++++++------------
>>>>  drivers/infiniband/sw/rdmavt/trace_mr.h |  2 +-
>>>>  3 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> So what does this change give us?
>>> We have same functionality, same number of lines, same everything ...
>>>
>>
>> Actually this is inspired by an hns bug. When ib_map_mr_sg() passes a NULL
>> sg_offset pointer to hns_roce_map_mr_sg(), we dereference this pointer
>> without a NULL check.
>>
>> Of course we can fix it by adding NULL check in hns, but I think this
>> patch may be a better solution since the sg_offset is guaranteed to be
>> a valid pointer. This could benefit future drivers who also want to
>> dereference sg_offset, they won't need to care about NULL checks.
> 
> Let's fix hns please. We are moving away from SG in RDMA.
> 

Sure, thanks

Junxian

>>
>> Junxian
>>
>>> Thanks
>>>
>>>>
>>>> --
>>>> 2.33.0
>>>>
>>>>