Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp
On Mon, Oct 16, 2023 at 03:12:02PM -0700, sharmaa...@linuxonhyperv.com wrote: > diff --git a/drivers/infiniband/hw/mana/qp.c b/drivers/infiniband/hw/mana/qp.c > index ef3275ac92a0..19fae28985c3 100644 > --- a/drivers/infiniband/hw/mana/qp.c > +++ b/drivers/infiniband/hw/mana/qp.c > @@ -210,6 +210,8 @@ static int mana_ib_create_qp_rss(struct ib_qp *ibqp, > struct ib_pd *pd, > wq->id = wq_spec.queue_index; > cq->id = cq_spec.queue_index; > > + xa_store(&mib_dev->rq_to_qp_lookup_table, wq->id, qp, > GFP_KERNEL); > + A store with no erase? A load with no locking? This can't be right Jason
RE: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp
> -Original Message- > From: Jason Gunthorpe > Sent: Monday, October 23, 2023 11:24 AM > To: sharmaa...@linuxonhyperv.com > Cc: Long Li ; Leon Romanovsky ; > Dexuan Cui ; Wei Liu ; David S. > Miller ; Eric Dumazet ; > Jakub Kicinski ; Paolo Abeni ; linux- > r...@vger.kernel.org; linux-hyperv@vger.kernel.org; > net...@vger.kernel.org; linux-ker...@vger.kernel.org; Ajay Sharma > > Subject: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > On Mon, Oct 16, 2023 at 03:12:02PM -0700, > sharmaa...@linuxonhyperv.com wrote: > > > diff --git a/drivers/infiniband/hw/mana/qp.c > > b/drivers/infiniband/hw/mana/qp.c index ef3275ac92a0..19fae28985c3 > > 100644 > > --- a/drivers/infiniband/hw/mana/qp.c > > +++ b/drivers/infiniband/hw/mana/qp.c > > @@ -210,6 +210,8 @@ static int mana_ib_create_qp_rss(struct ib_qp > *ibqp, struct ib_pd *pd, > > wq->id = wq_spec.queue_index; > > cq->id = cq_spec.queue_index; > > > > + xa_store(&mib_dev->rq_to_qp_lookup_table, wq->id, qp, > GFP_KERNEL); > > + > > A store with no erase? > > A load with no locking? > > This can't be right > > Jason This wq->id is assigned from the HW and is guaranteed to be unique. May be I am not following why do we need a lock here. Can you please explain ? Ajay
RE: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp
> Subject: RE: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > > > -Original Message- > > From: Jason Gunthorpe > > Sent: Monday, October 23, 2023 11:24 AM > > To: sharmaa...@linuxonhyperv.com > > Cc: Long Li ; Leon Romanovsky ; > > Dexuan Cui ; Wei Liu ; David S. > > Miller ; Eric Dumazet ; > > Jakub Kicinski ; Paolo Abeni ; > > linux- r...@vger.kernel.org; linux-hyperv@vger.kernel.org; > > net...@vger.kernel.org; linux-ker...@vger.kernel.org; Ajay Sharma > > > > Subject: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > > > On Mon, Oct 16, 2023 at 03:12:02PM -0700, > sharmaa...@linuxonhyperv.com > > wrote: > > > > > diff --git a/drivers/infiniband/hw/mana/qp.c > > > b/drivers/infiniband/hw/mana/qp.c index ef3275ac92a0..19fae28985c3 > > > 100644 > > > --- a/drivers/infiniband/hw/mana/qp.c > > > +++ b/drivers/infiniband/hw/mana/qp.c > > > @@ -210,6 +210,8 @@ static int mana_ib_create_qp_rss(struct ib_qp > > *ibqp, struct ib_pd *pd, > > > wq->id = wq_spec.queue_index; > > > cq->id = cq_spec.queue_index; > > > > > > + xa_store(&mib_dev->rq_to_qp_lookup_table, wq->id, qp, > > GFP_KERNEL); > > > + > > > > A store with no erase? > > > > A load with no locking? > > > > This can't be right > > > > Jason > > This wq->id is assigned from the HW and is guaranteed to be unique. May be I > am not following why do we need a lock here. Can you please explain ? > Ajay I think we need to check the return value of xa_store(), and call xa_erase() in mana_ib_destroy_qp(). wq->id is generated by the hardware. If we believe in hardware always behaves in good manner, we don't need a lock. Thanks, Long
Re: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp
On Fri, Oct 27, 2023 at 09:35:05PM +, Long Li wrote: > > Subject: RE: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > > > > > > -Original Message- > > > From: Jason Gunthorpe > > > Sent: Monday, October 23, 2023 11:24 AM > > > To: sharmaa...@linuxonhyperv.com > > > Cc: Long Li ; Leon Romanovsky ; > > > Dexuan Cui ; Wei Liu ; David S. > > > Miller ; Eric Dumazet ; > > > Jakub Kicinski ; Paolo Abeni ; > > > linux- r...@vger.kernel.org; linux-hyperv@vger.kernel.org; > > > net...@vger.kernel.org; linux-ker...@vger.kernel.org; Ajay Sharma > > > > > > Subject: [EXTERNAL] Re: [Patch v7 5/5] RDMA/mana_ib: Send event to qp > > > > > > On Mon, Oct 16, 2023 at 03:12:02PM -0700, > > sharmaa...@linuxonhyperv.com > > > wrote: > > > > > > > diff --git a/drivers/infiniband/hw/mana/qp.c > > > > b/drivers/infiniband/hw/mana/qp.c index ef3275ac92a0..19fae28985c3 > > > > 100644 > > > > --- a/drivers/infiniband/hw/mana/qp.c > > > > +++ b/drivers/infiniband/hw/mana/qp.c > > > > @@ -210,6 +210,8 @@ static int mana_ib_create_qp_rss(struct ib_qp > > > *ibqp, struct ib_pd *pd, > > > > wq->id = wq_spec.queue_index; > > > > cq->id = cq_spec.queue_index; > > > > > > > > + xa_store(&mib_dev->rq_to_qp_lookup_table, wq->id, qp, > > > GFP_KERNEL); > > > > + > > > > > > A store with no erase? > > > > > > A load with no locking? > > > > > > This can't be right > > > > > > Jason > > > > This wq->id is assigned from the HW and is guaranteed to be unique. May be I > > am not following why do we need a lock here. Can you please explain ? > > Ajay > > I think we need to check the return value of xa_store(), and call xa_erase() > in mana_ib_destroy_qp(). > > wq->id is generated by the hardware. If we believe in hardware > always behaves in good manner, we don't need a lock. It has nothing to do with how the ID is created, you need to explain how the missing erase can't race with any loads, in a comment above the unlocked xa_load. Jason
