[PATCH] qed: Enable RDMA relaxed ordering

[PATCH] qed: Enable RDMA relaxed ordering

[Shai Malin]

Enable the RoCE and iWARP FW relaxed ordering.

Signed-off-by: Ariel Elior <aelior@marvell.com>
Signed-off-by: Shai Malin <smalin@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
index 4f4b79250a2b..496092655f26 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
@@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn *p_hwfn,
 				    cnq_id);
 	}
 
+	p_params_header->relaxed_ordering = 1;
+
 	return qed_spq_post(p_hwfn, p_ent, NULL);
 }
 
-- 
2.22.0

Re: [PATCH] qed: Enable RDMA relaxed ordering

[Leon Romanovsky]

+RDMA

Jakub, David

Can we please ask that everything directly or indirectly related to RDMA
will be sent to linux-rdma@ too?

On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> Enable the RoCE and iWARP FW relaxed ordering.
> 
> Signed-off-by: Ariel Elior <aelior@marvell.com>
> Signed-off-by: Shai Malin <smalin@marvell.com>
> ---
>  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> index 4f4b79250a2b..496092655f26 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn *p_hwfn,
>  				    cnq_id);
>  	}
>  
> +	p_params_header->relaxed_ordering = 1;

Maybe it is only description that needs to be updated, but I would
expect to see call to pcie_relaxed_ordering_enabled() before setting
relaxed_ordering to always true.

If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag should
be taken into account too.

Thanks

> +
>  	return qed_spq_post(p_hwfn, p_ent, NULL);
>  }
>  
> -- 
> 2.22.0
> 

Re: [PATCH] qed: Enable RDMA relaxed ordering

[Jason Gunthorpe]

On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> +RDMA
> 
> Jakub, David
> 
> Can we please ask that everything directly or indirectly related to RDMA
> will be sent to linux-rdma@ too?
> 
> On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > Enable the RoCE and iWARP FW relaxed ordering.
> > 
> > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > Signed-off-by: Shai Malin <smalin@marvell.com>
> >  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > index 4f4b79250a2b..496092655f26 100644
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn *p_hwfn,
> >  				    cnq_id);
> >  	}
> >  
> > +	p_params_header->relaxed_ordering = 1;
> 
> Maybe it is only description that needs to be updated, but I would
> expect to see call to pcie_relaxed_ordering_enabled() before setting
> relaxed_ordering to always true.
> 
> If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag should
> be taken into account too.

Why does this file even exist in netdev? This whole struct
qed_rdma_ops mess looks like another mis-design to support out of tree
modules??

Jason

RE: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Ariel Elior]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Monday, August 23, 2021 4:34 PM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Shai Malin <smalin@marvell.com>; davem@davemloft.net;
> kuba@kernel.org; netdev@vger.kernel.org; Ariel Elior
> <aelior@marvell.com>; malin1024@gmail.com; RDMA mailing list <linux-
> rdma@vger.kernel.org>
> Subject: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > +RDMA
> >
> > Jakub, David
> >
> > Can we please ask that everything directly or indirectly related to
> > RDMA will be sent to linux-rdma@ too?
> >
> > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > Enable the RoCE and iWARP FW relaxed ordering.
> > >
> > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > index 4f4b79250a2b..496092655f26 100644
> > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> *p_hwfn,
> > >  				    cnq_id);
> > >  	}
> > >
> > > +	p_params_header->relaxed_ordering = 1;
> >
> > Maybe it is only description that needs to be updated, but I would
> > expect to see call to pcie_relaxed_ordering_enabled() before setting
> > relaxed_ordering to always true.
> >
> > If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag
> > should be taken into account too.
> 
> Why does this file even exist in netdev? This whole struct qed_rdma_ops
> mess looks like another mis-design to support out of tree modules??
> 
> Jason

Hi Jason,
qed_rdma_ops is not related to in tree / out of tree drivers. The qed is the
core module which is used by the protocol drivers which drive this type of nic:
qede, qedr, qedi and qedf for ethernet, rdma, iscsi and fcoe respectively.
qed mostly serves as a HW abstraction layer, hiding away the details of FW
interaction and device register usage, and may also hold Linux specific things
which are protocol agnostic, such as dcbx, sriov, debug data collection logic,
etc. qed interacts with the protocol drivers through ops structs for many
purposes (dcbx, ptp, sriov, etc). And also for rdma. It's just a way for us to
separate the modules in a clean way.
Thanks,
Ariel

Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Jason Gunthorpe]

