[PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

The check for QP type different than XRC has wrongly excluded driver QP
types from the resource tracker.

Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/core/core_priv.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index 589ed805e0ad..3a8b0911c3bc 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -321,7 +321,9 @@ static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
 					  struct ib_udata *udata,
 					  struct ib_uobject *uobj)
 {
+	enum ib_qp_type qp_type = attr->qp_type;
 	struct ib_qp *qp;
+	bool is_xrc;
 
 	if (!dev->ops.create_qp)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -339,7 +341,8 @@ static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
 	 * and more importantly they are created internaly by driver,
 	 * see mlx5 create_dev_resources() as an example.
 	 */
-	if (attr->qp_type < IB_QPT_XRC_INI) {
+	is_xrc = qp_type == IB_QPT_XRC_INI || qp_type == IB_QPT_XRC_TGT;
+	if ((qp_type < IB_QPT_MAX && !is_xrc) || qp_type == IB_QPT_DRIVER) {
 		qp->res.type = RDMA_RESTRACK_QP;
 		if (uobj)
 			rdma_restrack_uadd(&qp->res);
-- 
2.22.0

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Jason Gunthorpe]

On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> The check for QP type different than XRC has wrongly excluded driver QP
> types from the resource tracker.

-rc commit messages need to describe the problem this is fixing from
the users perspective.

Jason

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

On 30/07/2019 15:19, Jason Gunthorpe wrote:
> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
>> The check for QP type different than XRC has wrongly excluded driver QP
>> types from the resource tracker.
> 
> -rc commit messages need to describe the problem this is fixing from
> the users perspective.

Thanks, will resubmit.

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Leon Romanovsky]

On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> The check for QP type different than XRC has wrongly excluded driver QP
> types from the resource tracker.
>
> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")

It is a little bit over to say "wrongly". At that time, we did it on purpose
because it was unclear how to represent such QP types to users and we didn't
have vendor specific hooks introduced by Steve later on.

I would like to see the output or "rdma res" and "rdma res show qp" with
running driver QP after your change.

Thanks

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

On 30/07/2019 16:38, Leon Romanovsky wrote:
> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
>> The check for QP type different than XRC has wrongly excluded driver QP
>> types from the resource tracker.
>>
>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
> 
> It is a little bit over to say "wrongly". At that time, we did it on purpose
> because it was unclear how to represent such QP types to users and we didn't
> have vendor specific hooks introduced by Steve later on.

It's very confusing to see a test running and zero QPs in "rdma res".
I'm fine with removing the "wrongly" :), but I still think this should be
targeted to for-rc as a bug fix.

> 
> I would like to see the output or "rdma res" and "rdma res show qp" with
> running driver QP after your change.

gal@server [~] $ rdma res
0: efa_0: pd 2 cq 4 qp 2 cm_id 0 mr 2 ctx 2
gal@server [~] $ rdma res show qp
link efa_0/1 lqpn 0 type UNKNOWN state RTS sq-psn 13400680 pdn 0 pid 17847 comm ib_send_bw
link efa_0/1 lqpn 1 type UNKNOWN state RTR sq-psn 0 pdn 1 pid 17820 comm ib_send_bw

We can change the UNKNOWN to DRIVER/something else in userspace code.

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Leon Romanovsky]

On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
> On 30/07/2019 16:38, Leon Romanovsky wrote:
> > On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> >> The check for QP type different than XRC has wrongly excluded driver QP
> >> types from the resource tracker.
> >>
> >> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
> >
> > It is a little bit over to say "wrongly". At that time, we did it on purpose
> > because it was unclear how to represent such QP types to users and we didn't
> > have vendor specific hooks introduced by Steve later on.
>
> It's very confusing to see a test running and zero QPs in "rdma res".
> I'm fine with removing the "wrongly" :), but I still think this should be
> targeted to for-rc as a bug fix.

Yes, please remove "wrongly" and change Fixes line to be
"Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
because before addition of EFA driver all other drivers had QPs.

>
> >
> > I would like to see the output or "rdma res" and "rdma res show qp" with
> > running driver QP after your change.
>
> gal@server [~] $ rdma res
> 0: efa_0: pd 2 cq 4 qp 2 cm_id 0 mr 2 ctx 2
> gal@server [~] $ rdma res show qp
> link efa_0/1 lqpn 0 type UNKNOWN state RTS sq-psn 13400680 pdn 0 pid 17847 comm ib_send_bw
> link efa_0/1 lqpn 1 type UNKNOWN state RTR sq-psn 0 pdn 1 pid 17820 comm ib_send_bw
>
> We can change the UNKNOWN to DRIVER/something else in userspace code.

We need to change it too.

Thanks

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

