Re: [PATCH] RDMA/mana_ib: Use v2 version of cfg_rx_steer_req to enable RX coalescing

[Leon Romanovsky]

On Wed, May 10, 2023 at 04:37:57PM +0000, Long Li wrote:
> >Subject: Re: [PATCH] RDMA/mana_ib: Use v2 version of cfg_rx_steer_req to
> >enable RX coalescing
> >
> >On Tue, May 09, 2023 at 07:08:36PM +0000, Long Li wrote:
> >> > Subject: Re: [PATCH] RDMA/mana_ib: Use v2 version of
> >> > cfg_rx_steer_req to enable RX coalescing
> >> >
> >> > On Mon, May 08, 2023 at 02:45:44PM +0000, Haiyang Zhang wrote:
> >> > >
> >> > >
> >> > > > -----Original Message-----
> >> > > > From: Leon Romanovsky <leon@xxxxxxxxxx>
> >> > > > Sent: Monday, May 8, 2023 2:10 AM
> >> > > > To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> >> > > > Cc: Long Li <longli@xxxxxxxxxxxxx>; Jason Gunthorpe
> >> > > > <jgg@xxxxxxxx>; Ajay Sharma <sharmaajay@xxxxxxxxxxxxx>; Dexuan
> >> > > > Cui <decui@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> >> > > > Wei Liu
> >> > <wei.liu@xxxxxxxxxx>; David S.
> >> > > > Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet
> >> > > > <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> >> > > > Abeni <pabeni@xxxxxxxxxx>;
> >> > > > linux- rdma@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx;
> >> > > > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> > > > Subject: Re: [PATCH] RDMA/mana_ib: Use v2 version of
> >> > > > cfg_rx_steer_req to enable RX coalescing
> >> > > >
> >> > > > On Sun, May 07, 2023 at 09:39:27PM +0000, Haiyang Zhang wrote:
> >> > > > >
> >> > > > >
> >> > > > > > -----Original Message-----
> >> > > > > > From: Leon Romanovsky <leon@xxxxxxxxxx>
> >> > > > > > Sent: Sunday, May 7, 2023 4:11 AM
> >> > > > > > To: Long Li <longli@xxxxxxxxxxxxx>
> >> > > > > > Cc: Jason Gunthorpe <jgg@xxxxxxxx>; Ajay Sharma
> >> > > > > > <sharmaajay@xxxxxxxxxxxxx>; Dexuan Cui
> >> > > > > > <decui@xxxxxxxxxxxxx>; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> >> > > > > > Haiyang Zhang
> >> > > > <haiyangz@xxxxxxxxxxxxx>;
> >> > > > > > Wei Liu <wei.liu@xxxxxxxxxx>; David S. Miller
> >> > > > > > <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>;
> >> > Jakub
> >> > > > > > Kicinski <kuba@xxxxxxxxxx>;
> >> > > > Paolo
> >> > > > > > Abeni <pabeni@xxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx;
> >> > > > > > linux- hyperv@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> >> > > > > > linux- kernel@xxxxxxxxxxxxxxx
> >> > > > > > Subject: Re: [PATCH] RDMA/mana_ib: Use v2 version of
> >> > > > > > cfg_rx_steer_req
> >> > > > to
> >> > > > > > enable RX coalescing
> >> > > > > >
> >> > > > > > On Fri, May 05, 2023 at 11:51:48AM -0700,
> >> > > > > > longli@xxxxxxxxxxxxxxxxx
> >> > > > > > wrote:
> >> > > > > > > From: Long Li <longli@xxxxxxxxxxxxx>
> >> > > > > > >
> >> > > > > > > With RX coalescing, one CQE entry can be used to indicate
> >> > > > > > > multiple
> >> > > > packets
> >> > > > > > > on the receive queue. This saves processing time and PCI
> >> > > > > > > bandwidth over the CQ.
> >> > > > > > >
> >> > > > > > > Signed-off-by: Long Li <longli@xxxxxxxxxxxxx>
> >> > > > > > > ---
> >> > > > > > >  drivers/infiniband/hw/mana/qp.c |  5 ++++-
> >> > > > > > >  include/net/mana/mana.h         | 17 +++++++++++++++++
> >> > > > > > >  2 files changed, 21 insertions(+), 1 deletion(-)
> >> > > > > >
> >> > > > > > Why didn't you change mana_cfg_vport_steering() too?
> >> > > > >
> >> > > > > The mana_cfg_vport_steering() is for mana_en (Enthernet)
> >> > > > > driver, not the mana_ib driver.
> >> > > > >
> >> > > > > The changes for mana_en will be done in a separate patch
> >> > > > > together with changes for mana_en RX code patch to support
> >> > > > > multiple packets /
> >> > CQE.
> >> > > >
> >> > > > I'm aware of the difference between mana_en and mana_ib.
> >> > > >
> >> > > > The change you proposed doesn't depend on "support multiple
> >> > > > packets / CQE."
> >> > > > and works perfectly with one packet/CQE also, does it?
> >> > >
> >> > > No.
> >> > > If we add the following setting to the mana_en /
> >> > > mana_cfg_vport_steering(), the NIC may put multiple packets in one
> >> > > CQE, so we need to have the changes for mana_en RX code path to
> >> > > support
> >> > multiple packets / CQE.
> >> > > +	req->cqe_coalescing_enable = true;
> >> >
> >> > You can leave "cqe_coalescing_enable = false" for ETH and still
> >> > reuse your new
> >> > v2 struct.
> >>
> >> I think your proposal will work for both Ethernet and IB.
> >>
> >> The idea is that we want this patch to change the behavior of the IB driver. We
> >plan to make another patch for the Ethernet driver. This makes it easier to track
> >all changes for a driver.
> >
> >And I don't want to deal with deletion of v1 struct for two/three kernel cycles
> >instead of one patch in one cycle.
> 
> I'm resubmitting this patch to replace v1 for both driver.

Great

> 
> >
> >>
> >> >
> >> > H>
> >> > > So we plan to set this cqe_coalescing_enable, and the changes for
> >> > > mana_en RX code path to support multiple packets / CQE in another
> >patch.
> >> >
> >> > And how does it work with IB without changing anything except this
> >> > proposed patch?
> >>
> >> The RX CQE Coalescing is implemented in the user-mode. This feature is
> >always turned on from cluster. The user-mode code is written in a way that can
> >deal with both CQE Coalescing and CQE non-coalescing, so it doesn't depend on
> >kernel version for the correct behavior.
> >
> >Yes, but how does userspace know that CQE coalescing was enabled?
> 
> The user-mode doesn't know if CQE is enabled in advance. If this information is required I can modify the patch to pass this information along to rdma-core. However, this is not useful as the cluster is running with CQE coalescing enabled by default, there is no need to know this information.

It is ok, not needed. if we can do something without UAPI involvement,
it will be always preferable way.

Thanks