On Mon, Aug 23, 2021 at 02:54:13PM +0000, Ariel Elior wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Monday, August 23, 2021 4:34 PM
> > To: Leon Romanovsky <leon@kernel.org>
> > Cc: Shai Malin <smalin@marvell.com>; davem@davemloft.net;
> > kuba@kernel.org; netdev@vger.kernel.org; Ariel Elior
> > <aelior@marvell.com>; malin1024@gmail.com; RDMA mailing list <linux-
> > rdma@vger.kernel.org>
> > Subject: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> > 
> > External Email
> > 
> > On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > > +RDMA
> > >
> > > Jakub, David
> > >
> > > Can we please ask that everything directly or indirectly related to
> > > RDMA will be sent to linux-rdma@ too?
> > >
> > > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > > Enable the RoCE and iWARP FW relaxed ordering.
> > > >
> > > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > index 4f4b79250a2b..496092655f26 100644
> > > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> > *p_hwfn,
> > > >  				    cnq_id);
> > > >  	}
> > > >
> > > > +	p_params_header->relaxed_ordering = 1;
> > >
> > > Maybe it is only description that needs to be updated, but I would
> > > expect to see call to pcie_relaxed_ordering_enabled() before setting
> > > relaxed_ordering to always true.
> > >
> > > If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag
> > > should be taken into account too.
> > 
> > Why does this file even exist in netdev? This whole struct qed_rdma_ops
> > mess looks like another mis-design to support out of tree modules??
> > 
> > Jason
> 
> Hi Jason,
> qed_rdma_ops is not related to in tree / out of tree drivers. The qed is the
> core module which is used by the protocol drivers which drive this type of nic:
> qede, qedr, qedi and qedf for ethernet, rdma, iscsi and fcoe respectively.
> qed mostly serves as a HW abstraction layer, hiding away the details of FW
> interaction and device register usage, and may also hold Linux specific things
> which are protocol agnostic, such as dcbx, sriov, debug data collection logic,
> etc. qed interacts with the protocol drivers through ops structs for many
> purposes (dcbx, ptp, sriov, etc). And also for rdma. It's just a way for us to
> separate the modules in a clean way.

Delete the ops struct.

Move the RDMA functions to the RDMA module

Directly export the core functions needed to make that work

Two halfs of the same dirver do not and should not have an ops structure
ABI between them.

Jason

Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Leon Romanovsky]

On Mon, Aug 23, 2021 at 12:17:42PM -0300, Jason Gunthorpe wrote:
> On Mon, Aug 23, 2021 at 02:54:13PM +0000, Ariel Elior wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > Sent: Monday, August 23, 2021 4:34 PM
> > > To: Leon Romanovsky <leon@kernel.org>
> > > Cc: Shai Malin <smalin@marvell.com>; davem@davemloft.net;
> > > kuba@kernel.org; netdev@vger.kernel.org; Ariel Elior
> > > <aelior@marvell.com>; malin1024@gmail.com; RDMA mailing list <linux-
> > > rdma@vger.kernel.org>
> > > Subject: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> > > 
> > > External Email
> > > 
> > > On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > > > +RDMA
> > > >
> > > > Jakub, David
> > > >
> > > > Can we please ask that everything directly or indirectly related to
> > > > RDMA will be sent to linux-rdma@ too?
> > > >
> > > > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > > > Enable the RoCE and iWARP FW relaxed ordering.
> > > > >
> > > > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > index 4f4b79250a2b..496092655f26 100644
> > > > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> > > *p_hwfn,
> > > > >  				    cnq_id);
> > > > >  	}
> > > > >
> > > > > +	p_params_header->relaxed_ordering = 1;
> > > >
> > > > Maybe it is only description that needs to be updated, but I would
> > > > expect to see call to pcie_relaxed_ordering_enabled() before setting
> > > > relaxed_ordering to always true.
> > > >
> > > > If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag
> > > > should be taken into account too.
> > > 
> > > Why does this file even exist in netdev? This whole struct qed_rdma_ops
> > > mess looks like another mis-design to support out of tree modules??
> > > 
> > > Jason
> > 
> > Hi Jason,
> > qed_rdma_ops is not related to in tree / out of tree drivers. The qed is the
> > core module which is used by the protocol drivers which drive this type of nic:
> > qede, qedr, qedi and qedf for ethernet, rdma, iscsi and fcoe respectively.
> > qed mostly serves as a HW abstraction layer, hiding away the details of FW
> > interaction and device register usage, and may also hold Linux specific things
> > which are protocol agnostic, such as dcbx, sriov, debug data collection logic,
> > etc. qed interacts with the protocol drivers through ops structs for many
> > purposes (dcbx, ptp, sriov, etc). And also for rdma. It's just a way for us to
> > separate the modules in a clean way.
> 
> Delete the ops struct.
> 
> Move the RDMA functions to the RDMA module
> 
> Directly export the core functions needed to make that work
> 
> Two halfs of the same dirver do not and should not have an ops structure
> ABI between them.

