| Message ID | 1437548143-24893-3-git-send-email-sagig@mellanox.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Wed, Jul 22, 2015 at 09:55:02AM +0300, Sagi Grimberg wrote: > > +struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, > + enum ib_mr_type mr_type, > + u32 max_entries, > + u32 flags) > +{ This is just a copy of mlx4_ib_alloc_fast_reg_mr with this added: > + if (mr_type != IB_MR_TYPE_FAST_REG || flags) > + return ERR_PTR(-EINVAL); Are all the driver updates the same? It looks like it. I'd suggest shortening this patch series, have the core provide the wrapper immediately: struct ib_mr *ib_alloc_mr(struct ib_pd *pd, { ... if (pd->device->alloc_mr) { mr = pd->device->alloc_mr(pd, mr_type, max_entries, flags); } else { if (mr_type != IB_MR_TYPE_FAST_REG || flags || !ib_dev->alloc_fast_reg_mr) return ERR_PTR(-ENOSYS); mr = pd->device->alloc_fast_reg_mr(..); } } Then go through the series to remove ib_alloc_fast_reg_mr Then go through one series to migrate the drivers from alloc_fast_reg_mr to alloc_mr Then entirely drop alloc_fast_reg_mr from the driver API. That should be shorter and easier to read the driver diffs, which is the major change here. This whole section (up to 20) looks reasonable to me.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/22/2015 7:58 PM, Jason Gunthorpe wrote: > On Wed, Jul 22, 2015 at 09:55:02AM +0300, Sagi Grimberg wrote: >> >> +struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, >> + enum ib_mr_type mr_type, >> + u32 max_entries, >> + u32 flags) >> +{ > > This is just a copy of mlx4_ib_alloc_fast_reg_mr with > this added: > >> + if (mr_type != IB_MR_TYPE_FAST_REG || flags) >> + return ERR_PTR(-EINVAL); > > Are all the driver updates the same? It looks like it. > > I'd suggest shortening this patch series, have the core provide the > wrapper immediately: > > struct ib_mr *ib_alloc_mr(struct ib_pd *pd, > { > ... > > if (pd->device->alloc_mr) { > mr = pd->device->alloc_mr(pd, mr_type, max_entries, flags); > } else { > if (mr_type != IB_MR_TYPE_FAST_REG || flags || > !ib_dev->alloc_fast_reg_mr) > return ERR_PTR(-ENOSYS); > mr = pd->device->alloc_fast_reg_mr(..); > } > } > > Then go through the series to remove ib_alloc_fast_reg_mr > > Then go through one series to migrate the drivers from > alloc_fast_reg_mr to alloc_mr > > Then entirely drop alloc_fast_reg_mr from the driver API. > > That should be shorter and easier to read the driver diffs, which is > the major change here. Yea, it would be better... Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/22/2015 12:22 PM, Sagi Grimberg wrote: > On 7/22/2015 7:58 PM, Jason Gunthorpe wrote: >> On Wed, Jul 22, 2015 at 09:55:02AM +0300, Sagi Grimberg wrote: >>> >>> +struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, >>> + enum ib_mr_type mr_type, >>> + u32 max_entries, >>> + u32 flags) >>> +{ >> >> This is just a copy of mlx4_ib_alloc_fast_reg_mr with >> this added: >> >>> + if (mr_type != IB_MR_TYPE_FAST_REG || flags) >>> + return ERR_PTR(-EINVAL); >> >> Are all the driver updates the same? It looks like it. >> >> I'd suggest shortening this patch series, have the core provide the >> wrapper immediately: >> >> struct ib_mr *ib_alloc_mr(struct ib_pd *pd, >> { >> ... >> >> if (pd->device->alloc_mr) { >> mr = pd->device->alloc_mr(pd, mr_type, max_entries, flags); >> } else { >> if (mr_type != IB_MR_TYPE_FAST_REG || flags || >> !ib_dev->alloc_fast_reg_mr) >> return ERR_PTR(-ENOSYS); >> mr = pd->device->alloc_fast_reg_mr(..); >> } >> } >> >> Then go through the series to remove ib_alloc_fast_reg_mr >> >> Then go through one series to migrate the drivers from >> alloc_fast_reg_mr to alloc_mr >> >> Then entirely drop alloc_fast_reg_mr from the driver API. >> >> That should be shorter and easier to read the driver diffs, which is >> the major change here. > > Yea, it would be better... 43 patches overflows my stack ;) I agree with Jason's suggestion. Steve. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 22, 2015 at 01:50:01PM -0500, Steve Wise wrote:
> 43 patches overflows my stack ;) I agree with Jason's suggestion.
Saig, you may as well just send the ib_alloc_mr rework as a series and
get it done with, I'd pass off on the core parts of v2.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/22/2015 9:54 PM, Jason Gunthorpe wrote: > On Wed, Jul 22, 2015 at 01:50:01PM -0500, Steve Wise wrote: > >> 43 patches overflows my stack ;) I agree with Jason's suggestion. > > Saig, you may as well just send the ib_alloc_mr rework as a series and > get it done with, I'd pass off on the core parts of v2. I'll split that off from the rest. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c index a6f44ee..54671c7 100644 --- a/drivers/infiniband/hw/mlx4/main.c +++ b/drivers/infiniband/hw/mlx4/main.c @@ -2298,6 +2298,7 @@ static void *mlx4_ib_add(struct mlx4_dev *dev) ibdev->ib_dev.rereg_user_mr = mlx4_ib_rereg_user_mr; ibdev->ib_dev.dereg_mr = mlx4_ib_dereg_mr; ibdev->ib_dev.alloc_fast_reg_mr = mlx4_ib_alloc_fast_reg_mr; + ibdev->ib_dev.alloc_mr = mlx4_ib_alloc_mr; ibdev->ib_dev.alloc_fast_reg_page_list = mlx4_ib_alloc_fast_reg_page_list; ibdev->ib_dev.free_fast_reg_page_list = mlx4_ib_free_fast_reg_page_list; ibdev->ib_dev.attach_mcast = mlx4_ib_mcg_attach; diff --git a/drivers/infiniband/hw/mlx4/mlx4_ib.h b/drivers/infiniband/hw/mlx4/mlx4_ib.h index 334387f..c8b5679 100644 --- a/drivers/infiniband/hw/mlx4/mlx4_ib.h +++ b/drivers/infiniband/hw/mlx4/mlx4_ib.h @@ -680,6 +680,10 @@ struct ib_mw *mlx4_ib_alloc_mw(struct ib_pd *pd, enum ib_mw_type type); int mlx4_ib_bind_mw(struct ib_qp *qp, struct ib_mw *mw, struct ib_mw_bind *mw_bind); int mlx4_ib_dealloc_mw(struct ib_mw *mw); +struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, + enum ib_mr_type mr_type, + u32 max_entries, + u32 flags); struct ib_mr *mlx4_ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len); struct ib_fast_reg_page_list *mlx4_ib_alloc_fast_reg_page_list(struct ib_device *ibdev, diff --git a/drivers/infiniband/hw/mlx4/mr.c b/drivers/infiniband/hw/mlx4/mr.c index e0d2717..3cba374 100644 --- a/drivers/infiniband/hw/mlx4/mr.c +++ b/drivers/infiniband/hw/mlx4/mr.c @@ -350,6 +350,44 @@ int mlx4_ib_dealloc_mw(struct ib_mw *ibmw) return 0; } +struct ib_mr *mlx4_ib_alloc_mr(struct ib_pd *pd, + enum ib_mr_type mr_type, + u32 max_entries, + u32 flags) +{ + struct mlx4_ib_dev *dev = to_mdev(pd->device); + struct mlx4_ib_mr *mr; + int err; + + if (mr_type != IB_MR_TYPE_FAST_REG || flags) + return ERR_PTR(-EINVAL); + + mr = kmalloc(sizeof *mr, GFP_KERNEL); + if (!mr) + return ERR_PTR(-ENOMEM); + + err = mlx4_mr_alloc(dev->dev, to_mpd(pd)->pdn, 0, 0, 0, + max_entries, 0, &mr->mmr); + if (err) + goto err_free; + + err = mlx4_mr_enable(dev->dev, &mr->mmr); + if (err) + goto err_mr; + + mr->ibmr.rkey = mr->ibmr.lkey = mr->mmr.key; + mr->umem = NULL; + + return &mr->ibmr; + +err_mr: + (void) mlx4_mr_free(dev->dev, &mr->mmr); + +err_free: + kfree(mr); + return ERR_PTR(err); +} + struct ib_mr *mlx4_ib_alloc_fast_reg_mr(struct ib_pd *pd, int max_page_list_len) {
Signed-off-by: Sagi Grimberg <sagig@mellanox.com> --- drivers/infiniband/hw/mlx4/main.c | 1 + drivers/infiniband/hw/mlx4/mlx4_ib.h | 4 ++++ drivers/infiniband/hw/mlx4/mr.c | 38 ++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+)