| Message ID | 20210822185448.12053-1-smalin@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | qed: Enable RDMA relaxed ordering | expand |
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
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
> -----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
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
> -----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
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); }