Yea, I read drivers/net/ethernet/qlogic/qed/qed_rdma.c and have hard
time to believe that hiding RDMA objects and code from the RDMA community
can be counted as "a clean way".

Thanks

> 
> Jason

RE: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Ariel Elior]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, August 24, 2021 3:25 PM
> To: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Ariel Elior <aelior@marvell.com>; Shai Malin <smalin@marvell.com>;
> davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org;
> malin1024@gmail.com; RDMA mailing list <linux-rdma@vger.kernel.org>
> Subject: Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> 
> On Mon, Aug 23, 2021 at 12:17:42PM -0300, Jason Gunthorpe wrote:
> > On Mon, Aug 23, 2021 at 02:54:13PM +0000, Ariel Elior wrote:
> > > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Sent: Monday, August 23, 2021 4:34 PM
> > > > To: Leon Romanovsky <leon@kernel.org>
> > > > Cc: Shai Malin <smalin@marvell.com>; davem@davemloft.net;
> > > > kuba@kernel.org; netdev@vger.kernel.org; Ariel Elior
> > > > <aelior@marvell.com>; malin1024@gmail.com; RDMA mailing list
> > > > <linux- rdma@vger.kernel.org>
> > > > Subject: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> > > >
> > > > External Email
> > > >
> > > > On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > > > > +RDMA
> > > > >
> > > > > Jakub, David
> > > > >
> > > > > Can we please ask that everything directly or indirectly related
> > > > > to RDMA will be sent to linux-rdma@ too?
> > > > >
> > > > > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > > > > Enable the RoCE and iWARP FW relaxed ordering.
> > > > > >
> > > > > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > > > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > > > > drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > > index 4f4b79250a2b..496092655f26 100644
> > > > > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct
> > > > > > qed_hwfn
> > > > *p_hwfn,
> > > > > >  				    cnq_id);
> > > > > >  	}
> > > > > >
> > > > > > +	p_params_header->relaxed_ordering = 1;
> > > > >
> > > > > Maybe it is only description that needs to be updated, but I
> > > > > would expect to see call to pcie_relaxed_ordering_enabled()
> > > > > before setting relaxed_ordering to always true.
> > > > >
> > > > > If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING
> > > > > flag should be taken into account too.
> > > >
> > > > Why does this file even exist in netdev? This whole struct
> > > > qed_rdma_ops mess looks like another mis-design to support out of
> tree modules??
> > > >
> > > > Jason
> > >
> > > Hi Jason,
> > > qed_rdma_ops is not related to in tree / out of tree drivers. The
> > > qed is the core module which is used by the protocol drivers which drive
> this type of nic:
> > > qede, qedr, qedi and qedf for ethernet, rdma, iscsi and fcoe respectively.
> > > qed mostly serves as a HW abstraction layer, hiding away the details
> > > of FW interaction and device register usage, and may also hold Linux
> > > specific things which are protocol agnostic, such as dcbx, sriov,
> > > debug data collection logic, etc. qed interacts with the protocol
> > > drivers through ops structs for many purposes (dcbx, ptp, sriov,
> > > etc). And also for rdma. It's just a way for us to separate the modules in a
> clean way.
> >
> > Delete the ops struct.
> >
> > Move the RDMA functions to the RDMA module
> >
> > Directly export the core functions needed to make that work
> >
> > Two halfs of the same dirver do not and should not have an ops
> > structure ABI between them.
> 
> Yea, I read drivers/net/ethernet/qlogic/qed/qed_rdma.c and have hard time
> to believe that hiding RDMA objects and code from the RDMA community
> can be counted as "a clean way".
Hi Jason, Leon