On 30/07/2019 18:19, Leon Romanovsky wrote:
> On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
>> On 30/07/2019 16:38, Leon Romanovsky wrote:
>>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
>>>> The check for QP type different than XRC has wrongly excluded driver QP
>>>> types from the resource tracker.
>>>>
>>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
>>>
>>> It is a little bit over to say "wrongly". At that time, we did it on purpose
>>> because it was unclear how to represent such QP types to users and we didn't
>>> have vendor specific hooks introduced by Steve later on.
>>
>> It's very confusing to see a test running and zero QPs in "rdma res".
>> I'm fine with removing the "wrongly" :), but I still think this should be
>> targeted to for-rc as a bug fix.
> 
> Yes, please remove "wrongly" and change Fixes line to be
> "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
> because before addition of EFA driver all other drivers had QPs.

How are DC QPs being counted?

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Leon Romanovsky]

On Wed, Jul 31, 2019 at 10:05:31AM +0300, Gal Pressman wrote:
> On 30/07/2019 18:19, Leon Romanovsky wrote:
> > On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
> >> On 30/07/2019 16:38, Leon Romanovsky wrote:
> >>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> >>>> The check for QP type different than XRC has wrongly excluded driver QP
> >>>> types from the resource tracker.
> >>>>
> >>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
> >>>
> >>> It is a little bit over to say "wrongly". At that time, we did it on purpose
> >>> because it was unclear how to represent such QP types to users and we didn't
> >>> have vendor specific hooks introduced by Steve later on.
> >>
> >> It's very confusing to see a test running and zero QPs in "rdma res".
> >> I'm fine with removing the "wrongly" :), but I still think this should be
> >> targeted to for-rc as a bug fix.
> >
> > Yes, please remove "wrongly" and change Fixes line to be
> > "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
> > because before addition of EFA driver all other drivers had QPs.
>
> How are DC QPs being counted?

They were not counted on purpose. We didn't imagine acceptance of
non-RDMA driver which doesn't support any standard QPs and doesn't
work with kernel verbs.

Thanks

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

On 31/07/2019 10:46, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 10:05:31AM +0300, Gal Pressman wrote:
>> On 30/07/2019 18:19, Leon Romanovsky wrote:
>>> On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
>>>> On 30/07/2019 16:38, Leon Romanovsky wrote:
>>>>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
>>>>>> The check for QP type different than XRC has wrongly excluded driver QP
>>>>>> types from the resource tracker.
>>>>>>
>>>>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
>>>>>
>>>>> It is a little bit over to say "wrongly". At that time, we did it on purpose
>>>>> because it was unclear how to represent such QP types to users and we didn't
>>>>> have vendor specific hooks introduced by Steve later on.
>>>>
>>>> It's very confusing to see a test running and zero QPs in "rdma res".
>>>> I'm fine with removing the "wrongly" :), but I still think this should be
>>>> targeted to for-rc as a bug fix.
>>>
>>> Yes, please remove "wrongly" and change Fixes line to be
>>> "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
>>> because before addition of EFA driver all other drivers had QPs.
>>
>> How are DC QPs being counted?
> 
> They were not counted on purpose. We didn't imagine acceptance of
> non-RDMA driver which doesn't support any standard QPs and doesn't
> work with kernel verbs.

Running dcping/perftest over DC shows zero QPs? On purpose?
Sounds like a bug to me..

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Leon Romanovsky]

On Wed, Jul 31, 2019 at 10:53:10AM +0300, Gal Pressman wrote:
> On 31/07/2019 10:46, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 10:05:31AM +0300, Gal Pressman wrote:
> >> On 30/07/2019 18:19, Leon Romanovsky wrote:
> >>> On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
> >>>> On 30/07/2019 16:38, Leon Romanovsky wrote:
> >>>>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> >>>>>> The check for QP type different than XRC has wrongly excluded driver QP
> >>>>>> types from the resource tracker.
> >>>>>>
> >>>>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
> >>>>>
> >>>>> It is a little bit over to say "wrongly". At that time, we did it on purpose
> >>>>> because it was unclear how to represent such QP types to users and we didn't
> >>>>> have vendor specific hooks introduced by Steve later on.
> >>>>
> >>>> It's very confusing to see a test running and zero QPs in "rdma res".
> >>>> I'm fine with removing the "wrongly" :), but I still think this should be
> >>>> targeted to for-rc as a bug fix.
> >>>
> >>> Yes, please remove "wrongly" and change Fixes line to be
> >>> "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
> >>> because before addition of EFA driver all other drivers had QPs.
> >>
> >> How are DC QPs being counted?
> >
> > They were not counted on purpose. We didn't imagine acceptance of
> > non-RDMA driver which doesn't support any standard QPs and doesn't
> > work with kernel verbs.
>
> Running dcping/perftest over DC shows zero QPs?

