RDMA/mana_ib: Use v2 version of cfg_rx_steer_req to enable RX coalescing

Commit Message

Long Li May 5, 2023, 6:51 p.m. UTC

From: Long Li <[e-mail]>

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 <[e-mail]>
---
 drivers/infiniband/hw/mana/qp.c |  5 ++++-
 include/net/mana/mana.h         | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

Comments

Leon Romanovsky May 7, 2023, 8:10 a.m. UTC | #1

On Fri, May 05, 2023 at 11:51:48AM -0700, [e-mail] wrote:
> From: Long Li <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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?

> 
> diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
> index 54b61930a7fd..83c768f96506 100644
> --- a/drivers/infiniband/hw/mana/qp.c
> +++ b/drivers/infiniband/hw/mana/qp.c
> @@ -13,7 +13,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
>  				      u8 *rx_hash_key)
>  {
>  	struct mana_port_context *mpc = netdev_priv(ndev);
> -	struct mana_cfg_rx_steer_req *req = NULL;
> +	struct mana_cfg_rx_steer_req_v2 *req = NULL;

There is no need in NULL here, req is going to be overwritten almost
immediately.

Thanks

>  	struct mana_cfg_rx_steer_resp resp = {};
>  	mana_handle_t *req_indir_tab;
>  	struct gdma_context *gc;
> @@ -33,6 +33,8 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
>  	mana_gd_init_req_hdr(&req->hdr, MANA_CONFIG_VPORT_RX, req_buf_size,
>  			     sizeof(resp));
>  
> +	req->hdr.req.msg_version = GDMA_MESSAGE_V2;
> +
>  	req->vport = mpc->port_handle;
>  	req->rx_enable = 1;
>  	req->update_default_rxobj = 1;
> @@ -46,6 +48,7 @@ static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
>  	req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
>  	req->indir_tab_offset = sizeof(*req);
>  	req->update_indir_tab = true;
> +	req->cqe_coalescing_enable = true;
>  
>  	req_indir_tab = (mana_handle_t *)(req + 1);
>  	/* The ind table passed to the hardware must have
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index cd386aa7c7cc..f8314b7c386c 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -596,6 +596,23 @@ struct mana_cfg_rx_steer_req {
>  	u8 hashkey[MANA_HASH_KEY_SIZE];
>  }; /* HW DATA */
>  
> +struct mana_cfg_rx_steer_req_v2 {
> +	struct gdma_req_hdr hdr;
> +	mana_handle_t vport;
> +	u16 num_indir_entries;
> +	u16 indir_tab_offset;
> +	u32 rx_enable;
> +	u32 rss_enable;
> +	u8 update_default_rxobj;
> +	u8 update_hashkey;
> +	u8 update_indir_tab;
> +	u8 reserved;
> +	mana_handle_t default_rxobj;
> +	u8 hashkey[MANA_HASH_KEY_SIZE];
> +	u8 cqe_coalescing_enable;
> +	u8 reserved2[7];
> +}; /* HW DATA */
> +
>  struct mana_cfg_rx_steer_resp {
>  	struct gdma_resp_hdr hdr;
>  }; /* HW DATA */
> -- 
> 2.17.1
>

Haiyang Zhang May 7, 2023, 9:39 p.m. UTC | #2

> -----Original Message-----
> From: Leon Romanovsky <[e-mail]>
> Sent: Sunday, May 7, 2023 4:11 AM
> To: Long Li <[e-mail]>
> Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> Srinivasan <[e-mail]>; Haiyang Zhang <[e-mail]>;
> Wei Liu <[e-mail]>; David S. Miller <[e-mail]>; Eric
> Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> Abeni <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> 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, [e-mail]
> wrote:
> > From: Long Li <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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.

Thanks,
- Haiyang

Leon Romanovsky May 8, 2023, 6:09 a.m. UTC | #3

On Sun, May 07, 2023 at 09:39:27PM +0000, Haiyang Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <[e-mail]>
> > Sent: Sunday, May 7, 2023 4:11 AM
> > To: Long Li <[e-mail]>
> > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> > <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> > Srinivasan <[e-mail]>; Haiyang Zhang <[e-mail]>;
> > Wei Liu <[e-mail]>; David S. Miller <[e-mail]>; Eric
> > Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> > Abeni <[e-mail]>; [e-mail]; linux-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > 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, [e-mail]
> > wrote:
> > > From: Long Li <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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?

Thanks

> 
> Thanks,
> - Haiyang
>

Haiyang Zhang May 8, 2023, 2:45 p.m. UTC | #4

> -----Original Message-----
> From: Leon Romanovsky <[e-mail]>
> Sent: Monday, May 8, 2023 2:10 AM
> To: Haiyang Zhang <[e-mail]>
> Cc: Long Li <[e-mail]>; Jason Gunthorpe <[e-mail]>; Ajay
> Sharma <[e-mail]>; Dexuan Cui <[e-mail]>;
> KY Srinivasan <[e-mail]>; Wei Liu <[e-mail]>; David S.
> Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> > > Sent: Sunday, May 7, 2023 4:11 AM
> > > To: Long Li <[e-mail]>
> > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> > > <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> > > Srinivasan <[e-mail]>; Haiyang Zhang
> <[e-mail]>;
> > > Wei Liu <[e-mail]>; David S. Miller <[e-mail]>; Eric
> > > Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>;
> Paolo
> > > Abeni <[e-mail]>; [e-mail]; linux-
> > > [e-mail]; [e-mail]; linux-
> > > [e-mail]
> > > 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, [e-mail]
> > > wrote:
> > > > From: Long Li <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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;

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.

Thanks,
- Haiyang

Leon Romanovsky May 9, 2023, 7:30 a.m. UTC | #5

On Mon, May 08, 2023 at 02:45:44PM +0000, Haiyang Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Leon Romanovsky <[e-mail]>
> > Sent: Monday, May 8, 2023 2:10 AM
> > To: Haiyang Zhang <[e-mail]>
> > Cc: Long Li <[e-mail]>; Jason Gunthorpe <[e-mail]>; Ajay
> > Sharma <[e-mail]>; Dexuan Cui <[e-mail]>;
> > KY Srinivasan <[e-mail]>; Wei Liu <[e-mail]>; David S.
> > Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> > Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>; linux-
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > 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 <[e-mail]>
> > > > Sent: Sunday, May 7, 2023 4:11 AM
> > > > To: Long Li <[e-mail]>
> > > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> > > > <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> > > > Srinivasan <[e-mail]>; Haiyang Zhang
> > <[e-mail]>;
> > > > Wei Liu <[e-mail]>; David S. Miller <[e-mail]>; Eric
> > > > Dumazet <[e-mail]>; Jakub Kicinski <[e-mail]>;
> > Paolo
> > > > Abeni <[e-mail]>; [e-mail]; linux-
> > > > [e-mail]; [e-mail]; linux-
> > > > [e-mail]
> > > > 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, [e-mail]
> > > > wrote:
> > > > > From: Long Li <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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.

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?

Thanks

> 
> Thanks,
> - Haiyang
>

Long Li May 9, 2023, 7:08 p.m. UTC | #6

> 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 <[e-mail]>
> > > Sent: Monday, May 8, 2023 2:10 AM
> > > To: Haiyang Zhang <[e-mail]>
> > > Cc: Long Li <[e-mail]>; Jason Gunthorpe <[e-mail]>;
> > > Ajay Sharma <[e-mail]>; Dexuan Cui
> > > <[e-mail]>; KY Srinivasan <[e-mail]>; Wei Liu
> <[e-mail]>; David S.
> > > Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> > > Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>;
> > > linux- [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > 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 <[e-mail]>
> > > > > Sent: Sunday, May 7, 2023 4:11 AM
> > > > > To: Long Li <[e-mail]>
> > > > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> > > > > <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> > > > > Srinivasan <[e-mail]>; Haiyang Zhang
> > > <[e-mail]>;
> > > > > Wei Liu <[e-mail]>; David S. Miller
> > > > > <[e-mail]>; Eric Dumazet <[e-mail]>;
> Jakub
> > > > > Kicinski <[e-mail]>;
> > > Paolo
> > > > > Abeni <[e-mail]>; [e-mail]; linux-
> > > > > [e-mail]; [e-mail]; linux-
> > > > > [e-mail]
> > > > > 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,
> > > > > [e-mail]
> > > > > wrote:
> > > > > > From: Long Li <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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.

> 
> 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.
Thanks,
Long

Leon Romanovsky May 10, 2023, 6:58 a.m. UTC | #7

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 <[e-mail]>
> > > > Sent: Monday, May 8, 2023 2:10 AM
> > > > To: Haiyang Zhang <[e-mail]>
> > > > Cc: Long Li <[e-mail]>; Jason Gunthorpe <[e-mail]>;
> > > > Ajay Sharma <[e-mail]>; Dexuan Cui
> > > > <[e-mail]>; KY Srinivasan <[e-mail]>; Wei Liu
> > <[e-mail]>; David S.
> > > > Miller <[e-mail]>; Eric Dumazet <[e-mail]>;
> > > > Jakub Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>;
> > > > linux- [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]
> > > > 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 <[e-mail]>
> > > > > > Sent: Sunday, May 7, 2023 4:11 AM
> > > > > > To: Long Li <[e-mail]>
> > > > > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> > > > > > <[e-mail]>; Dexuan Cui <[e-mail]>; KY
> > > > > > Srinivasan <[e-mail]>; Haiyang Zhang
> > > > <[e-mail]>;
> > > > > > Wei Liu <[e-mail]>; David S. Miller
> > > > > > <[e-mail]>; Eric Dumazet <[e-mail]>;
> > Jakub
> > > > > > Kicinski <[e-mail]>;
> > > > Paolo
> > > > > > Abeni <[e-mail]>; [e-mail]; linux-
> > > > > > [e-mail]; [e-mail]; linux-
> > > > > > [e-mail]
> > > > > > 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,
> > > > > > [e-mail]
> > > > > > wrote:
> > > > > > > From: Long Li <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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.

> 
> > 
> > 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?

Thanks

> Thanks,
> Long

Long Li May 10, 2023, 4:37 p.m. UTC | #8

>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 <[e-mail]>
>> > > > Sent: Monday, May 8, 2023 2:10 AM
>> > > > To: Haiyang Zhang <[e-mail]>
>> > > > Cc: Long Li <[e-mail]>; Jason Gunthorpe
>> > > > <[e-mail]>; Ajay Sharma <[e-mail]>; Dexuan
>> > > > Cui <[e-mail]>; KY Srinivasan <[e-mail]>;
>> > > > Wei Liu
>> > <[e-mail]>; David S.
>> > > > Miller <[e-mail]>; Eric Dumazet
>> > > > <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
>> > > > Abeni <[e-mail]>;
>> > > > linux- [e-mail]; [e-mail];
>> > > > [e-mail]; [e-mail]
>> > > > 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 <[e-mail]>
>> > > > > > Sent: Sunday, May 7, 2023 4:11 AM
>> > > > > > To: Long Li <[e-mail]>
>> > > > > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
>> > > > > > <[e-mail]>; Dexuan Cui
>> > > > > > <[e-mail]>; KY Srinivasan <[e-mail]>;
>> > > > > > Haiyang Zhang
>> > > > <[e-mail]>;
>> > > > > > Wei Liu <[e-mail]>; David S. Miller
>> > > > > > <[e-mail]>; Eric Dumazet <[e-mail]>;
>> > Jakub
>> > > > > > Kicinski <[e-mail]>;
>> > > > Paolo
>> > > > > > Abeni <[e-mail]>; [e-mail];
>> > > > > > linux- [e-mail]; [e-mail];
>> > > > > > linux- [e-mail]
>> > > > > > 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,
>> > > > > > [e-mail]
>> > > > > > wrote:
>> > > > > > > From: Long Li <[e-mail]>
>> > > > > > >
>> > > > > > > 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 <[e-mail]>
>> > > > > > > ---
>> > > > > > >  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.

>
>>
>> >
>> > 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.

Leon Romanovsky May 10, 2023, 4:57 p.m. UTC | #9

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 <[e-mail]>
> >> > > > Sent: Monday, May 8, 2023 2:10 AM
> >> > > > To: Haiyang Zhang <[e-mail]>
> >> > > > Cc: Long Li <[e-mail]>; Jason Gunthorpe
> >> > > > <[e-mail]>; Ajay Sharma <[e-mail]>; Dexuan
> >> > > > Cui <[e-mail]>; KY Srinivasan <[e-mail]>;
> >> > > > Wei Liu
> >> > <[e-mail]>; David S.
> >> > > > Miller <[e-mail]>; Eric Dumazet
> >> > > > <[e-mail]>; Jakub Kicinski <[e-mail]>; Paolo
> >> > > > Abeni <[e-mail]>;
> >> > > > linux- [e-mail]; [e-mail];
> >> > > > [e-mail]; [e-mail]
> >> > > > 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 <[e-mail]>
> >> > > > > > Sent: Sunday, May 7, 2023 4:11 AM
> >> > > > > > To: Long Li <[e-mail]>
> >> > > > > > Cc: Jason Gunthorpe <[e-mail]>; Ajay Sharma
> >> > > > > > <[e-mail]>; Dexuan Cui
> >> > > > > > <[e-mail]>; KY Srinivasan <[e-mail]>;
> >> > > > > > Haiyang Zhang
> >> > > > <[e-mail]>;
> >> > > > > > Wei Liu <[e-mail]>; David S. Miller
> >> > > > > > <[e-mail]>; Eric Dumazet <[e-mail]>;
> >> > Jakub
> >> > > > > > Kicinski <[e-mail]>;
> >> > > > Paolo
> >> > > > > > Abeni <[e-mail]>; [e-mail];
> >> > > > > > linux- [e-mail]; [e-mail];
> >> > > > > > linux- [e-mail]
> >> > > > > > 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,
> >> > > > > > [e-mail]
> >> > > > > > wrote:
> >> > > > > > > From: Long Li <[e-mail]>
> >> > > > > > >
> >> > > > > > > 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 <[e-mail]>
> >> > > > > > > ---
> >> > > > > > >  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

Patch

diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c
index 54b61930a7fd..83c768f96506 100644
--- a/drivers/infiniband/hw/mana/qp.c
+++ b/drivers/infiniband/hw/mana/qp.c
@@ -13,7 +13,7 @@  static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
 				      u8 *rx_hash_key)
 {
 	struct mana_port_context *mpc = netdev_priv(ndev);
-	struct mana_cfg_rx_steer_req *req = NULL;
+	struct mana_cfg_rx_steer_req_v2 *req = NULL;
 	struct mana_cfg_rx_steer_resp resp = {};
 	mana_handle_t *req_indir_tab;
 	struct gdma_context *gc;
@@ -33,6 +33,8 @@  static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
 	mana_gd_init_req_hdr(&req->hdr, MANA_CONFIG_VPORT_RX, req_buf_size,
 			     sizeof(resp));
 
+	req->hdr.req.msg_version = GDMA_MESSAGE_V2;
+
 	req->vport = mpc->port_handle;
 	req->rx_enable = 1;
 	req->update_default_rxobj = 1;
@@ -46,6 +48,7 @@  static int mana_ib_cfg_vport_steering(struct mana_ib_dev *dev,
 	req->num_indir_entries = MANA_INDIRECT_TABLE_SIZE;
 	req->indir_tab_offset = sizeof(*req);
 	req->update_indir_tab = true;
+	req->cqe_coalescing_enable = true;
 
 	req_indir_tab = (mana_handle_t *)(req + 1);
 	/* The ind table passed to the hardware must have
diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
index cd386aa7c7cc..f8314b7c386c 100644
--- a/include/net/mana/mana.h
+++ b/include/net/mana/mana.h
@@ -596,6 +596,23 @@  struct mana_cfg_rx_steer_req {
 	u8 hashkey[MANA_HASH_KEY_SIZE];
 }; /* HW DATA */
 
+struct mana_cfg_rx_steer_req_v2 {
+	struct gdma_req_hdr hdr;
+	mana_handle_t vport;
+	u16 num_indir_entries;
+	u16 indir_tab_offset;
+	u32 rx_enable;
+	u32 rss_enable;
+	u8 update_default_rxobj;
+	u8 update_hashkey;
+	u8 update_indir_tab;
+	u8 reserved;
+	mana_handle_t default_rxobj;
+	u8 hashkey[MANA_HASH_KEY_SIZE];
+	u8 cqe_coalescing_enable;
+	u8 reserved2[7];
+}; /* HW DATA */
+
 struct mana_cfg_rx_steer_resp {
 	struct gdma_resp_hdr hdr;
 }; /* HW DATA */