diff mbox series

[for-next,v6,09/10] RDMA/cm: Make QP FLUSHABLE

Message ID 20221116081951.32750-10-lizhijian@fujitsu.com
State New
Headers
Series RDMA/rxe: Add RDMA FLUSH operation |

Commit Message

Zhijian Li (Fujitsu) Nov. 16, 2022, 8:19 a.m. UTC 
  It enables flushable access flag for qp

Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
---
V5: new patch, inspired by Bob
---
 drivers/infiniband/core/cm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe Nov. 22, 2022, 2:52 p.m. UTC | #1 
On Wed, Nov 16, 2022 at 04:19:50PM +0800, Li Zhijian wrote:
> It enables flushable access flag for qp
> 
> Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> ---
> V5: new patch, inspired by Bob
> ---
>  drivers/infiniband/core/cm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index 1f9938a2c475..58837aac980b 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>  		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>  		if (cm_id_priv->responder_resources)
>  			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
> -						    IB_ACCESS_REMOTE_ATOMIC;
> +						    IB_ACCESS_REMOTE_ATOMIC |
> +						    IB_ACCESS_FLUSHABLE;

What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?

Do flush ops require a responder resource?

Why should CM set it unconditionally?

Explain in the commit message

Jason
Zhijian Li (Fujitsu) Nov. 23, 2022, 6:07 a.m. UTC | #2 
On 22/11/2022 22:52, Jason Gunthorpe wrote:
> On Wed, Nov 16, 2022 at 04:19:50PM +0800, Li Zhijian wrote:
>> It enables flushable access flag for qp
>>
>> Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>> ---
>> V5: new patch, inspired by Bob
>> ---
>>   drivers/infiniband/core/cm.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index 1f9938a2c475..58837aac980b 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>>   		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>>   		if (cm_id_priv->responder_resources)
>>   			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
>> -						    IB_ACCESS_REMOTE_ATOMIC;
>> +						    IB_ACCESS_REMOTE_ATOMIC |
>> +						    IB_ACCESS_FLUSHABLE;
> 
> What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?

