| Message ID | 20190730110137.37826-1-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-rc] RDMA/restrack: Track driver QP types in resource tracker | expand |
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
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.
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
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.
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
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?
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
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..
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
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?
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
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);
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(-)