2024-04-17 14:21:22

by Konstantin Taranov

[permalink] [raw]
Subject: [PATCH rdma-next 0/2] RDMA/mana_ib: Enable DMA-mapped memory regions

From: Konstantin Taranov <[email protected]>

This patch series enables creation of DMA-mapped memory regions.
It allows GPA creation in kernel-level PDs and implements get_dma_mr().
Note, mana_ib_get_dma_mr was already declared, but not implemented.

Konstantin Taranov (2):
RDMA/mana_ib: Allow registration of DMA-mapped memory in PDs
RDMA/mana_ib: Implement get_dma_mr

drivers/infiniband/hw/mana/device.c | 1 +
drivers/infiniband/hw/mana/main.c | 3 +++
drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
include/net/mana/gdma.h | 6 +++++
4 files changed, 46 insertions(+)


base-commit: dfcdb38b21e4fb92a49acdbdf6afa82c07c8eba0
--
2.43.0



2024-04-17 14:21:31

by Konstantin Taranov

[permalink] [raw]
Subject: [PATCH rdma-next 1/2] RDMA/mana_ib: Allow registration of DMA-mapped memory in PDs

From: Konstantin Taranov <[email protected]>

Allow the HW to register DMA-mapped memory for kernel-level PDs.

Signed-off-by: Konstantin Taranov <[email protected]>
---
drivers/infiniband/hw/mana/main.c | 3 +++
include/net/mana/gdma.h | 1 +
2 files changed, 4 insertions(+)

diff --git a/drivers/infiniband/hw/mana/main.c b/drivers/infiniband/hw/mana/main.c
index b31dcff32699..820af42d1fe1 100644
--- a/drivers/infiniband/hw/mana/main.c
+++ b/drivers/infiniband/hw/mana/main.c
@@ -82,6 +82,9 @@ int mana_ib_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
mana_gd_init_req_hdr(&req.hdr, GDMA_CREATE_PD, sizeof(req),
sizeof(resp));

