On Fri, 5 Sep 2025 11:45:36 +0800
Dust Li <dust.li@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >Reviewed-by: Wenjia Zhang <wenjia@xxxxxxxxxxxxx>
> >---
> > 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