No, try it and you will see other QPs.

> On purpose?
> Sounds like a bug to me..

OK.

Thanks

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Gal Pressman]

On 31/07/2019 11:34, Leon Romanovsky wrote:
> On Wed, Jul 31, 2019 at 10:53:10AM +0300, Gal Pressman wrote:
>> On 31/07/2019 10:46, Leon Romanovsky wrote:
>>> On Wed, Jul 31, 2019 at 10:05:31AM +0300, Gal Pressman wrote:
>>>> On 30/07/2019 18:19, Leon Romanovsky wrote:
>>>>> On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
>>>>>> On 30/07/2019 16:38, Leon Romanovsky wrote:
>>>>>>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
>>>>>>>> The check for QP type different than XRC has wrongly excluded driver QP
>>>>>>>> types from the resource tracker.
>>>>>>>>
>>>>>>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
>>>>>>>
>>>>>>> It is a little bit over to say "wrongly". At that time, we did it on purpose
>>>>>>> because it was unclear how to represent such QP types to users and we didn't
>>>>>>> have vendor specific hooks introduced by Steve later on.
>>>>>>
>>>>>> It's very confusing to see a test running and zero QPs in "rdma res".
>>>>>> I'm fine with removing the "wrongly" :), but I still think this should be
>>>>>> targeted to for-rc as a bug fix.
>>>>>
>>>>> Yes, please remove "wrongly" and change Fixes line to be
>>>>> "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
>>>>> because before addition of EFA driver all other drivers had QPs.
>>>>
>>>> How are DC QPs being counted?
>>>
>>> They were not counted on purpose. We didn't imagine acceptance of
>>> non-RDMA driver which doesn't support any standard QPs and doesn't
>>> work with kernel verbs.
>>
>> Running dcping/perftest over DC shows zero QPs?
> 
> No, try it and you will see other QPs.
> 
>> On purpose?
>> Sounds like a bug to me..
> 
> OK.

Does OK mean you're OK with counting DC QPs after this patch?

Re: [PATCH for-rc] RDMA/restrack: Track driver QP types in resource tracker

[Leon Romanovsky]

On Wed, Jul 31, 2019 at 11:51:58AM +0300, Gal Pressman wrote:
> On 31/07/2019 11:34, Leon Romanovsky wrote:
> > On Wed, Jul 31, 2019 at 10:53:10AM +0300, Gal Pressman wrote:
> >> On 31/07/2019 10:46, Leon Romanovsky wrote:
> >>> On Wed, Jul 31, 2019 at 10:05:31AM +0300, Gal Pressman wrote:
> >>>> On 30/07/2019 18:19, Leon Romanovsky wrote:
> >>>>> On Tue, Jul 30, 2019 at 04:49:52PM +0300, Gal Pressman wrote:
> >>>>>> On 30/07/2019 16:38, Leon Romanovsky wrote:
> >>>>>>> On Tue, Jul 30, 2019 at 02:01:37PM +0300, Gal Pressman wrote:
> >>>>>>>> The check for QP type different than XRC has wrongly excluded driver QP
> >>>>>>>> types from the resource tracker.
> >>>>>>>>
> >>>>>>>> Fixes: 78a0cd648a80 ("RDMA/core: Add resource tracking for create and destroy QPs")
> >>>>>>>
> >>>>>>> It is a little bit over to say "wrongly". At that time, we did it on purpose
> >>>>>>> because it was unclear how to represent such QP types to users and we didn't
> >>>>>>> have vendor specific hooks introduced by Steve later on.
> >>>>>>
> >>>>>> It's very confusing to see a test running and zero QPs in "rdma res".
> >>>>>> I'm fine with removing the "wrongly" :), but I still think this should be
> >>>>>> targeted to for-rc as a bug fix.
> >>>>>
> >>>>> Yes, please remove "wrongly" and change Fixes line to be
> >>>>> "Fixes: 40909f664d27 ("RDMA/efa: Add EFA verbs implementation")",
> >>>>> because before addition of EFA driver all other drivers had QPs.
> >>>>
> >>>> How are DC QPs being counted?
> >>>
> >>> They were not counted on purpose. We didn't imagine acceptance of
> >>> non-RDMA driver which doesn't support any standard QPs and doesn't
> >>> work with kernel verbs.
> >>
> >> Running dcping/perftest over DC shows zero QPs?
> >
> > No, try it and you will see other QPs.
> >
> >> On purpose?
> >> Sounds like a bug to me..
> >
> > OK.
>
> Does OK mean you're OK with counting DC QPs after this patch?

I'm OK with the idea, I'm not OK with the description.

Thanks