+ if (!udata)
+ flags |= GDMA_PD_FLAG_ALLOW_GPA_MR;
+
req.flags = flags;
err = mana_gd_send_request(gc, sizeof(req), &req,
sizeof(resp), &resp);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 27684135bb4d..8d796a30ddde 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -762,6 +762,7 @@ struct gdma_destroy_dma_region_req {

enum gdma_pd_flags {
GDMA_PD_FLAG_INVALID = 0,
+ GDMA_PD_FLAG_ALLOW_GPA_MR = 1,
};

struct gdma_create_pd_req {
--
2.43.0


2024-04-17 14:44:44

by Konstantin Taranov

[permalink] [raw]
Subject: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

From: Konstantin Taranov <[email protected]>

Implement allocation of DMA-mapped memory regions.

Signed-off-by: Konstantin Taranov <[email protected]>
---
drivers/infiniband/hw/mana/device.c | 1 +
drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
include/net/mana/gdma.h | 5 ++++
3 files changed, 42 insertions(+)

diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
index 6fa902ee80a6..043cef09f1c2 100644
--- a/drivers/infiniband/hw/mana/device.c
+++ b/drivers/infiniband/hw/mana/device.c
@@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops = {
.destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
.destroy_wq = mana_ib_destroy_wq,
.disassociate_ucontext = mana_ib_disassociate_ucontext,
+ .get_dma_mr = mana_ib_get_dma_mr,
.get_port_immutable = mana_ib_get_port_immutable,
.mmap = mana_ib_mmap,
.modify_qp = mana_ib_modify_qp,
diff --git a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
index 4f13423ecdbd..7c9394926a18 100644
--- a/drivers/infiniband/hw/mana/mr.c
+++ b/drivers/infiniband/hw/mana/mr.c
@@ -8,6 +8,8 @@
#define VALID_MR_FLAGS \
(IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ)

+#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
+
static enum gdma_mr_access_flags
mana_ib_verbs_to_gdma_access_flags(int access_flags)
{
@@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev *dev, struct mana_ib_mr *mr,
req.mr_type = mr_params->mr_type;

switch (mr_params->mr_type) {
+ case GDMA_MR_TYPE_GPA:
+ break;
case GDMA_MR_TYPE_GVA:
req.gva.dma_region_handle = mr_params->gva.dma_region_handle;
req.gva.virtual_address = mr_params->gva.virtual_address;
@@ -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd *ibpd, u64 start, u64 length,
return ERR_PTR(err);
}

+struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int access_flags)
+{
+ struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd, ibpd);
+ struct gdma_create_mr_params mr_params = {};
+ struct ib_device *ibdev = ibpd->device;
+ struct mana_ib_dev *dev;
+ struct mana_ib_mr *mr;
+ int err;
+
+ dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
+
+ if (access_flags & ~VALID_DMA_MR_FLAGS)
+ return ERR_PTR(-EINVAL);
+
+ mr = kzalloc(sizeof(*mr), GFP_KERNEL);
+ if (!mr)
+ return ERR_PTR(-ENOMEM);
+
+ mr_params.pd_handle = pd->pd_handle;
+ mr_params.mr_type = GDMA_MR_TYPE_GPA;
+
+ err = mana_ib_gd_create_mr(dev, mr, &mr_params);
+ if (err)
+ goto err_free;
+
+ return &mr->ibmr;
+
+err_free:
+ kfree(mr);
+ return ERR_PTR(err);
+}
+
int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
{
struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr, ibmr);
diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
index 8d796a30ddde..dc19b5cb33a6 100644
--- a/include/net/mana/gdma.h
+++ b/include/net/mana/gdma.h
@@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
};/* HW DATA */

enum gdma_mr_type {
+ /*
+ * Guest Physical Address - MRs of this type allow access
+ * to any DMA-mapped memory using bus-logical address
+ */
+ GDMA_MR_TYPE_GPA = 1,
/* Guest Virtual Address - MRs of this type allow access
* to memory mapped by PTEs associated with this MR using a virtual
* address that is set up in the MST
--
2.43.0


2024-04-17 15:06:17

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> From: Konstantin Taranov <[email protected]>
>
> Implement allocation of DMA-mapped memory regions.
>
> Signed-off-by: Konstantin Taranov <[email protected]>
> ---
> drivers/infiniband/hw/mana/device.c | 1 +
> drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
> include/net/mana/gdma.h | 5 ++++
> 3 files changed, 42 insertions(+)

What is the point of doing this without supporting enough verbs to
allow a kernel ULP?

Jason

2024-04-18 10:28:49

by Zhu Yanjun

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

On 17.04.24 16:20, Konstantin Taranov wrote:
> From: Konstantin Taranov <[email protected]>
>
> Implement allocation of DMA-mapped memory regions.
>
> Signed-off-by: Konstantin Taranov <[email protected]>
> ---
> drivers/infiniband/hw/mana/device.c | 1 +
> drivers/infiniband/hw/mana/mr.c | 36 +++++++++++++++++++++++++++++
> include/net/mana/gdma.h | 5 ++++
> 3 files changed, 42 insertions(+)
>
> diff --git a/drivers/infiniband/hw/mana/device.c b/drivers/infiniband/hw/mana/device.c
> index 6fa902ee80a6..043cef09f1c2 100644
> --- a/drivers/infiniband/hw/mana/device.c
> +++ b/drivers/infiniband/hw/mana/device.c
> @@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops = {
> .destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
> .destroy_wq = mana_ib_destroy_wq,
> .disassociate_ucontext = mana_ib_disassociate_ucontext,
> + .get_dma_mr = mana_ib_get_dma_mr,
> .get_port_immutable = mana_ib_get_port_immutable,
> .mmap = mana_ib_mmap,
> .modify_qp = mana_ib_modify_qp,
> diff --git a/drivers/infiniband/hw/mana/mr.c b/drivers/infiniband/hw/mana/mr.c
> index 4f13423ecdbd..7c9394926a18 100644
> --- a/drivers/infiniband/hw/mana/mr.c
> +++ b/drivers/infiniband/hw/mana/mr.c
> @@ -8,6 +8,8 @@
> #define VALID_MR_FLAGS \
> (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE | IB_ACCESS_REMOTE_READ)
>
> +#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
> +
> static enum gdma_mr_access_flags
> mana_ib_verbs_to_gdma_access_flags(int access_flags)
> {
> @@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev *dev, struct mana_ib_mr *mr,
> req.mr_type = mr_params->mr_type;
>
> switch (mr_params->mr_type) {
> + case GDMA_MR_TYPE_GPA:
> + break;
> case GDMA_MR_TYPE_GVA:
> req.gva.dma_region_handle = mr_params->gva.dma_region_handle;
> req.gva.virtual_address = mr_params->gva.virtual_address;
> @@ -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd *ibpd, u64 start, u64 length,
> return ERR_PTR(err);
> }
>

Not sure if the following function needs comments or not.
If yes, the kernel doc
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/doc-guide/kernel-doc.rst?h=v6.9-rc4#n67
can provide a good example.

Best Regards,
Zhu Yanjun

> +struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int access_flags)
> +{
> + struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd, ibpd);
> + struct gdma_create_mr_params mr_params = {};
> + struct ib_device *ibdev = ibpd->device;
> + struct mana_ib_dev *dev;
> + struct mana_ib_mr *mr;
> + int err;
> +
> + dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> +
> + if (access_flags & ~VALID_DMA_MR_FLAGS)
> + return ERR_PTR(-EINVAL);
> +
> + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> + if (!mr)
> + return ERR_PTR(-ENOMEM);
> +
> + mr_params.pd_handle = pd->pd_handle;
> + mr_params.mr_type = GDMA_MR_TYPE_GPA;
> +
> + err = mana_ib_gd_create_mr(dev, mr, &mr_params);
> + if (err)
> + goto err_free;
> +
> + return &mr->ibmr;
> +
> +err_free:
> + kfree(mr);
> + return ERR_PTR(err);
> +}
> +
> int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> {
> struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr, ibmr);
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index 8d796a30ddde..dc19b5cb33a6 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
> };/* HW DATA */
>
> enum gdma_mr_type {
> + /*
> + * Guest Physical Address - MRs of this type allow access
> + * to any DMA-mapped memory using bus-logical address
> + */
> + GDMA_MR_TYPE_GPA = 1,
> /* Guest Virtual Address - MRs of this type allow access
> * to memory mapped by PTEs associated with this MR using a virtual
> * address that is set up in the MST


2024-04-19 09:02:58

by Konstantin Taranov

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

> From: Zhu Yanjun <[email protected]>
> On 17.04.24 16:20, Konstantin Taranov wrote:
> > From: Konstantin Taranov <[email protected]>
> >
> > Implement allocation of DMA-mapped memory regions.
> >
> > Signed-off-by: Konstantin Taranov <[email protected]>
> > ---
> > drivers/infiniband/hw/mana/device.c | 1 +
> > drivers/infiniband/hw/mana/mr.c | 36
> +++++++++++++++++++++++++++++
> > include/net/mana/gdma.h | 5 ++++
> > 3 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/infiniband/hw/mana/device.c
> > b/drivers/infiniband/hw/mana/device.c
> > index 6fa902ee80a6..043cef09f1c2 100644
> > --- a/drivers/infiniband/hw/mana/device.c
> > +++ b/drivers/infiniband/hw/mana/device.c
> > @@ -29,6 +29,7 @@ static const struct ib_device_ops mana_ib_dev_ops =
> {
> > .destroy_rwq_ind_table = mana_ib_destroy_rwq_ind_table,
> > .destroy_wq = mana_ib_destroy_wq,
> > .disassociate_ucontext = mana_ib_disassociate_ucontext,
> > + .get_dma_mr = mana_ib_get_dma_mr,
> > .get_port_immutable = mana_ib_get_port_immutable,
> > .mmap = mana_ib_mmap,
> > .modify_qp = mana_ib_modify_qp,
> > diff --git a/drivers/infiniband/hw/mana/mr.c
> > b/drivers/infiniband/hw/mana/mr.c index 4f13423ecdbd..7c9394926a18
> > 100644
> > --- a/drivers/infiniband/hw/mana/mr.c
> > +++ b/drivers/infiniband/hw/mana/mr.c
> > @@ -8,6 +8,8 @@
> > #define VALID_MR_FLAGS \
> > (IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_WRITE |
> > IB_ACCESS_REMOTE_READ)
> >
> > +#define VALID_DMA_MR_FLAGS IB_ACCESS_LOCAL_WRITE
> > +
> > static enum gdma_mr_access_flags
> > mana_ib_verbs_to_gdma_access_flags(int access_flags)
> > {
> > @@ -39,6 +41,8 @@ static int mana_ib_gd_create_mr(struct mana_ib_dev
> *dev, struct mana_ib_mr *mr,
> > req.mr_type = mr_params->mr_type;
> >
> > switch (mr_params->mr_type) {
> > + case GDMA_MR_TYPE_GPA:
> > + break;
> > case GDMA_MR_TYPE_GVA:
> > req.gva.dma_region_handle = mr_params-
> >gva.dma_region_handle;
> > req.gva.virtual_address = mr_params->gva.virtual_address;
> @@
> > -168,6 +172,38 @@ struct ib_mr *mana_ib_reg_user_mr(struct ib_pd
> *ibpd, u64 start, u64 length,
> > return ERR_PTR(err);
> > }
> >
>
> Not sure if the following function needs comments or not.
> If yes, the kernel doc
> https://git.ke/
> rnel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2
> Ftree%2FDocumentation%2Fdoc-guide%2Fkernel-doc.rst%3Fh%3Dv6.9-
> rc4%23n67&data=05%7C02%7Ckotaranov%40microsoft.com%7C2816715935
> 85405f280e08dc5f925007%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C
> 0%7C638490329257001758%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w
> LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C
> %7C&sdata=1Mzt0DKzty2jMJYm52gP%2FaloYnFGUTzN7gzAP05RdoQ%3D&res
> erved=0
> can provide a good example.
>

Thanks! I will have a look and see how I can improve comments.

> Best Regards,
> Zhu Yanjun
>
> > +struct ib_mr *mana_ib_get_dma_mr(struct ib_pd *ibpd, int
> > +access_flags) {
> > + struct mana_ib_pd *pd = container_of(ibpd, struct mana_ib_pd,
> ibpd);
> > + struct gdma_create_mr_params mr_params = {};
> > + struct ib_device *ibdev = ibpd->device;
> > + struct mana_ib_dev *dev;
> > + struct mana_ib_mr *mr;
> > + int err;
> > +
> > + dev = container_of(ibdev, struct mana_ib_dev, ib_dev);
> > +
> > + if (access_flags & ~VALID_DMA_MR_FLAGS)
> > + return ERR_PTR(-EINVAL);
> > +
> > + mr = kzalloc(sizeof(*mr), GFP_KERNEL);
> > + if (!mr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + mr_params.pd_handle = pd->pd_handle;
> > + mr_params.mr_type = GDMA_MR_TYPE_GPA;
> > +
> > + err = mana_ib_gd_create_mr(dev, mr, &mr_params);
> > + if (err)
> > + goto err_free;
> > +
> > + return &mr->ibmr;
> > +
> > +err_free:
> > + kfree(mr);
> > + return ERR_PTR(err);
> > +}
> > +
> > int mana_ib_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
> > {
> > struct mana_ib_mr *mr = container_of(ibmr, struct mana_ib_mr,
> > ibmr); diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> > index 8d796a30ddde..dc19b5cb33a6 100644
> > --- a/include/net/mana/gdma.h
> > +++ b/include/net/mana/gdma.h
> > @@ -788,6 +788,11 @@ struct gdma_destory_pd_resp {
> > };/* HW DATA */
> >
> > enum gdma_mr_type {
> > + /*
> > + * Guest Physical Address - MRs of this type allow access
> > + * to any DMA-mapped memory using bus-logical address
> > + */
> > + GDMA_MR_TYPE_GPA = 1,
> > /* Guest Virtual Address - MRs of this type allow access
> > * to memory mapped by PTEs associated with this MR using a virtual
> > * address that is set up in the MST

2024-04-19 09:14:29

by Konstantin Taranov

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

> From: Jason Gunthorpe <[email protected]>
> On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> > From: Konstantin Taranov <[email protected]>
> >
> > Implement allocation of DMA-mapped memory regions.
> >
> > Signed-off-by: Konstantin Taranov <[email protected]>
> > ---
> > drivers/infiniband/hw/mana/device.c | 1 +
> > drivers/infiniband/hw/mana/mr.c | 36
> +++++++++++++++++++++++++++++
> > include/net/mana/gdma.h | 5 ++++
> > 3 files changed, 42 insertions(+)
>
> What is the point of doing this without supporting enough verbs to allow a
> kernel ULP?
>

True, the proposed code is useless at this state.
Nevertheless, mana_ib team aims to send kernel ULP patches after we are done
with uverbs pathes (i.e., udata is not null). As this change does not conflict with the
current effort, I decided to send this patch now. I can extend the series to make
it more useful.

Jason, could you suggest a minimal list of ib_device_ops methods, that includes
get_dma_mr, which can be approved?

Thanks,
Konstantin

> Jason

2024-04-19 12:14:32

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > From: Jason Gunthorpe <[email protected]>
> > On Wed, Apr 17, 2024 at 07:20:59AM -0700, Konstantin Taranov wrote:
> > > From: Konstantin Taranov <[email protected]>
> > >
> > > Implement allocation of DMA-mapped memory regions.
> > >
> > > Signed-off-by: Konstantin Taranov <[email protected]>
> > > ---
> > > drivers/infiniband/hw/mana/device.c | 1 +
> > > drivers/infiniband/hw/mana/mr.c | 36
> > +++++++++++++++++++++++++++++
> > > include/net/mana/gdma.h | 5 ++++
> > > 3 files changed, 42 insertions(+)
> >
> > What is the point of doing this without supporting enough verbs to allow a
> > kernel ULP?
> >
>
> True, the proposed code is useless at this state.
> Nevertheless, mana_ib team aims to send kernel ULP patches after we are done
> with uverbs pathes (i.e., udata is not null). As this change does not conflict with the
> current effort, I decided to send this patch now. I can extend the series to make
> it more useful.
>
> Jason, could you suggest a minimal list of ib_device_ops methods, that includes
> get_dma_mr, which can be approved?

Is there any chance you can send a single series to support a
ULP. NVMe or something like?

Jason

2024-04-22 09:13:19

by Konstantin Taranov

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

> From: Jason Gunthorpe <[email protected]>
> On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > > From: Jason Gunthorpe <[email protected]> On Wed, Apr 17, 2024 at
> > > 07:20:59AM -0700, Konstantin Taranov wrote:
> > > > From: Konstantin Taranov <[email protected]>
> > > >
> > > > Implement allocation of DMA-mapped memory regions.
> > > >
> > > > Signed-off-by: Konstantin Taranov <[email protected]>
> > > > ---
> > > > drivers/infiniband/hw/mana/device.c | 1 +
> > > > drivers/infiniband/hw/mana/mr.c | 36
> > > +++++++++++++++++++++++++++++
> > > > include/net/mana/gdma.h | 5 ++++
> > > > 3 files changed, 42 insertions(+)
> > >
> > > What is the point of doing this without supporting enough verbs to
> > > allow a kernel ULP?
> > >
> >
> > True, the proposed code is useless at this state.
> > Nevertheless, mana_ib team aims to send kernel ULP patches after we
> > are done with uverbs pathes (i.e., udata is not null). As this change
> > does not conflict with the current effort, I decided to send this
> > patch now. I can extend the series to make it more useful.
> >
> > Jason, could you suggest a minimal list of ib_device_ops methods,
> > that includes get_dma_mr, which can be approved?
>
> Is there any chance you can send a single series to support a ULP. NVMe or
> something like?

Sure, I can. I will investigate the way to make get_dma_mr used with fewer changes.

Generally, I am wondering what would be easier for reviewers.
Should I try to send short patch series enabling one feature, or should I actually try
to produce long patch series that enable a use-case consisting of several features?

Konstantin

2024-04-22 17:35:55

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH rdma-next 2/2] RDMA/mana_ib: Implement get_dma_mr

On Mon, Apr 22, 2024 at 09:12:46AM +0000, Konstantin Taranov wrote:
> > From: Jason Gunthorpe <[email protected]>
> > On Fri, Apr 19, 2024 at 09:14:14AM +0000, Konstantin Taranov wrote:
> > > > From: Jason Gunthorpe <[email protected]> On Wed, Apr 17, 2024 at
> > > > 07:20:59AM -0700, Konstantin Taranov wrote:
> > > > > From: Konstantin Taranov <[email protected]>
> > > > >
> > > > > Implement allocation of DMA-mapped memory regions.
> > > > >
> > > > > Signed-off-by: Konstantin Taranov <[email protected]>
> > > > > ---
> > > > > drivers/infiniband/hw/mana/device.c | 1 +
> > > > > drivers/infiniband/hw/mana/mr.c | 36
> > > > +++++++++++++++++++++++++++++
> > > > > include/net/mana/gdma.h | 5 ++++
> > > > > 3 files changed, 42 insertions(+)
> > > >
> > > > What is the point of doing this without supporting enough verbs to
> > > > allow a kernel ULP?
> > > >
> > >
> > > True, the proposed code is useless at this state.
> > > Nevertheless, mana_ib team aims to send kernel ULP patches after we
> > > are done with uverbs pathes (i.e., udata is not null). As this change
> > > does not conflict with the current effort, I decided to send this
> > > patch now. I can extend the series to make it more useful.
> > >
> > > Jason, could you suggest a minimal list of ib_device_ops methods,
> > > that includes get_dma_mr, which can be approved?
> >
> > Is there any chance you can send a single series to support a ULP. NVMe or
> > something like?
>
> Sure, I can. I will investigate the way to make get_dma_mr used with fewer changes.
>
> Generally, I am wondering what would be easier for reviewers.
> Should I try to send short patch series enabling one feature, or should I actually try
> to produce long patch series that enable a use-case consisting of several features?

Generally the guideline is to avoid adding dead code, some exceptions
may be possible, but that should be the gold standard to try for,
IMHO.

If you want to support kernel ULPs then say witch kernel ULP you want
and send a series to make it work.

If that series is way too big then split it into two halfs and post
both at the start.

Jason