| Message ID | 20220127213755.31697-5-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [RFC,v9,01/26] RDMA/rxe: Move rxe_mcast_add/delete to rxe_mcast.c | expand |
On Thu, Jan 27, 2022 at 03:37:33PM -0600, Bob Pearson wrote: > Add code to check if a QP is attached to one or more multicast groups > when destroy_qp is called and return an error if so. The core code already does some of this anyhow.. > diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c > index 949784198d80..34e3c52f0b72 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c > @@ -114,6 +114,7 @@ static int rxe_mcast_add_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp, > grp->num_qp++; > elem->qp = qp; > elem->grp = grp; > + atomic_inc(&qp->mcg_num); eg what prevents qp from being concurrently destroyed here? The core code because it doesn't allow a multicast group to be added concurrently with destruction of a QP. > +int rxe_qp_chk_destroy(struct rxe_qp *qp) > +{ > + /* See IBA o10-2.2.3 > + * An attempt to destroy a QP while attached to a mcast group > + * will fail immediately. > + */ > + if (atomic_read(&qp->mcg_num)) { > + pr_warn_once("Attempt to destroy QP while attached to multicast group\n"); > + return -EBUSY; Don't print But yes, I think drivers are expected to do this, though most likely this is already happening for other reasons and this is mearly protective against bugs. Jason
On 1/28/22 06:53, Jason Gunthorpe wrote: > On Thu, Jan 27, 2022 at 03:37:33PM -0600, Bob Pearson wrote: >> Add code to check if a QP is attached to one or more multicast groups >> when destroy_qp is called and return an error if so. > > The core code already does some of this anyhow.. > >> diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c >> index 949784198d80..34e3c52f0b72 100644 >> +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c >> @@ -114,6 +114,7 @@ static int rxe_mcast_add_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp, >> grp->num_qp++; >> elem->qp = qp; >> elem->grp = grp; >> + atomic_inc(&qp->mcg_num); > > eg what prevents qp from being concurrently destroyed here? > > The core code because it doesn't allow a multicast group to be added > concurrently with destruction of a QP. > >> +int rxe_qp_chk_destroy(struct rxe_qp *qp) >> +{ >> + /* See IBA o10-2.2.3 >> + * An attempt to destroy a QP while attached to a mcast group >> + * will fail immediately. >> + */ >> + if (atomic_read(&qp->mcg_num)) { >> + pr_warn_once("Attempt to destroy QP while attached to multicast group\n"); >> + return -EBUSY; > > Don't print > > But yes, I think drivers are expected to do this, though most likely > this is already happening for other reasons and this is mearly > protective against bugs. > > Jason The real reason for this patch becomes apparent in the next one or two. With this no longer an issue half the complexity of rxe_mcast goes away. I'll get rid of the print. Personally I find them helpful when debugging user code. Maybe a pr_debug? Bob
On Fri, Jan 28, 2022 at 10:18:45AM -0600, Bob Pearson wrote: > On 1/28/22 06:53, Jason Gunthorpe wrote: > > On Thu, Jan 27, 2022 at 03:37:33PM -0600, Bob Pearson wrote: > >> Add code to check if a QP is attached to one or more multicast groups > >> when destroy_qp is called and return an error if so. > > > > The core code already does some of this anyhow.. > > > >> diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c > >> index 949784198d80..34e3c52f0b72 100644 > >> +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c > >> @@ -114,6 +114,7 @@ static int rxe_mcast_add_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp, > >> grp->num_qp++; > >> elem->qp = qp; > >> elem->grp = grp; > >> + atomic_inc(&qp->mcg_num); > > > > eg what prevents qp from being concurrently destroyed here? > > > > The core code because it doesn't allow a multicast group to be added > > concurrently with destruction of a QP. > > > >> +int rxe_qp_chk_destroy(struct rxe_qp *qp) > >> +{ > >> + /* See IBA o10-2.2.3 > >> + * An attempt to destroy a QP while attached to a mcast group > >> + * will fail immediately. > >> + */ > >> + if (atomic_read(&qp->mcg_num)) { > >> + pr_warn_once("Attempt to destroy QP while attached to multicast group\n"); > >> + return -EBUSY; > > > > Don't print > > > > But yes, I think drivers are expected to do this, though most likely > > this is already happening for other reasons and this is mearly > > protective against bugs. > > > > Jason > > The real reason for this patch becomes apparent in the next one or two. With this no longer an issue half the complexity of rxe_mcast goes away. I'll get rid of the print. > Personally I find them helpful when debugging user code. Maybe a > pr_debug? Sure Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_loc.h b/drivers/infiniband/sw/rxe/rxe_loc.h index dc606241f0d6..052beaaacf43 100644 --- a/drivers/infiniband/sw/rxe/rxe_loc.h +++ b/drivers/infiniband/sw/rxe/rxe_loc.h @@ -101,26 +101,19 @@ const char *rxe_parent_name(struct rxe_dev *rxe, unsigned int port_num); /* rxe_qp.c */ int rxe_qp_chk_init(struct rxe_dev *rxe, struct ib_qp_init_attr *init); - int rxe_qp_from_init(struct rxe_dev *rxe, struct rxe_qp *qp, struct rxe_pd *pd, struct ib_qp_init_attr *init, struct rxe_create_qp_resp __user *uresp, struct ib_pd *ibpd, struct ib_udata *udata); - int rxe_qp_to_init(struct rxe_qp *qp, struct ib_qp_init_attr *init); - int rxe_qp_chk_attr(struct rxe_dev *rxe, struct rxe_qp *qp, struct ib_qp_attr *attr, int mask); - int rxe_qp_from_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask, struct ib_udata *udata); - int rxe_qp_to_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask); - void rxe_qp_error(struct rxe_qp *qp); - +int rxe_qp_chk_destroy(struct rxe_qp *qp); void rxe_qp_destroy(struct rxe_qp *qp); - void rxe_qp_cleanup(struct rxe_pool_elem *elem); static inline int qp_num(struct rxe_qp *qp) diff --git a/drivers/infiniband/sw/rxe/rxe_mcast.c b/drivers/infiniband/sw/rxe/rxe_mcast.c index 949784198d80..34e3c52f0b72 100644 --- a/drivers/infiniband/sw/rxe/rxe_mcast.c +++ b/drivers/infiniband/sw/rxe/rxe_mcast.c @@ -114,6 +114,7 @@ static int rxe_mcast_add_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp, grp->num_qp++; elem->qp = qp; elem->grp = grp; + atomic_inc(&qp->mcg_num); list_add(&elem->qp_list, &grp->qp_list); list_add(&elem->grp_list, &qp->grp_list); @@ -143,6 +144,7 @@ static int rxe_mcast_drop_grp_elem(struct rxe_dev *rxe, struct rxe_qp *qp, list_del(&elem->qp_list); list_del(&elem->grp_list); grp->num_qp--; + atomic_dec(&qp->mcg_num); spin_unlock_bh(&grp->mcg_lock); spin_unlock_bh(&qp->grp_lock); diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c index 5018b9387694..2af19b79dd23 100644 --- a/drivers/infiniband/sw/rxe/rxe_qp.c +++ b/drivers/infiniband/sw/rxe/rxe_qp.c @@ -770,6 +770,20 @@ int rxe_qp_to_attr(struct rxe_qp *qp, struct ib_qp_attr *attr, int mask) return 0; } +int rxe_qp_chk_destroy(struct rxe_qp *qp) +{ + /* See IBA o10-2.2.3 + * An attempt to destroy a QP while attached to a mcast group + * will fail immediately. + */ + if (atomic_read(&qp->mcg_num)) { + pr_warn_once("Attempt to destroy QP while attached to multicast group\n"); + return -EBUSY; + } + + return 0; +} + /* called by the destroy qp verb */ void rxe_qp_destroy(struct rxe_qp *qp) { diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.c b/drivers/infiniband/sw/rxe/rxe_verbs.c index f7682541f9af..9f0aef4b649d 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.c +++ b/drivers/infiniband/sw/rxe/rxe_verbs.c @@ -493,6 +493,11 @@ static int rxe_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *attr, static int rxe_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata) { struct rxe_qp *qp = to_rqp(ibqp); + int ret; + + ret = rxe_qp_chk_destroy(qp); + if (ret) + return ret; rxe_qp_destroy(qp); rxe_drop_index(qp); diff --git a/drivers/infiniband/sw/rxe/rxe_verbs.h b/drivers/infiniband/sw/rxe/rxe_verbs.h index 388b7dc23dd7..4910d0782e33 100644 --- a/drivers/infiniband/sw/rxe/rxe_verbs.h +++ b/drivers/infiniband/sw/rxe/rxe_verbs.h @@ -235,6 +235,7 @@ struct rxe_qp { /* list of mcast groups qp has joined (for cleanup) */ struct list_head grp_list; spinlock_t grp_lock; /* guard grp_list */ + atomic_t mcg_num; struct sk_buff_head req_pkts; struct sk_buff_head resp_pkts;
Add code to check if a QP is attached to one or more multicast groups when destroy_qp is called and return an error if so. Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- drivers/infiniband/sw/rxe/rxe_loc.h | 9 +-------- drivers/infiniband/sw/rxe/rxe_mcast.c | 2 ++ drivers/infiniband/sw/rxe/rxe_qp.c | 14 ++++++++++++++ drivers/infiniband/sw/rxe/rxe_verbs.c | 5 +++++ drivers/infiniband/sw/rxe/rxe_verbs.h | 1 + 5 files changed, 23 insertions(+), 8 deletions(-)