I certainly see your point, and understand the motivation to have rdma content
in the rdma tree. We will start work on refactoring the (day 1) driver design to
have more rdma logic in the rdma driver and invoke the core module when needed.
Changing ops to exported functions will also be part of that.

But realistically I don't think we can move it all. Please understand that the
core module has many responsibilities which must take RDMA components into
account, but are not just rdma specific (the same logic is applied for the other
protocol drivers).

A few examples for this might be laying out host memory for connection contexts,
computing bar offsets, computing resource amounts and allocating resources for
VFs/PFs, etc.

I think we can definitely move some of the RDMA logic from core module to the
rdma driver (as in the case of this patchset) and I understand it makes more
sense that way. But I can think of multiple code areas where this would be very
difficult.

The current design is for the core module to own data structures and logic for
device configuration and manipulation. These flows/data-structures are
triggered/populated from multiple entry points: some at the low level part of
protocol flows (e.g. create QP) which can easily transition to be an exported
function as you directed, but other entry points may also be activated earlier
e.g. when device is initialized, even before rdma driver is loaded (based on
device configuration information, for example) in which case we would not be
able to invoke functionality residing in the rdma driver, but still have to
populate data structures, invoke FW flows, configure registers, etc. which have
to do with RDMA. 

Additionally, the qedr RDMA driver services both roce and iwarp, which means
there are TCP related flows/data structures which are shared between it and our
iSCSI driver qedi. This is nicely handled in the common module avoiding any code
duplication. Ripping it out and locating it in the protocol driver would be
difficult to perform and hard to maintain across the two trees.

Likewise functionality like light l2 queues, dcbx, sriov are shared amongst all
the protocol drivers. Exposing the functionality through export instead of ops
is no problem, but moving the logic outside of the core module to a specific
protocol is both a considerable design change and may lead to code duplication
or some very convoluted flows.  

In our view the qed/qede/qedr/qedi/qedf are separate drivers, hence we used
function pointer structures for the communication between them. We use
hierarchies of structures of function pointers to group toghether those which
have common purposes (dcbx, ll2, Ethernet, rdma). Changing that to flat exported
functions for the RDMA protocol is no problem if it is preferred by you.

In summary - we got the message and will work on it, but this is no small task
and may take some time, and will likely not result in total removal of any
mention whatsoever of rdma from the core module (but will reduce it
considerably).
> 
> Thanks
> 
> >
> > Jason

Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Jason Gunthorpe]

On Tue, Aug 24, 2021 at 07:16:41PM +0000, Ariel Elior wrote:

> In our view the qed/qede/qedr/qedi/qedf are separate drivers, hence we used
> function pointer structures for the communication between them. We use
> hierarchies of structures of function pointers to group toghether those which
> have common purposes (dcbx, ll2, Ethernet, rdma). Changing that to flat exported
> functions for the RDMA protocol is no problem if it is preferred by you.

I wouldn't twist the driver into knots, but you definately should not
be using function pointers when there is only one implementation,
eliminating that would be a fine start and looks straightforward.

Many of the functions in the rdma ops do not look complicated to move,
yes, it moves around the layering a bit, but that is OK and probably
more maintainable in the end. eg modify_qp seems fairly disconnected
at the first couple layers of function calls.

> In summary - we got the message and will work on it, but this is no small task
> and may take some time, and will likely not result in total removal of any
> mention whatsoever of rdma from the core module (but will reduce it
> considerably).

I wouldn't go for complete removal, you just need to have a core
driver with an exported API that makes some sense for the device.