Previous, responder of RXE will check qp_access_flags in check_op_valid():
  256 static enum resp_states check_op_valid(struct rxe_qp *qp, 

  257                                        struct rxe_pkt_info *pkt) 

  258 { 

  259         switch (qp_type(qp)) { 

  260         case IB_QPT_RC: 

  261                 if (((pkt->mask & RXE_READ_MASK) && 

  262                      !(qp->attr.qp_access_flags & 
IB_ACCESS_REMOTE_READ)) || 
 

  263                     ((pkt->mask & RXE_WRITE_MASK) && 

  264                      !(qp->attr.qp_access_flags & 
IB_ACCESS_REMOTE_WRITE)) ||
  265                     ((pkt->mask & RXE_ATOMIC_MASK) && 

  266                      !(qp->attr.qp_access_flags & 
IB_ACCESS_REMOTE_ATOMIC))) {
  267                         return RESPST_ERR_UNSUPPORTED_OPCODE; 

  268                 }

based on this, additional IB_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL 
were added in patch7 since V5 suggested by Bob.
Because of this change, QP should become FLUSHABLE correspondingly.

> 
> Do flush ops require a responder resource?

Yes, i think so. See IBA spec, oA19-9: FLUSH shall consume a single 
responder...


> 
> Why should CM set it unconditionally?
> 

I had ever checked git history log of qp->qp_access_flags, they did as 
it's. So i also think qp_access_flags should accept all new IBA 
abilities unconditionally.

What do you think of this @Jason


Thanks
Zhijian
> Explain in the commit message
> 
> Jason
Jason Gunthorpe Nov. 24, 2022, 5:39 p.m. UTC | #3 
On Wed, Nov 23, 2022 at 06:07:37AM +0000, lizhijian@fujitsu.com wrote:
> 
> 
> On 22/11/2022 22:52, Jason Gunthorpe wrote:
> > On Wed, Nov 16, 2022 at 04:19:50PM +0800, Li Zhijian wrote:
> >> It enables flushable access flag for qp
> >>
> >> Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
> >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >> ---
> >> V5: new patch, inspired by Bob
> >> ---
> >>   drivers/infiniband/core/cm.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> >> index 1f9938a2c475..58837aac980b 100644
> >> --- a/drivers/infiniband/core/cm.c
> >> +++ b/drivers/infiniband/core/cm.c
> >> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
> >>   		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
> >>   		if (cm_id_priv->responder_resources)
> >>   			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
> >> -						    IB_ACCESS_REMOTE_ATOMIC;
> >> +						    IB_ACCESS_REMOTE_ATOMIC |
> >> +						    IB_ACCESS_FLUSHABLE;
> > 
> > What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?
> 
> Previous, responder of RXE will check qp_access_flags in check_op_valid():
>   256 static enum resp_states check_op_valid(struct rxe_qp *qp, 
> 
>   257                                        struct rxe_pkt_info *pkt) 
> 
>   258 { 
> 
>   259         switch (qp_type(qp)) { 
> 
>   260         case IB_QPT_RC: 
> 
>   261                 if (((pkt->mask & RXE_READ_MASK) && 
> 
>   262                      !(qp->attr.qp_access_flags & 
> IB_ACCESS_REMOTE_READ)) || 
>  
> 
>   263                     ((pkt->mask & RXE_WRITE_MASK) && 
> 
>   264                      !(qp->attr.qp_access_flags & 
> IB_ACCESS_REMOTE_WRITE)) ||
>   265                     ((pkt->mask & RXE_ATOMIC_MASK) && 
> 
>   266                      !(qp->attr.qp_access_flags & 
> IB_ACCESS_REMOTE_ATOMIC))) {
>   267                         return RESPST_ERR_UNSUPPORTED_OPCODE; 
> 
>   268                 }
> 
> based on this, additional IB_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL 
> were added in patch7 since V5 suggested by Bob.
> Because of this change, QP should become FLUSHABLE correspondingly.

But nothing ever reads IB_ACCESS_FLUSHABLE, so why is it added?

Jason
Zhijian Li (Fujitsu) Nov. 25, 2022, 2:22 a.m. UTC | #4 
On 25/11/2022 01:39, Jason Gunthorpe wrote:
> On Wed, Nov 23, 2022 at 06:07:37AM +0000, lizhijian@fujitsu.com wrote:
>>
>>
>> On 22/11/2022 22:52, Jason Gunthorpe wrote:
>>> On Wed, Nov 16, 2022 at 04:19:50PM +0800, Li Zhijian wrote:
>>>> It enables flushable access flag for qp
>>>>
>>>> Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
>>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
>>>> ---
>>>> V5: new patch, inspired by Bob
>>>> ---
>>>>    drivers/infiniband/core/cm.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>>>> index 1f9938a2c475..58837aac980b 100644
>>>> --- a/drivers/infiniband/core/cm.c
>>>> +++ b/drivers/infiniband/core/cm.c
>>>> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>>>>    		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>>>>    		if (cm_id_priv->responder_resources)
>>>>    			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
>>>> -						    IB_ACCESS_REMOTE_ATOMIC;
>>>> +						    IB_ACCESS_REMOTE_ATOMIC |
>>>> +						    IB_ACCESS_FLUSHABLE;
>>>
>>> What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?
>>
>> Previous, responder of RXE will check qp_access_flags in check_op_valid():
>>    256 static enum resp_states check_op_valid(struct rxe_qp *qp,
>>
>>    257                                        struct rxe_pkt_info *pkt)
>>
>>    258 {
>>
>>    259         switch (qp_type(qp)) {
>>
>>    260         case IB_QPT_RC:
>>
>>    261                 if (((pkt->mask & RXE_READ_MASK) &&
>>
>>    262                      !(qp->attr.qp_access_flags &
>> IB_ACCESS_REMOTE_READ)) ||
>>   
>>
>>    263                     ((pkt->mask & RXE_WRITE_MASK) &&
>>
>>    264                      !(qp->attr.qp_access_flags &
>> IB_ACCESS_REMOTE_WRITE)) ||
>>    265                     ((pkt->mask & RXE_ATOMIC_MASK) &&
>>
>>    266                      !(qp->attr.qp_access_flags &
>> IB_ACCESS_REMOTE_ATOMIC))) {
>>    267                         return RESPST_ERR_UNSUPPORTED_OPCODE;
>>
>>    268                 }
>>
>> based on this, additional IB_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL
>> were added in patch7 since V5 suggested by Bob.
>> Because of this change, QP should become FLUSHABLE correspondingly.
> 
> But nothing ever reads IB_ACCESS_FLUSHABLE, so why is it added?

include/rdma/ib_verbs.h:
+	IB_ACCESS_FLUSH_GLOBAL = IB_UVERBS_ACCESS_FLUSH_GLOBAL,
+	IB_ACCESS_FLUSH_PERSISTENT = IB_UVERBS_ACCESS_FLUSH_PERSISTENT,
+	IB_ACCESS_FLUSHABLE = IB_ACCESS_FLUSH_GLOBAL |
+			      IB_ACCESS_FLUSH_PERSISTENT,

IB_ACCESS_FLUSHABLE is a wrapper of IB_ACCESS_FLUSH_GLOBAL | 
IB_ACCESS_FLUSH_PERSISTENT. With this wrapper, i will write one less 
line of code :)

I'm fine to expand it in next version.


> 
> Jason
Jason Gunthorpe Nov. 25, 2022, 2:16 p.m. UTC | #5 
On Fri, Nov 25, 2022 at 02:22:24AM +0000, lizhijian@fujitsu.com wrote:
> 
> 
> On 25/11/2022 01:39, Jason Gunthorpe wrote:
> > On Wed, Nov 23, 2022 at 06:07:37AM +0000, lizhijian@fujitsu.com wrote:
> >>
> >>
> >> On 22/11/2022 22:52, Jason Gunthorpe wrote:
> >>> On Wed, Nov 16, 2022 at 04:19:50PM +0800, Li Zhijian wrote:
> >>>> It enables flushable access flag for qp
> >>>>
> >>>> Reviewed-by: Zhu Yanjun <zyjzyj2000@gmail.com>
> >>>> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
> >>>> ---
> >>>> V5: new patch, inspired by Bob
> >>>> ---
> >>>>    drivers/infiniband/core/cm.c | 3 ++-
> >>>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> >>>> index 1f9938a2c475..58837aac980b 100644
> >>>> --- a/drivers/infiniband/core/cm.c
> >>>> +++ b/drivers/infiniband/core/cm.c
> >>>> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
> >>>>    		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
> >>>>    		if (cm_id_priv->responder_resources)
> >>>>    			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
> >>>> -						    IB_ACCESS_REMOTE_ATOMIC;
> >>>> +						    IB_ACCESS_REMOTE_ATOMIC |
> >>>> +						    IB_ACCESS_FLUSHABLE;
> >>>
> >>> What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?
> >>
> >> Previous, responder of RXE will check qp_access_flags in check_op_valid():
> >>    256 static enum resp_states check_op_valid(struct rxe_qp *qp,
> >>
> >>    257                                        struct rxe_pkt_info *pkt)
> >>
> >>    258 {
> >>
> >>    259         switch (qp_type(qp)) {
> >>
> >>    260         case IB_QPT_RC:
> >>
> >>    261                 if (((pkt->mask & RXE_READ_MASK) &&
> >>
> >>    262                      !(qp->attr.qp_access_flags &
> >> IB_ACCESS_REMOTE_READ)) ||
> >>   
> >>
> >>    263                     ((pkt->mask & RXE_WRITE_MASK) &&
> >>
> >>    264                      !(qp->attr.qp_access_flags &
> >> IB_ACCESS_REMOTE_WRITE)) ||
> >>    265                     ((pkt->mask & RXE_ATOMIC_MASK) &&
> >>
> >>    266                      !(qp->attr.qp_access_flags &
> >> IB_ACCESS_REMOTE_ATOMIC))) {
> >>    267                         return RESPST_ERR_UNSUPPORTED_OPCODE;
> >>
> >>    268                 }
> >>
> >> based on this, additional IB_FLUSH_PERSISTENT and IB_ACCESS_FLUSH_GLOBAL
> >> were added in patch7 since V5 suggested by Bob.
> >> Because of this change, QP should become FLUSHABLE correspondingly.
> > 
> > But nothing ever reads IB_ACCESS_FLUSHABLE, so why is it added?
> 
> include/rdma/ib_verbs.h:
> +	IB_ACCESS_FLUSH_GLOBAL = IB_UVERBS_ACCESS_FLUSH_GLOBAL,
> +	IB_ACCESS_FLUSH_PERSISTENT = IB_UVERBS_ACCESS_FLUSH_PERSISTENT,
> +	IB_ACCESS_FLUSHABLE = IB_ACCESS_FLUSH_GLOBAL |
> +			      IB_ACCESS_FLUSH_PERSISTENT,
> 
> IB_ACCESS_FLUSHABLE is a wrapper of IB_ACCESS_FLUSH_GLOBAL | 
> IB_ACCESS_FLUSH_PERSISTENT. With this wrapper, i will write one less 
> line of code :)
> 
> I'm fine to expand it in next version.

OIC, that is why it escaped grep

But this is back to my original question - why is it OK to do this
here in CMA? Shouldn't this cause other drivers to refuse to create
the QP because they don't support the flag?

Jason

> 
> > 
> > Jason
Zhijian Li (Fujitsu) Nov. 28, 2022, 10:23 a.m. UTC | #6 
On 25/11/2022 22:16, Jason Gunthorpe wrote:
>>>>>> ---
>>>>>>     drivers/infiniband/core/cm.c | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>>>>>> index 1f9938a2c475..58837aac980b 100644
>>>>>> --- a/drivers/infiniband/core/cm.c
>>>>>> +++ b/drivers/infiniband/core/cm.c
>>>>>> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>>>>>>     		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>>>>>>     		if (cm_id_priv->responder_resources)
>>>>>>     			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
>>>>>> -						    IB_ACCESS_REMOTE_ATOMIC;
>>>>>> +						    IB_ACCESS_REMOTE_ATOMIC |
>>>>>> +						    IB_ACCESS_FLUSHABLE;
>>>>> What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?

>> I'm fine to expand it in next version.
> OIC, that is why it escaped grep
> 
> But this is back to my original question - why is it OK to do this
> here in CMA? Shouldn't this cause other drivers to refuse to create
> the QP because they don't support the flag?
> 

Jason,

My flush example got wrong since V5 where responder side does 
qp_access_flags check. so i added this patch.

I also agreed it's a good idea that we should only add this flush flag 
to the supported drivers. But i haven't figured out how to achieve it in 
current RDMA.

After more digging into rdma-core, i found that this flag can be also 
set from usespace by calling ibv_modify_qp().
For server side(responder), ibv_modify_qp() must be called after 
rdma_accept(). rdma_accept() inside will modify qp_access_flags 
again(rdma_get_request is the first place to modify qp_access_flags).

Back to the original question, IIUC, current rdma-core have no API to 
set qp_access_flags during qp creating.

FLUSH operation in responder side will check both mr->access_flags and 
qp_access_flags. So unsupported device/driver are not able to do flush 
at all though qp_access_flags apply to all drivers.


------------------------------------------
(gdb) bt
#0  __ibv_modify_qp_1_1 (qp=0x40fbf0, attr=0x7fffffffd920, attr_mask=57)
     at /home/lizhijian/rdma-core/libibverbs/verbs.c:715
#1  0x00007ffff7faa1db in ucma_init_conn_qp (id_priv=0x40f6d0, qp=0x40fbf0)
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1380
#2  0x00007ffff7faada2 in rdma_create_qp_ex (id=0x40f6d0, 
attr=0x7fffffffda30)
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1676
#3  0x00007ffff7faae94 in rdma_create_qp (id=0x40f6d0, pd=0x407710, 
qp_init_attr=0x7fffffffdae0)
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1702
#4  0x00007ffff7fab5d3 in rdma_get_request (listen=0x40ede0, id=0x4051a8 
<id>)
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1883
#5  0x0000000000401af9 in run () at 
/home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:91
#6  0x00000000004021ea in main (argc=7, argv=0x7fffffffe228)
     at /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:282

(gdb) bt
#0  __ibv_modify_qp_1_1 (qp=0x40fbf0, attr=0x7fffffffd930, 
attr_mask=1216897)
     at /home/lizhijian/rdma-core/libibverbs/verbs.c:715
#1  0x00007ffff7fa9f16 in ucma_modify_qp_rtr (id=0x40f6d0, resp_res=128 
'\200')
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1304
#2  0x00007ffff7fab80a in rdma_accept (id=0x40f6d0, conn_param=0x0)
     at /home/lizhijian/rdma-core/librdmacm/cma.c:1932
#3  0x0000000000401c8a in run () at 
/home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:132
#4  0x00000000004021ea in main (argc=7, argv=0x7fffffffe228)
     at /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:282


> Jason
>
Zhijian Li (Fujitsu) Dec. 5, 2022, 10:07 a.m. UTC | #7 
Jason

Could you take a look at this update:
- Make QP FLUSHABLE for supported device only
- add more explanation

commit 1a95f94125b59183d5fc643a917e0a2e7bb07981
Author: Li Zhijian <lizhijian@fujitsu.com>
Date:   Mon Sep 26 17:53:06 2022 +0800

     RDMA/cm: Make QP FLUSHABLE for supported device
     
     Similar to RDMA and Atomic qp attributes enabled by default in CM, enable
     FLUSH attribute for supported device. That makes applications that are
     built with rdma_create_ep, rdma_accept APIs have FLUSH qp attribute
     natively so that user is able to request FLUSH operation simpler.
     
     Note that, a FLUSH operation requires FLUSH are supported by both
     device(HCA) and memory region(MR) and QP at the same time, so it's safe
     to enable FLUSH qp attribute by default here.
     
     FLUSH attribute can be disable by modify_qp() interface.
     
     Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>
     ---
     V6: enable flush for supported device only #Jason
     V5: new patch, inspired by Bob
     Signed-off-by: Li Zhijian <lizhijian@fujitsu.com>

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 1f9938a2c475..603c0aecc361 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4094,9 +4094,18 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
                 *qp_attr_mask = IB_QP_STATE | IB_QP_ACCESS_FLAGS |
                                 IB_QP_PKEY_INDEX | IB_QP_PORT;
                 qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
-               if (cm_id_priv->responder_resources)
+               if (cm_id_priv->responder_resources) {
+                       struct ib_device *ib_dev = cm_id_priv->id.device;
+                       u64 support_flush = ib_dev->attrs.device_cap_flags &
+                         (IB_DEVICE_FLUSH_GLOBAL | IB_DEVICE_FLUSH_PERSISTENT);
+                       u32 flushable = support_flush ?
+                                       (IB_ACCESS_FLUSH_GLOBAL |
+                                        IB_ACCESS_FLUSH_PERSISTENT) : 0;
+
                         qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
-                                                   IB_ACCESS_REMOTE_ATOMIC;
+                                                   IB_ACCESS_REMOTE_ATOMIC |
+                                                   flushable;
+               }
                 qp_attr->pkey_index = cm_id_priv->av.pkey_index;
                 if (cm_id_priv->av.port)
                         qp_attr->port_num = cm_id_priv->av.port->port_num;

Thanks
Zhijian


On 28/11/2022 18:23, lizhijian@fujitsu.com wrote:
> 
> 
> On 25/11/2022 22:16, Jason Gunthorpe wrote:
>>>>>>> ---
>>>>>>>      drivers/infiniband/core/cm.c | 3 ++-
>>>>>>>      1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>>>>>>> index 1f9938a2c475..58837aac980b 100644
>>>>>>> --- a/drivers/infiniband/core/cm.c
>>>>>>> +++ b/drivers/infiniband/core/cm.c
>>>>>>> @@ -4096,7 +4096,8 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>>>>>>>      		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>>>>>>>      		if (cm_id_priv->responder_resources)
>>>>>>>      			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
>>>>>>> -						    IB_ACCESS_REMOTE_ATOMIC;
>>>>>>> +						    IB_ACCESS_REMOTE_ATOMIC |
>>>>>>> +						    IB_ACCESS_FLUSHABLE;
>>>>>> What is the point of this? Nothing checks IB_ACCESS_FLUSHABLE ?
> 
>>> I'm fine to expand it in next version.
>> OIC, that is why it escaped grep
>>
>> But this is back to my original question - why is it OK to do this
>> here in CMA? Shouldn't this cause other drivers to refuse to create
>> the QP because they don't support the flag?
>>
> 
> Jason,
> 
> My flush example got wrong since V5 where responder side does
> qp_access_flags check. so i added this patch.
> 
> I also agreed it's a good idea that we should only add this flush flag
> to the supported drivers. But i haven't figured out how to achieve it in
> current RDMA.
> 
> After more digging into rdma-core, i found that this flag can be also
> set from usespace by calling ibv_modify_qp().
> For server side(responder), ibv_modify_qp() must be called after
> rdma_accept(). rdma_accept() inside will modify qp_access_flags
> again(rdma_get_request is the first place to modify qp_access_flags).
> 
> Back to the original question, IIUC, current rdma-core have no API to
> set qp_access_flags during qp creating.
> 
> FLUSH operation in responder side will check both mr->access_flags and
> qp_access_flags. So unsupported device/driver are not able to do flush
> at all though qp_access_flags apply to all drivers.
> 
> 
> ------------------------------------------
> (gdb) bt
> #0  __ibv_modify_qp_1_1 (qp=0x40fbf0, attr=0x7fffffffd920, attr_mask=57)
>       at /home/lizhijian/rdma-core/libibverbs/verbs.c:715
> #1  0x00007ffff7faa1db in ucma_init_conn_qp (id_priv=0x40f6d0, qp=0x40fbf0)
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1380
> #2  0x00007ffff7faada2 in rdma_create_qp_ex (id=0x40f6d0,
> attr=0x7fffffffda30)
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1676
> #3  0x00007ffff7faae94 in rdma_create_qp (id=0x40f6d0, pd=0x407710,
> qp_init_attr=0x7fffffffdae0)
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1702
> #4  0x00007ffff7fab5d3 in rdma_get_request (listen=0x40ede0, id=0x4051a8
> <id>)
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1883
> #5  0x0000000000401af9 in run () at
> /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:91
> #6  0x00000000004021ea in main (argc=7, argv=0x7fffffffe228)
>       at /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:282
> 
> (gdb) bt
> #0  __ibv_modify_qp_1_1 (qp=0x40fbf0, attr=0x7fffffffd930,
> attr_mask=1216897)
>       at /home/lizhijian/rdma-core/libibverbs/verbs.c:715
> #1  0x00007ffff7fa9f16 in ucma_modify_qp_rtr (id=0x40f6d0, resp_res=128
> '\200')
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1304
> #2  0x00007ffff7fab80a in rdma_accept (id=0x40f6d0, conn_param=0x0)
>       at /home/lizhijian/rdma-core/librdmacm/cma.c:1932
> #3  0x0000000000401c8a in run () at
> /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:132
> #4  0x00000000004021ea in main (argc=7, argv=0x7fffffffe228)
>       at /home/lizhijian/rdma-core/librdmacm/examples/rdma_flush_server.c:282
> 
> 
>> Jason
Jason Gunthorpe Dec. 5, 2022, 5:12 p.m. UTC | #8 
On Mon, Dec 05, 2022 at 10:07:11AM +0000, lizhijian@fujitsu.com wrote:
> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
> index 1f9938a2c475..603c0aecc361 100644
> --- a/drivers/infiniband/core/cm.c
> +++ b/drivers/infiniband/core/cm.c
> @@ -4094,9 +4094,18 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>                  *qp_attr_mask = IB_QP_STATE | IB_QP_ACCESS_FLAGS |
>                                  IB_QP_PKEY_INDEX | IB_QP_PORT;
>                  qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
> -               if (cm_id_priv->responder_resources)
> +               if (cm_id_priv->responder_resources) {
> +                       struct ib_device *ib_dev = cm_id_priv->id.device;
> +                       u64 support_flush = ib_dev->attrs.device_cap_flags &
> +                         (IB_DEVICE_FLUSH_GLOBAL | IB_DEVICE_FLUSH_PERSISTENT);
> +                       u32 flushable = support_flush ?
> +                                       (IB_ACCESS_FLUSH_GLOBAL |
> +                                        IB_ACCESS_FLUSH_PERSISTENT) : 0;
> +
>                          qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
> -                                                   IB_ACCESS_REMOTE_ATOMIC;
> +                                                   IB_ACCESS_REMOTE_ATOMIC |
> +                                                   flushable;
> +               }

