mbox series

[rdma-next,v1,0/3] Add ConnectX DCS offload support

Message ID cover.1624258894.git.leonro@nvidia.com
Headers show
Series Add ConnectX DCS offload support | expand

Message

Leon Romanovsky June 21, 2021, 7:06 a.m. UTC
From: Leon Romanovsky <leonro@nvidia.com>

Changelog:
v1:
 * Rephrase commit message of second patch
v0: https://lore.kernel.org/linux-rdma/cover.1622723815.git.leonro@nvidia.com

------------

This patchset from Lior adds support of DCI stream channel (DCS) support.

DCS is an offload to SW load balancing of DC initiator work requests.

A single DC QP initiator (DCI) can be connected to only one target at the time
and can't start new connection until the previous work request is completed.

This limitation causes to delays when the initiator process needs to
transfer data to multiple targets at the same time.

Thanks

Lior Nahmanson (3):
  net/mlx5: Add DCS caps & fields support
  RDMA/mlx5: Separate DCI QP creation logic
  RDMA/mlx5: Add DCS offload support

 drivers/infiniband/hw/mlx5/main.c |  10 ++
 drivers/infiniband/hw/mlx5/qp.c   | 168 ++++++++++++++++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h     |  14 ++-
 include/uapi/rdma/mlx5-abi.h      |  17 ++-
 4 files changed, 204 insertions(+), 5 deletions(-)

Comments

Jason Gunthorpe June 22, 2021, 6:45 p.m. UTC | #1
On Mon, Jun 21, 2021 at 10:06:15AM +0300, Leon Romanovsky wrote:
> From: Lior Nahmanson <liorna@nvidia.com>

> 

> This patch isolates DCI QP creation logic to separate function, so this

> change will reduce complexity when adding new features to DCI QP without

> interfering with other QP types.

> 

> The code was copied from create_user_qp() while taking only DCI relevant bits.

> 

> Reviewed-by: Meir Lichtinger <meirl@nvidia.com>

> Signed-off-by: Lior Nahmanson <liorna@nvidia.com>

> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

>  drivers/infiniband/hw/mlx5/qp.c | 157 ++++++++++++++++++++++++++++++++

>  1 file changed, 157 insertions(+)

> 

> diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c

> index 7a5f1eba60e3..65a380543f5a 100644

> +++ b/drivers/infiniband/hw/mlx5/qp.c

> @@ -1974,6 +1974,160 @@ static int create_xrc_tgt_qp(struct mlx5_ib_dev *dev, struct mlx5_ib_qp *qp,

>  	return 0;

>  }

>  

> +static int create_dci(struct mlx5_ib_dev *dev, struct ib_pd *pd,

> +		      struct mlx5_ib_qp *qp,

> +		      struct mlx5_create_qp_params *params)

