Message-ID: <BY3PR18MB4641E80F8ABF42A5621B7573C4C59@BY3PR18MB4641.namprd18.prod.outlook.com>
Date:   Tue, 24 Aug 2021 19:16:41 +0000
From:   Ariel Elior <aelior@...vell.com>
To:     Leon Romanovsky <leon@...nel.org>, Jason Gunthorpe <jgg@...pe.ca>
CC:     Shai Malin <smalin@...vell.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "malin1024@...il.com" <malin1024@...il.com>,
        RDMA mailing list <linux-rdma@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH] qed: Enable RDMA relaxed ordering



> -----Original Message-----
> From: Leon Romanovsky <leon@...nel.org>
> Sent: Tuesday, August 24, 2021 3:25 PM
> To: Jason Gunthorpe <jgg@...pe.ca>
> Cc: Ariel Elior <aelior@...vell.com>; Shai Malin <smalin@...vell.com>;
> davem@...emloft.net; kuba@...nel.org; netdev@...r.kernel.org;
> malin1024@...il.com; RDMA mailing list <linux-rdma@...r.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@...pe.ca>
> > > > Sent: Monday, August 23, 2021 4:34 PM
> > > > To: Leon Romanovsky <leon@...nel.org>
> > > > Cc: Shai Malin <smalin@...vell.com>; davem@...emloft.net;
> > > > kuba@...nel.org; netdev@...r.kernel.org; Ariel Elior
> > > > <aelior@...vell.com>; malin1024@...il.com; RDMA mailing list
> > > > <linux- rdma@...r.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@...vell.com>
> > > > > > Signed-off-by: Shai Malin <smalin@...vell.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

Powered by blists - more mailing lists