This makes more sense

Jason
Zhijian Li (Fujitsu) Dec. 7, 2022, 1:25 a.m. UTC | #9 
On 06/12/2022 01:12, Jason Gunthorpe wrote:
> On Mon, Dec 05, 2022 at 10:07:11AM +0000, lizhijian@fujitsu.com wrote:
>> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
>> index 1f9938a2c475..603c0aecc361 100644
>> --- a/drivers/infiniband/core/cm.c
>> +++ b/drivers/infiniband/core/cm.c
>> @@ -4094,9 +4094,18 @@ static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
>>                   *qp_attr_mask = IB_QP_STATE | IB_QP_ACCESS_FLAGS |
>>                                   IB_QP_PKEY_INDEX | IB_QP_PORT;
>>                   qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
>> -               if (cm_id_priv->responder_resources)
>> +               if (cm_id_priv->responder_resources) {
>> +                       struct ib_device *ib_dev = cm_id_priv->id.device;
>> +                       u64 support_flush = ib_dev->attrs.device_cap_flags &
>> +                         (IB_DEVICE_FLUSH_GLOBAL | IB_DEVICE_FLUSH_PERSISTENT);
>> +                       u32 flushable = support_flush ?
>> +                                       (IB_ACCESS_FLUSH_GLOBAL |
>> +                                        IB_ACCESS_FLUSH_PERSISTENT) : 0;
>> +
>>                           qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
>> -                                                   IB_ACCESS_REMOTE_ATOMIC;
>> +                                                   IB_ACCESS_REMOTE_ATOMIC |
>> +                                                   flushable;
>> +               }
> 
> This makes more sense

thanks for your help, i have posted V7 revision.
https://lore.kernel.org/lkml/20221206130201.30986-1-lizhijian@fujitsu.com/T/#t

Thanks
Zhijian

> 
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index 1f9938a2c475..58837aac980b 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -4096,7 +4096,8 @@  static int cm_init_qp_init_attr(struct cm_id_private *cm_id_priv,
 		qp_attr->qp_access_flags = IB_ACCESS_REMOTE_WRITE;
 		if (cm_id_priv->responder_resources)
 			qp_attr->qp_access_flags |= IB_ACCESS_REMOTE_READ |
-						    IB_ACCESS_REMOTE_ATOMIC;
+						    IB_ACCESS_REMOTE_ATOMIC |
+						    IB_ACCESS_FLUSHABLE;
 		qp_attr->pkey_index = cm_id_priv->av.pkey_index;
 		if (cm_id_priv->av.port)
 			qp_attr->port_num = cm_id_priv->av.port->port_num;