| Message ID | 20211229034438.1854908-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rxe: Check the last packet by RXE_END_MASK | expand |
On Wed, Dec 29, 2021 at 11:44:38AM +0800, Xiao Yang wrote: > It's wrong to check the last packet by RXE_COMP_MASK because the flag > is to indicate if responder needs to generate a completion. > > Fixes: 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") > Fixes: 8700e3e7c485 ("Soft RoCE driver") > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_resp.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Bob/Zhu is this OK? > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > index e8f435fa6e4d..380934e38923 100644 > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > @@ -814,6 +814,10 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) > return RESPST_ERR_INVALIDATE_RKEY; > } > > + if (pkt->mask & RXE_END_MASK) > + /* We successfully processed this new request. */ > + qp->resp.msn++; > + > /* next expected psn, read handles this separately */ > qp->resp.psn = (pkt->psn + 1) & BTH_PSN_MASK; > qp->resp.ack_psn = qp->resp.psn; > @@ -821,11 +825,9 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) > qp->resp.opcode = pkt->opcode; > qp->resp.status = IB_WC_SUCCESS; > > - if (pkt->mask & RXE_COMP_MASK) { > - /* We successfully processed this new request. */ > - qp->resp.msn++; > + if (pkt->mask & RXE_COMP_MASK) > return RESPST_COMPLETE; > - } else if (qp_type(qp) == IB_QPT_RC) > + else if (qp_type(qp) == IB_QPT_RC) > return RESPST_ACKNOWLEDGE; > else > return RESPST_CLEANUP; > -- > 2.25.1 > > >
Yes. On Wed, Jan 5, 2022 at 6:40 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Dec 29, 2021 at 11:44:38AM +0800, Xiao Yang wrote: > > It's wrong to check the last packet by RXE_COMP_MASK because the flag > > is to indicate if responder needs to generate a completion. > > > > Fixes: 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") > > Fixes: 8700e3e7c485 ("Soft RoCE driver") > > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > > --- > > drivers/infiniband/sw/rxe/rxe_resp.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > Bob/Zhu is this OK? > > > diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c > > index e8f435fa6e4d..380934e38923 100644 > > --- a/drivers/infiniband/sw/rxe/rxe_resp.c > > +++ b/drivers/infiniband/sw/rxe/rxe_resp.c > > @@ -814,6 +814,10 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) > > return RESPST_ERR_INVALIDATE_RKEY; > > } > > > > + if (pkt->mask & RXE_END_MASK) > > + /* We successfully processed this new request. */ > > + qp->resp.msn++; > > + > > /* next expected psn, read handles this separately */ > > qp->resp.psn = (pkt->psn + 1) & BTH_PSN_MASK; > > qp->resp.ack_psn = qp->resp.psn; > > @@ -821,11 +825,9 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) > > qp->resp.opcode = pkt->opcode; > > qp->resp.status = IB_WC_SUCCESS; > > > > - if (pkt->mask & RXE_COMP_MASK) { > > - /* We successfully processed this new request. */ > > - qp->resp.msn++; > > + if (pkt->mask & RXE_COMP_MASK) > > return RESPST_COMPLETE; > > - } else if (qp_type(qp) == IB_QPT_RC) > > + else if (qp_type(qp) == IB_QPT_RC) > > return RESPST_ACKNOWLEDGE; > > else > > return RESPST_CLEANUP; > > -- > > 2.25.1 > > > > > >
在 2022/1/6 8:40, Jason Gunthorpe 写道: > On Wed, Dec 29, 2021 at 11:44:38AM +0800, Xiao Yang wrote: >> It's wrong to check the last packet by RXE_COMP_MASK because the flag >> is to indicate if responder needs to generate a completion. >> >> Fixes: 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") >> Fixes: 8700e3e7c485 ("Soft RoCE driver") >> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> >> --- >> drivers/infiniband/sw/rxe/rxe_resp.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) > Bob/Zhu is this OK? Add david.marchand@6wind.com. From the following from the commit log of 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") " Logically, the requester associates a sequential Send Sequence Number (SSN) with each WQE posted to the send queue. The SSN bears a one- to-one relationship to the MSN returned by the responder in each re- sponse packet. Therefore, when the requester receives a response, it in- terprets the MSN as representing the SSN of the most recent request completed by the responder to determine which send WQE(s) can be completed." " It seems that it does not mean to check the last packet. It means that it receives a MSN response. Please David Marchand <david.marchand@6wind.com> to check this commit. Thanks a lot. Zhu Yanjun > >> diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c >> index e8f435fa6e4d..380934e38923 100644 >> --- a/drivers/infiniband/sw/rxe/rxe_resp.c >> +++ b/drivers/infiniband/sw/rxe/rxe_resp.c >> @@ -814,6 +814,10 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) >> return RESPST_ERR_INVALIDATE_RKEY; >> } >> >> + if (pkt->mask & RXE_END_MASK) >> + /* We successfully processed this new request. */ >> + qp->resp.msn++; >> + >> /* next expected psn, read handles this separately */ >> qp->resp.psn = (pkt->psn + 1) & BTH_PSN_MASK; >> qp->resp.ack_psn = qp->resp.psn; >> @@ -821,11 +825,9 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) >> qp->resp.opcode = pkt->opcode; >> qp->resp.status = IB_WC_SUCCESS; >> >> - if (pkt->mask & RXE_COMP_MASK) { >> - /* We successfully processed this new request. */ >> - qp->resp.msn++; >> + if (pkt->mask & RXE_COMP_MASK) >> return RESPST_COMPLETE; >> - } else if (qp_type(qp) == IB_QPT_RC) >> + else if (qp_type(qp) == IB_QPT_RC) >> return RESPST_ACKNOWLEDGE; >> else >> return RESPST_CLEANUP; >> -- >> 2.25.1 >> >> >>
On 2022/1/7 19:49, Yanjun Zhu wrote: > It seems that it does not mean to check the last packet. It means that > it receives a MSN response. Hi Yanjun, Checking the last packet is a way to indicate that responder has completed an entire request(including multiple packets) and then increases a msn. Best Regards, Xiao Yang
在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: > On 2022/1/7 19:49, Yanjun Zhu wrote: >> It seems that it does not mean to check the last packet. It means that >> it receives a MSN response. > Hi Yanjun, > > Checking the last packet is a way to indicate that responder has > completed an entire request(including multiple packets) and then > increases a msn. Hi, Xiao What does the msn mean? Thanks a lot. Zhu Yanjun > > Best Regards, > Xiao Yang
> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: > > > 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >> On 2022/1/7 19:49, Yanjun Zhu wrote: >>> It seems that it does not mean to check the last packet. It means that >>> it receives a MSN response. >> Hi Yanjun, >> >> Checking the last packet is a way to indicate that responder has >> completed an entire request(including multiple packets) and then >> increases a msn. > > Hi, Xiao > > What does the msn mean? Message Sequence Number. Thxs, Håkon > > Thanks a lot. > > Zhu Yanjun > >> >> Best Regards, >> Xiao Yang
在 2022/1/11 23:16, Haakon Bugge 写道: > >> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: >> >> >> 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >>> On 2022/1/7 19:49, Yanjun Zhu wrote: >>>> It seems that it does not mean to check the last packet. It means that >>>> it receives a MSN response. >>> Hi Yanjun, >>> >>> Checking the last packet is a way to indicate that responder has >>> completed an entire request(including multiple packets) and then >>> increases a msn. >> Hi, Xiao >> >> What does the msn mean? > Message Sequence Number. Thanks, Haakon I am reading the following from the spec. " C9-148: An HCA responder using Reliable Connection service shall initialize its MSN value to zero. The responder shall increment its MSN whenever it has successfully completed processing a new, valid request message. The MSN shall not be incremented for duplicate requests. The incremented MSN shall be returned in the last or only packet of an RDMA READ or Atomic response. For RDMA READ requests, the responder may increment its MSN after it has completed validating the request and before it has begun transmitting any of the requested data, and may return the incremented MSN in the AETH of the first response packet. The MSN shall be incremented only once for any given request message. " It seems that the above describe how to handle MSN increment in details. Zhu Yanjun > > > Thxs, Håkon > >> Thanks a lot. >> >> Zhu Yanjun >> >>> Best Regards, >>> Xiao Yang
On 2022/1/12 9:19, Yanjun Zhu wrote: > > 在 2022/1/11 23:16, Haakon Bugge 写道: >> >>> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: >>> >>> >>> 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >>>> On 2022/1/7 19:49, Yanjun Zhu wrote: >>>>> It seems that it does not mean to check the last packet. It means >>>>> that >>>>> it receives a MSN response. >>>> Hi Yanjun, >>>> >>>> Checking the last packet is a way to indicate that responder has >>>> completed an entire request(including multiple packets) and then >>>> increases a msn. >>> Hi, Xiao >>> >>> What does the msn mean? >> Message Sequence Number. > > Thanks, Haakon > > I am reading the following from the spec. > > " > > C9-148: An HCA responder using Reliable Connection service shall > initialize > > its MSN value to zero. The responder shall increment its MSN > whenever it has successfully completed processing a new, valid request > message. The MSN shall not be incremented for duplicate requests. The > incremented MSN shall be returned in the last or only packet of an RDMA > READ or Atomic response. For RDMA READ requests, the responder > may increment its MSN after it has completed validating the request and > before it has begun transmitting any of the requested data, and may > return > the incremented MSN in the AETH of the first response packet. The MSN > shall be incremented only once for any given request message. > > " > > It seems that the above describe how to handle MSN increment in details. Hi Yanjun, Sorry for the late reply. Right, 9.7.7.1 GENERATING MSN VALUE section explains Message Sequence Number(MSN). Best Regards, Xiao Yang > > Zhu Yanjun > >> >> >> Thxs, Håkon >> >>> Thanks a lot. >>> >>> Zhu Yanjun >>> >>>> Best Regards, >>>> Xiao Yang
在 2022/1/12 11:58, yangx.jy@fujitsu.com 写道: > On 2022/1/12 9:19, Yanjun Zhu wrote: >> 在 2022/1/11 23:16, Haakon Bugge 写道: >>>> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: >>>> >>>> >>>> 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >>>>> On 2022/1/7 19:49, Yanjun Zhu wrote: >>>>>> It seems that it does not mean to check the last packet. It means >>>>>> that >>>>>> it receives a MSN response. >>>>> Hi Yanjun, >>>>> >>>>> Checking the last packet is a way to indicate that responder has >>>>> completed an entire request(including multiple packets) and then >>>>> increases a msn. >>>> Hi, Xiao >>>> >>>> What does the msn mean? >>> Message Sequence Number. >> Thanks, Haakon >> >> I am reading the following from the spec. >> >> " >> >> C9-148: An HCA responder using Reliable Connection service shall >> initialize >> >> its MSN value to zero. The responder shall increment its MSN >> whenever it has successfully completed processing a new, valid request >> message. The MSN shall not be incremented for duplicate requests. The >> incremented MSN shall be returned in the last or only packet of an RDMA >> READ or Atomic response. For RDMA READ requests, the responder >> may increment its MSN after it has completed validating the request and >> before it has begun transmitting any of the requested data, and may >> return >> the incremented MSN in the AETH of the first response packet. The MSN >> shall be incremented only once for any given request message. >> >> " >> >> It seems that the above describe how to handle MSN increment in details. > Hi Yanjun, > > Sorry for the late reply. > > Right, 9.7.7.1 GENERATING MSN VALUE section explains Message Sequence Does your commit take into account of duplicate requests? Zhu Yanjun > Number(MSN). > > Best Regards, > Xiao Yang >> Zhu Yanjun >> >>> >>> Thxs, Håkon >>> >>>> Thanks a lot. >>>> >>>> Zhu Yanjun >>>> >>>>> Best Regards, >>>>> Xiao Yang
On 2022/1/15 11:38, Yanjun Zhu wrote: > > 在 2022/1/12 11:58, yangx.jy@fujitsu.com 写道: >> On 2022/1/12 9:19, Yanjun Zhu wrote: >>> 在 2022/1/11 23:16, Haakon Bugge 写道: >>>>> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: >>>>> >>>>> >>>>> 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >>>>>> On 2022/1/7 19:49, Yanjun Zhu wrote: >>>>>>> It seems that it does not mean to check the last packet. It means >>>>>>> that >>>>>>> it receives a MSN response. >>>>>> Hi Yanjun, >>>>>> >>>>>> Checking the last packet is a way to indicate that responder has >>>>>> completed an entire request(including multiple packets) and then >>>>>> increases a msn. >>>>> Hi, Xiao >>>>> >>>>> What does the msn mean? >>>> Message Sequence Number. >>> Thanks, Haakon >>> >>> I am reading the following from the spec. >>> >>> " >>> >>> C9-148: An HCA responder using Reliable Connection service shall >>> initialize >>> >>> its MSN value to zero. The responder shall increment its MSN >>> whenever it has successfully completed processing a new, valid request >>> message. The MSN shall not be incremented for duplicate requests. The >>> incremented MSN shall be returned in the last or only packet of an RDMA >>> READ or Atomic response. For RDMA READ requests, the responder >>> may increment its MSN after it has completed validating the request and >>> before it has begun transmitting any of the requested data, and may >>> return >>> the incremented MSN in the AETH of the first response packet. The MSN >>> shall be incremented only once for any given request message. >>> >>> " >>> >>> It seems that the above describe how to handle MSN increment in >>> details. >> Hi Yanjun, >> >> Sorry for the late reply. >> >> Right, 9.7.7.1 GENERATING MSN VALUE section explains Message Sequence > > Does your commit take into account of duplicate requests? Hi Yanjun, Responder will check duplicate requests by check_psn() and process them by duplicate_request(). According to the logic of duplicate_request(), responder doesn't increase msn for duplicate requests. Best Regards, Xiao Yang > > Zhu Yanjun > >> Number(MSN). >> >> Best Regards, >> Xiao Yang >>> Zhu Yanjun >>> >>>> >>>> Thxs, Håkon >>>> >>>>> Thanks a lot. >>>>> >>>>> Zhu Yanjun >>>>> >>>>>> Best Regards, >>>>>> Xiao Yang
在 2022/1/18 10:20, yangx.jy@fujitsu.com 写道: > On 2022/1/15 11:38, Yanjun Zhu wrote: >> 在 2022/1/12 11:58, yangx.jy@fujitsu.com 写道: >>> On 2022/1/12 9:19, Yanjun Zhu wrote: >>>> 在 2022/1/11 23:16, Haakon Bugge 写道: >>>>>> On 11 Jan 2022, at 15:42, Yanjun Zhu <yanjun.zhu@linux.dev> wrote: >>>>>> >>>>>> >>>>>> 在 2022/1/10 13:17, yangx.jy@fujitsu.com 写道: >>>>>>> On 2022/1/7 19:49, Yanjun Zhu wrote: >>>>>>>> It seems that it does not mean to check the last packet. It means >>>>>>>> that >>>>>>>> it receives a MSN response. >>>>>>> Hi Yanjun, >>>>>>> >>>>>>> Checking the last packet is a way to indicate that responder has >>>>>>> completed an entire request(including multiple packets) and then >>>>>>> increases a msn. >>>>>> Hi, Xiao >>>>>> >>>>>> What does the msn mean? >>>>> Message Sequence Number. >>>> Thanks, Haakon >>>> >>>> I am reading the following from the spec. >>>> >>>> " >>>> >>>> C9-148: An HCA responder using Reliable Connection service shall >>>> initialize >>>> >>>> its MSN value to zero. The responder shall increment its MSN >>>> whenever it has successfully completed processing a new, valid request >>>> message. The MSN shall not be incremented for duplicate requests. The >>>> incremented MSN shall be returned in the last or only packet of an RDMA >>>> READ or Atomic response. For RDMA READ requests, the responder >>>> may increment its MSN after it has completed validating the request and >>>> before it has begun transmitting any of the requested data, and may >>>> return >>>> the incremented MSN in the AETH of the first response packet. The MSN >>>> shall be incremented only once for any given request message. >>>> >>>> " >>>> >>>> It seems that the above describe how to handle MSN increment in >>>> details. >>> Hi Yanjun, >>> >>> Sorry for the late reply. >>> >>> Right, 9.7.7.1 GENERATING MSN VALUE section explains Message Sequence " ... Since the responder may choose to coalesce acknowledges, a single response packet may in fact acknowledge several request messages. Thus, when it receives a new MSN, the requester begins evaluating WQEs on its send queue beginning with the oldest outstanding WQE and progressing forward. ... " In the above, several request messages come. From the SPEC, msn should increase based on the number of request messages. Can your commit handle the above case? Zhu Yanjun >> Does your commit take into account of duplicate requests? > Hi Yanjun, > > Responder will check duplicate requests by check_psn() and process them > by duplicate_request(). > According to the logic of duplicate_request(), responder doesn't > increase msn for duplicate requests. > > Best Regards, > Xiao Yang >> Zhu Yanjun >> >>> Number(MSN). >>> >>> Best Regards, >>> Xiao Yang >>>> Zhu Yanjun >>>> >>>>> Thxs, Håkon >>>>> >>>>>> Thanks a lot. >>>>>> >>>>>> Zhu Yanjun >>>>>> >>>>>>> Best Regards, >>>>>>> Xiao Yang
On 2022/1/19 22:08, Yanjun Zhu wrote: > " > > ... > > Since the responder may choose to coalesce acknowledges, a single > response packet may in fact acknowledge > several request messages. Thus, when it receives a new MSN, the > requester begins evaluating WQEs on its send queue beginning with the > oldest outstanding WQE and progressing forward. > > ... > > " > > In the above, several request messages come. From the SPEC, msn should > increase based on the number of request messages. Hi Yanjun， Current logic shows that posting a WQE on SQ increases SSN (SSN++) and processing a WQE on responder successfully increases MSN (MSN++). I think current SoftRoce doesn't implement the rule you metioned. To be honest, the rule is not clear for me. when and how many acknowledges of several request messages can we coalesce? > > Can your commit handle the above case? No. Best Regards, Xiao Yang > > > Zhu Yanjun
在 2022/1/20 17:33, yangx.jy@fujitsu.com 写道: > On 2022/1/19 22:08, Yanjun Zhu wrote: >> " >> >> ... >> >> Since the responder may choose to coalesce acknowledges, a single >> response packet may in fact acknowledge >> several request messages. Thus, when it receives a new MSN, the >> requester begins evaluating WQEs on its send queue beginning with the >> oldest outstanding WQE and progressing forward. >> >> ... >> >> " >> >> In the above, several request messages come. From the SPEC, msn should >> increase based on the number of request messages. > Hi Yanjun， > > Current logic shows that posting a WQE on SQ increases SSN (SSN++) and > processing a WQE on responder successfully increases MSN (MSN++). > I think current SoftRoce doesn't implement the rule you metioned. > > To be honest, the rule is not clear for me. when and how many > acknowledges of several request messages can we coalesce? Hi, Jason Gunthorpe && Leon Romanovsky In Mellanox RDMA NIC, how this rule is implemented? Thanks a lot. Zhu Yanjun >> Can your commit handle the above case? > No. > > Best Regards, > Xiao Yang >> >> Zhu Yanjun
On Wed, Dec 29, 2021 at 11:44:38AM +0800, Xiao Yang wrote: > It's wrong to check the last packet by RXE_COMP_MASK because the flag > is to indicate if responder needs to generate a completion. > > Fixes: 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") > Fixes: 8700e3e7c485 ("Soft RoCE driver") > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_resp.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c index e8f435fa6e4d..380934e38923 100644 --- a/drivers/infiniband/sw/rxe/rxe_resp.c +++ b/drivers/infiniband/sw/rxe/rxe_resp.c @@ -814,6 +814,10 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) return RESPST_ERR_INVALIDATE_RKEY; } + if (pkt->mask & RXE_END_MASK) + /* We successfully processed this new request. */ + qp->resp.msn++; + /* next expected psn, read handles this separately */ qp->resp.psn = (pkt->psn + 1) & BTH_PSN_MASK; qp->resp.ack_psn = qp->resp.psn; @@ -821,11 +825,9 @@ static enum resp_states execute(struct rxe_qp *qp, struct rxe_pkt_info *pkt) qp->resp.opcode = pkt->opcode; qp->resp.status = IB_WC_SUCCESS; - if (pkt->mask & RXE_COMP_MASK) { - /* We successfully processed this new request. */ - qp->resp.msn++; + if (pkt->mask & RXE_COMP_MASK) return RESPST_COMPLETE; - } else if (qp_type(qp) == IB_QPT_RC) + else if (qp_type(qp) == IB_QPT_RC) return RESPST_ACKNOWLEDGE; else return RESPST_CLEANUP;
It's wrong to check the last packet by RXE_COMP_MASK because the flag is to indicate if responder needs to generate a completion. Fixes: 9fcd67d1772c ("IB/rxe: increment msn only when completing a request") Fixes: 8700e3e7c485 ("Soft RoCE driver") Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> --- drivers/infiniband/sw/rxe/rxe_resp.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)