Re: [PATCH net-next 1/2] net/smc: make wr buffer count configurable

[Halil Pasic <pasic@linux.ibm.com>]

On Fri, 5 Sep 2025 11:45:36 +0800
Dust Li <dust.li@linux.alibaba.com> wrote:

> On 2025-09-04 23:12:52, Halil Pasic wrote:
> >Think SMC_WR_BUF_CNT_SEND := SMC_WR_BUF_CNT used in send context and
> >SMC_WR_BUF_CNT_RECV := 3 * SMC_WR_BUF_CNT used in recv context. Those
> >get replaced with lgr->max_send_wr and lgr->max_recv_wr respective.  
> 
> Hi Halil,
> 
> I think making the WR buffer count configurable helps make SMC more flexible.

Hi Dust Li,

Thank you for having a look. 

> 
> However, there are two additional issues we need to consider:
> 
> 1. What if the two sides have different max_send_wr/max_recv_wr configurations?
> IIUC, For example, if the client sets max_send_wr to 64, but the server sets
> max_recv_wr to 16, the client might overflow the server's QP receive
> queue, potentially causing an RNR (Receiver Not Ready) error.

I don't think the 16 is spec-ed anywhere and if the client and the server
need to agree on the same value it should either be speced, or a
protocol mechanism for negotiating it needs to exist. So what is your
take on this as an SMC maintainer?

I think, we have tested heterogeneous setups and didn't see any grave
issues. But let me please do a follow up on this. Maybe the other
maintainers can chime in as well.

> 
> 2. Since WR buffers are configurable, it’d be helpful to add some
> monitoring methods so we can keep track of how many WR buffers each QP
> is currently using. This should be useful now that you introduced a fallback
> retry mechanism, which can cause the number of WR buffers to change
> dynamically.
> 

I agree, but I think that can be done in a different scope. I don't think
this needs to be a part of the MVP. Or do you think that it needs to
be part of the series?
> 
> Some other minor issues in the patch itself, see below
> 
> >
> >While at it let us also remove a confusing comment that is either not
> >about the context in which it resides (describing
> >qp_attr.cap.max_send_wr and qp_attr.cap.max_recv_wr) or not applicable
> >any more when these values become configurable .
> >
> >Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>
> >---
> > Documentation/networking/smc-sysctl.rst | 37 +++++++++++++++++++++++++
> > net/smc/smc.h                           |  2 ++
> > net/smc/smc_core.h                      |  4 +++
> > net/smc/smc_ib.c                        |  7 ++---
> > net/smc/smc_llc.c                       |  2 ++
> > net/smc/smc_sysctl.c                    | 22 +++++++++++++++
> > net/smc/smc_wr.c                        | 32 +++++++++++----------
> > net/smc/smc_wr.h                        |  2 --
> > 8 files changed, 86 insertions(+), 22 deletions(-)
> >
> >diff --git a/Documentation/networking/smc-sysctl.rst b/Documentation/networking/smc-sysctl.rst
> >index a874d007f2db..c687092329e3 100644
> >--- a/Documentation/networking/smc-sysctl.rst
> >+++ b/Documentation/networking/smc-sysctl.rst
> >@@ -71,3 +71,40 @@ smcr_max_conns_per_lgr - INTEGER
> > 	acceptable value ranges from 16 to 255. Only for SMC-R v2.1 and later.
> > 
> > 	Default: 255
> >+
> >+smcr_max_send_wr - INTEGER  
> 
> Why call it max ? But not something like smcr_send_wr_cnt ?

Because of the back-off mechanism. You are not guaranteed to get
this many but you are guaranteed to not get more.
> > static struct ctl_table smc_table[] = {
> > 	{
> >@@ -99,6 +103,24 @@ static struct ctl_table smc_table[] = {
> > 		.extra1		= SYSCTL_ZERO,
> > 		.extra2		= SYSCTL_ONE,
> > 	},
> >+	{
> >+		.procname       = "smcr_max_send_wr",
> >+		.data		= &smc_ib_sysctl_max_send_wr,
> >+		.maxlen         = sizeof(int),
> >+		.mode           = 0644,
> >+		.proc_handler   = proc_dointvec_minmax,
> >+		.extra1		= &smc_ib_sysctl_max_wr_min,
> >+		.extra2		= &smc_ib_sysctl_max_wr_max,
> >+	},
> >+	{
> >+		.procname       = "smcr_max_recv_wr",
> >+		.data		= &smc_ib_sysctl_max_recv_wr,
> >+		.maxlen         = sizeof(int),
> >+		.mode           = 0644,
> >+		.proc_handler   = proc_dointvec_minmax,  
> 
> It's better to use tab instead of space before those '=' here.
> 

I can definitely fix that and do a respin if you like. But I think
we need to sort out the other problems first.

Regards,
Halil