eg looking at a random op

qed_iwarp_set_engine_affin()

Is an "rdma" function but all it does is call
qed_llh_set_ppfid_affinity()

So export qed_llh and move the qed_iwarp to the rdma driver

etc

Jason

RE: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering

[Ariel Elior]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, August 24, 2021 10:42 PM
> To: Ariel Elior <aelior@marvell.com>
> Cc: Leon Romanovsky <leon@kernel.org>; Shai Malin
> <smalin@marvell.com>; davem@davemloft.net; kuba@kernel.org;
> netdev@vger.kernel.org; malin1024@gmail.com; RDMA mailing list <linux-
> rdma@vger.kernel.org>
> Subject: Re: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering
> 
> On Tue, Aug 24, 2021 at 07:16:41PM +0000, Ariel Elior wrote:
> 
> > In our view the qed/qede/qedr/qedi/qedf are separate drivers, hence we
> > used function pointer structures for the communication between them.
> > We use hierarchies of structures of function pointers to group
> > toghether those which have common purposes (dcbx, ll2, Ethernet,
> > rdma). Changing that to flat exported functions for the RDMA protocol is no
> problem if it is preferred by you.
> 
> I wouldn't twist the driver into knots, but you definately should not be using
> function pointers when there is only one implementation, eliminating that
> would be a fine start and looks straightforward.
> 
> Many of the functions in the rdma ops do not look complicated to move, yes,
> it moves around the layering a bit, but that is OK and probably more
> maintainable in the end. eg modify_qp seems fairly disconnected at the first
> couple layers of function calls.
> 
> > In summary - we got the message and will work on it, but this is no
> > small task and may take some time, and will likely not result in total
> > removal of any mention whatsoever of rdma from the core module (but
> > will reduce it considerably).
> 
> I wouldn't go for complete removal, you just need to have a core driver with
> an exported API that makes some sense for the device.
> 
> eg looking at a random op
> 
> qed_iwarp_set_engine_affin()
> 
> Is an "rdma" function but all it does is call
> qed_llh_set_ppfid_affinity()
> 
> So export qed_llh and move the qed_iwarp to the rdma driver
> 
> etc

Got it, and makes sense to me. I get the point on single instance of
function pointers being redundant. We will start work on the
necessary redesign right away. Meanwhile you may see a few
more critical fixes/small features coming from us which are already
queued up internally on our end, which I hope can be accepted
before we perform the changes discussed here.
Thanks,
Ariel




> 
> Jason

Re: [PATCH] qed: Enable RDMA relaxed ordering

[Shai Malin]

On Thu, Aug 26, 2021 at 04:07:00PM +0030, Jason Gunthorpe wrote:
> On Thu, Aug 26, 2021 at 12:05:18PM +0000, Shai Malin wrote:
> > On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > > +RDMA
> > >
> > > Jakub, David
> > >
> > > Can we please ask that everything directly or indirectly related to RDMA
> > > will be sent to linux-rdma@ too?
> >
> > In addition to all that was discussed regarding qed_rdma.c
> > and qed_rdma_ops - certainly, everything directly or indirectly
> > related to RDMA will be sent to linux-rdma.
> >
> > >
> > > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > > Enable the RoCE and iWARP FW relaxed ordering.
> > > >
> > > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > >  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > index 4f4b79250a2b..496092655f26 100644
> > > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> > > *p_hwfn,
> > > >  				    cnq_id);
> > > >  	}
> > > >
> > > > +	p_params_header->relaxed_ordering = 1;
> > >
> > > Maybe it is only description that needs to be updated, but I would
> > > expect to see call to pcie_relaxed_ordering_enabled() before setting
> > > relaxed_ordering to always true.
> >
> > This change will only allow the FW to support relaxed ordering but it
> > will be enabled only if the device/root-complex/server supports
> > relaxed ordering.
> > The pcie_relaxed_ordering_enabled() is not needed in this case.
> 
> I'm confused, our RDMA model is not to blanket enable relaxed
> ordering, we set out rules that the driver has to follow when a
> relaxed ordering TLP can be issued:
>  - QP/CQ/etc internal queues - device decision based on correctness
>  - Kernel created MRs - always
>  - User created MRs - only if IB_ACCESS_RELAXED_ORDERING is set
> 
> So what does this flag do, and does it follow this model?