> +{


This is a huge amount of copying just to add 4 lines, why?

There must be a better way to do this qp stuff.

Why not put more stuff in _create_user_qp()?

Jason
Leon Romanovsky June 23, 2021, 6:03 a.m. UTC | #2
On Tue, Jun 22, 2021 at 03:45:56PM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 21, 2021 at 10:06:15AM +0300, Leon Romanovsky wrote:

> > From: Lior Nahmanson <liorna@nvidia.com>

> > 

> > This patch isolates DCI QP creation logic to separate function, so this

> > change will reduce complexity when adding new features to DCI QP without

> > interfering with other QP types.

> > 

> > The code was copied from create_user_qp() while taking only DCI relevant bits.

> > 

> > Reviewed-by: Meir Lichtinger <meirl@nvidia.com>

> > Signed-off-by: Lior Nahmanson <liorna@nvidia.com>

> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

> >  drivers/infiniband/hw/mlx5/qp.c | 157 ++++++++++++++++++++++++++++++++

> >  1 file changed, 157 insertions(+)

> > 

> > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c

> > index 7a5f1eba60e3..65a380543f5a 100644

> > +++ b/drivers/infiniband/hw/mlx5/qp.c

> > @@ -1974,6 +1974,160 @@ static int create_xrc_tgt_qp(struct mlx5_ib_dev *dev, struct mlx5_ib_qp *qp,

> >  	return 0;

> >  }

> >  

> > +static int create_dci(struct mlx5_ib_dev *dev, struct ib_pd *pd,

> > +		      struct mlx5_ib_qp *qp,

> > +		      struct mlx5_create_qp_params *params)

> > +{

> 

> This is a huge amount of copying just to add 4 lines, why?

> 

> There must be a better way to do this qp stuff.

> 

> Why not put more stuff in _create_user_qp()?


Lior proposed it in original patch, but I didn't like it. It caused to
mix of various QP types and maze of "if () else ()" that are not applicable
one to another.

The huge _create_user_qp() is the reason why create_dci() is not small,
we simply had hard time to understand if specific HW bit is needed or
not in DCI flow.

My goal is to have small per-QP type specific functions that calls
to simple functions for very narrow scope.

Something like that:
static int create_dci(...)
{
  ...
  configure_send_cq(..)
  configure_recv_sq(..)
  configure_srq(...)
  ...
}

static int create_user_qp(...)
{
  ...
  configure_send_cq(..)
  configure_recv_sq(..)
  configure_srq(...)
  ...
}


Thanks

> 

> Jason
Jason Gunthorpe July 16, 2021, 3:42 p.m. UTC | #3
On Mon, Jun 21, 2021 at 10:06:13AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>

> 

> Changelog:

> v1:

>  * Rephrase commit message of second patch

> v0: https://lore.kernel.org/linux-rdma/cover.1622723815.git.leonro@nvidia.com

> 

> 

> This patchset from Lior adds support of DCI stream channel (DCS) support.

> 

> DCS is an offload to SW load balancing of DC initiator work requests.

> 

> A single DC QP initiator (DCI) can be connected to only one target at the time

> and can't start new connection until the previous work request is completed.

> 

> This limitation causes to delays when the initiator process needs to

> transfer data to multiple targets at the same time.

> 

> Thanks

> 

> Lior Nahmanson (3):

>   net/mlx5: Add DCS caps & fields support

>   RDMA/mlx5: Separate DCI QP creation logic

>   RDMA/mlx5: Add DCS offload support


Okay, can you update the shared branch?

Thanks,
Jason
Leon Romanovsky July 18, 2021, 10:38 a.m. UTC | #4
On Fri, Jul 16, 2021 at 12:42:08PM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 21, 2021 at 10:06:13AM +0300, Leon Romanovsky wrote:

> > From: Leon Romanovsky <leonro@nvidia.com>

> > 

> > Changelog:

> > v1:

> >  * Rephrase commit message of second patch

> > v0: https://lore.kernel.org/linux-rdma/cover.1622723815.git.leonro@nvidia.com

> > 

> > 

> > This patchset from Lior adds support of DCI stream channel (DCS) support.

> > 

> > DCS is an offload to SW load balancing of DC initiator work requests.

> > 

> > A single DC QP initiator (DCI) can be connected to only one target at the time

> > and can't start new connection until the previous work request is completed.

> > 

> > This limitation causes to delays when the initiator process needs to

> > transfer data to multiple targets at the same time.

> > 

> > Thanks

> > 

> > Lior Nahmanson (3):

> >   net/mlx5: Add DCS caps & fields support

> >   RDMA/mlx5: Separate DCI QP creation logic

> >   RDMA/mlx5: Add DCS offload support

> 

> Okay, can you update the shared branch?


Pushed, 96cd2dd65bb0 ("net/mlx5: Add DCS caps & fields support")

Thanks

> 

> Thanks,

> Jason
Jason Gunthorpe July 20, 2021, 6:18 p.m. UTC | #5
On Mon, Jun 21, 2021 at 10:06:13AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>

> 

> Changelog:

> v1:

>  * Rephrase commit message of second patch

> v0: https://lore.kernel.org/linux-rdma/cover.1622723815.git.leonro@nvidia.com

> 

> ------------

> 

> This patchset from Lior adds support of DCI stream channel (DCS) support.

> 

> DCS is an offload to SW load balancing of DC initiator work requests.

> 

> A single DC QP initiator (DCI) can be connected to only one target at the time

> and can't start new connection until the previous work request is completed.

> 

> This limitation causes to delays when the initiator process needs to

> transfer data to multiple targets at the same time.

> 

> Thanks

> 

> Lior Nahmanson (3):

>   net/mlx5: Add DCS caps & fields support

>   RDMA/mlx5: Separate DCI QP creation logic

>   RDMA/mlx5: Add DCS offload support


Applied to for-next, thanks

Jason
Leon Romanovsky July 21, 2021, 4:34 a.m. UTC | #6
On Tue, Jul 20, 2021 at 03:18:48PM -0300, Jason Gunthorpe wrote:
> On Mon, Jun 21, 2021 at 10:06:13AM +0300, Leon Romanovsky wrote:

> > From: Leon Romanovsky <leonro@nvidia.com>

> > 

> > Changelog:

> > v1:

> >  * Rephrase commit message of second patch

> > v0: https://lore.kernel.org/linux-rdma/cover.1622723815.git.leonro@nvidia.com

> > 

> > ------------

> > 

> > This patchset from Lior adds support of DCI stream channel (DCS) support.

> > 

> > DCS is an offload to SW load balancing of DC initiator work requests.

> > 

> > A single DC QP initiator (DCI) can be connected to only one target at the time

> > and can't start new connection until the previous work request is completed.

> > 

> > This limitation causes to delays when the initiator process needs to

> > transfer data to multiple targets at the same time.

> > 

> > Thanks

> > 

> > Lior Nahmanson (3):

> >   net/mlx5: Add DCS caps & fields support

> >   RDMA/mlx5: Separate DCI QP creation logic

> >   RDMA/mlx5: Add DCS offload support

> 

> Applied to for-next, thanks


Jason,

You forgot to push this to the RDMA tree.

Thanks

> 

> Jason