The flag was not following the model. It was supposed to enable the 
RDMA FW to use relaxed ordering for all the above cases as long as the 
device will allow it. 
Following Leon's comment, I understand that it is not allowed with 
user created MRs.

Our gap is that in order to control the per MR relaxed ordering we will 
need to update the FW (in addition to the driver changes).

> 
> Jason

Re: [PATCH] qed: Enable RDMA relaxed ordering

[Jason Gunthorpe]

On Thu, Aug 26, 2021 at 12:05:18PM +0000, Shai Malin wrote:
> On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> > +RDMA
> > 
> > Jakub, David
> > 
> > Can we please ask that everything directly or indirectly related to RDMA
> > will be sent to linux-rdma@ too?
> 
> In addition to all that was discussed regarding qed_rdma.c 
> and qed_rdma_ops - certainly, everything directly or indirectly 
> related to RDMA will be sent to linux-rdma.
> 
> > 
> > On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > > Enable the RoCE and iWARP FW relaxed ordering.
> > >
> > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > >  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > index 4f4b79250a2b..496092655f26 100644
> > > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> > *p_hwfn,
> > >  				    cnq_id);
> > >  	}
> > >
> > > +	p_params_header->relaxed_ordering = 1;
> > 
> > Maybe it is only description that needs to be updated, but I would
> > expect to see call to pcie_relaxed_ordering_enabled() before setting
> > relaxed_ordering to always true.
> 
> This change will only allow the FW to support relaxed ordering but it 
> will be enabled only if the device/root-complex/server supports 
> relaxed ordering.
> The pcie_relaxed_ordering_enabled() is not needed in this case.

I'm confused, our RDMA model is not to blanket enable relaxed
ordering, we set out rules that the driver has to follow when a
relaxed ordering TLP can be issued:
 - QP/CQ/etc internal queues - device decision based on correctness
 - Kernel created MRs - always
 - User created MRs - only if IB_ACCESS_RELAXED_ORDERING is set

So what does this flag do, and does it follow this model?

Jason

Re: [PATCH] qed: Enable RDMA relaxed ordering

[Shai Malin]

On Mon, Aug 23, 2021 at 02:52:21PM +0300, Leon Romanovsky wrote:
> +RDMA
> 
> Jakub, David
> 
> Can we please ask that everything directly or indirectly related to RDMA
> will be sent to linux-rdma@ too?

In addition to all that was discussed regarding qed_rdma.c 
and qed_rdma_ops - certainly, everything directly or indirectly 
related to RDMA will be sent to linux-rdma.

> 
> On Sun, Aug 22, 2021 at 09:54:48PM +0300, Shai Malin wrote:
> > Enable the RoCE and iWARP FW relaxed ordering.
> >
> > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > Signed-off-by: Shai Malin <smalin@marvell.com>
> > ---
> >  drivers/net/ethernet/qlogic/qed/qed_rdma.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > index 4f4b79250a2b..496092655f26 100644
> > --- a/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > +++ b/drivers/net/ethernet/qlogic/qed/qed_rdma.c
> > @@ -643,6 +643,8 @@ static int qed_rdma_start_fw(struct qed_hwfn
> *p_hwfn,
> >  				    cnq_id);
> >  	}
> >
> > +	p_params_header->relaxed_ordering = 1;
> 
> Maybe it is only description that needs to be updated, but I would
> expect to see call to pcie_relaxed_ordering_enabled() before setting
> relaxed_ordering to always true.

This change will only allow the FW to support relaxed ordering but it 
will be enabled only if the device/root-complex/server supports 
relaxed ordering.
The pcie_relaxed_ordering_enabled() is not needed in this case.

> 
> If we are talking about RDMA, the IB_ACCESS_RELAXED_ORDERING flag should
> be taken into account too.

Thanks! 
We will need to add both FW and driver support to enable it.

> 
> Thanks
> 
> > +
> >  	return qed_spq_post(p_hwfn, p_ent, NULL);
> >  }
> >
> > --
> > 2.22.0
> >