* [PATCH for-next 0/2] Fix RMW to bit-fields and remove one superfluous ib_modify_qp @ 2021-06-22 13:20 Håkon Bugge 2021-06-22 13:20 ` [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition Håkon Bugge 2021-06-22 13:20 ` [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex Håkon Bugge 0 siblings, 2 replies; 8+ messages in thread From: Håkon Bugge @ 2021-06-22 13:20 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky; +Cc: linux-rdma, Mark Zhang This series removes one superfluous ib_modify_qp() when a connected QP is created through rdma_create_qp(). This commit removes a call to rdma_init_qp_attr() for connected QPs without holding the qp_mutex. The second commit tightens the access to bit-fields and synchronizes the access to them by means of mutex exclusion. Håkon Bugge (2): RDMA/cma: Remove unnecessary INIT->INIT transition RDMA/cma: Protect RMW with qp_mutex drivers/infiniband/core/cma.c | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition 2021-06-22 13:20 [PATCH for-next 0/2] Fix RMW to bit-fields and remove one superfluous ib_modify_qp Håkon Bugge @ 2021-06-22 13:20 ` Håkon Bugge 2021-06-22 14:47 ` Jason Gunthorpe 2021-06-22 13:20 ` [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex Håkon Bugge 1 sibling, 1 reply; 8+ messages in thread From: Håkon Bugge @ 2021-06-22 13:20 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky; +Cc: linux-rdma, Mark Zhang In rdma_create_qp(), a connected QP will be transitioned to the INIT state. Afterwards, the QP will be transitioned to the RTR state by the cma_modify_qp_rtr() function. But this function starts by performing an ib_modify_qp() to the INIT state again, before another ib_modify_qp() is performed to transition the QP to the RTR state. Hence, there is no need to transition the QP to the INIT state in rdma_create_qp(). Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> Reviewed-by: Leon Romanovsky <leonro@nvidia.com> --- v1 -> v2: * Fixed uninitialized ret variable as: Reported-by: kernel test robot <lkp@intel.com> --- drivers/infiniband/core/cma.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index ab148a6..e3f52c5 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -926,25 +926,12 @@ static int cma_init_ud_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) return ret; } -static int cma_init_conn_qp(struct rdma_id_private *id_priv, struct ib_qp *qp) -{ - struct ib_qp_attr qp_attr; - int qp_attr_mask, ret; - - qp_attr.qp_state = IB_QPS_INIT; - ret = rdma_init_qp_attr(&id_priv->id, &qp_attr, &qp_attr_mask); - if (ret) - return ret; - - return ib_modify_qp(qp, &qp_attr, qp_attr_mask); -} - int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, struct ib_qp_init_attr *qp_init_attr) { struct rdma_id_private *id_priv; struct ib_qp *qp; - int ret; + int ret = 0; id_priv = container_of(id, struct rdma_id_private, id); if (id->device != pd->device) { @@ -961,8 +948,6 @@ int rdma_create_qp(struct rdma_cm_id *id, struct ib_pd *pd, if (id->qp_type == IB_QPT_UD) ret = cma_init_ud_qp(id_priv, qp); - else - ret = cma_init_conn_qp(id_priv, qp); if (ret) goto out_destroy; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition 2021-06-22 13:20 ` [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition Håkon Bugge @ 2021-06-22 14:47 ` Jason Gunthorpe 2021-06-22 14:53 ` Haakon Bugge 0 siblings, 1 reply; 8+ messages in thread From: Jason Gunthorpe @ 2021-06-22 14:47 UTC (permalink / raw) To: Håkon Bugge; +Cc: Doug Ledford, Leon Romanovsky, linux-rdma, Mark Zhang On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote: > In rdma_create_qp(), a connected QP will be transitioned to the INIT > state. > > Afterwards, the QP will be transitioned to the RTR state by the > cma_modify_qp_rtr() function. But this function starts by performing > an ib_modify_qp() to the INIT state again, before another > ib_modify_qp() is performed to transition the QP to the RTR state. This makes me really nervous that something depends on this since the API is split up?? Jason ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition 2021-06-22 14:47 ` Jason Gunthorpe @ 2021-06-22 14:53 ` Haakon Bugge 2021-06-22 14:59 ` Jason Gunthorpe 0 siblings, 1 reply; 8+ messages in thread From: Haakon Bugge @ 2021-06-22 14:53 UTC (permalink / raw) To: Jason Gunthorpe Cc: Doug Ledford, Leon Romanovsky, OFED mailing list, Mark Zhang > On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote: >> In rdma_create_qp(), a connected QP will be transitioned to the INIT >> state. >> >> Afterwards, the QP will be transitioned to the RTR state by the >> cma_modify_qp_rtr() function. But this function starts by performing >> an ib_modify_qp() to the INIT state again, before another >> ib_modify_qp() is performed to transition the QP to the RTR state. > > This makes me really nervous that something depends on this since the > API is split up?? As I commented to Mark, no ULP creates a connected QP with rdma_create_qp() and thereafter modifies it with an INIT -> INIT transition. And if it did, the values modified would be overwritten by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is called. Thxs, Håkon ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition 2021-06-22 14:53 ` Haakon Bugge @ 2021-06-22 14:59 ` Jason Gunthorpe 2021-06-22 15:44 ` Haakon Bugge 0 siblings, 1 reply; 8+ messages in thread From: Jason Gunthorpe @ 2021-06-22 14:59 UTC (permalink / raw) To: Haakon Bugge; +Cc: Doug Ledford, Leon Romanovsky, OFED mailing list, Mark Zhang On Tue, Jun 22, 2021 at 02:53:33PM +0000, Haakon Bugge wrote: > > > > On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote: > >> In rdma_create_qp(), a connected QP will be transitioned to the INIT > >> state. > >> > >> Afterwards, the QP will be transitioned to the RTR state by the > >> cma_modify_qp_rtr() function. But this function starts by performing > >> an ib_modify_qp() to the INIT state again, before another > >> ib_modify_qp() is performed to transition the QP to the RTR state. > > > > This makes me really nervous that something depends on this since the > > API is split up?? > > As I commented to Mark, no ULP creates a connected QP with > rdma_create_qp() and thereafter modifies it with an INIT -> INIT > transition. And if it did, the values modified would be overwritten > by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is > called. Does anything call query_qp? Jason ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition 2021-06-22 14:59 ` Jason Gunthorpe @ 2021-06-22 15:44 ` Haakon Bugge 0 siblings, 0 replies; 8+ messages in thread From: Haakon Bugge @ 2021-06-22 15:44 UTC (permalink / raw) To: Jason Gunthorpe Cc: Doug Ledford, Leon Romanovsky, OFED mailing list, Mark Zhang > On 22 Jun 2021, at 16:59, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Tue, Jun 22, 2021 at 02:53:33PM +0000, Haakon Bugge wrote: >> >> >>> On 22 Jun 2021, at 16:47, Jason Gunthorpe <jgg@nvidia.com> wrote: >>> >>> On Tue, Jun 22, 2021 at 03:20:29PM +0200, Håkon Bugge wrote: >>>> In rdma_create_qp(), a connected QP will be transitioned to the INIT >>>> state. >>>> >>>> Afterwards, the QP will be transitioned to the RTR state by the >>>> cma_modify_qp_rtr() function. But this function starts by performing >>>> an ib_modify_qp() to the INIT state again, before another >>>> ib_modify_qp() is performed to transition the QP to the RTR state. >>> >>> This makes me really nervous that something depends on this since the >>> API is split up?? >> >> As I commented to Mark, no ULP creates a connected QP with >> rdma_create_qp() and thereafter modifies it with an INIT -> INIT >> transition. And if it did, the values modified would be overwritten >> by the (now) RESET -> INIT transition when cma_modify_qp_rtr() is >> called. > > Does anything call query_qp? iser_connected_handler() does, but that's when an RDMA_CM_EVENT_ESTABLISHED has been received, so the QP state is already RTS. Håkon ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex 2021-06-22 13:20 [PATCH for-next 0/2] Fix RMW to bit-fields and remove one superfluous ib_modify_qp Håkon Bugge 2021-06-22 13:20 ` [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition Håkon Bugge @ 2021-06-22 13:20 ` Håkon Bugge 2021-06-22 13:30 ` Haakon Bugge 1 sibling, 1 reply; 8+ messages in thread From: Håkon Bugge @ 2021-06-22 13:20 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky; +Cc: linux-rdma, Mark Zhang The struct rdma_id_private contains three bit-fields, tos_set, timeout_set, and min_rnr_timer_set. These are set by accessor functions without any synchronization. If two or all accessor functions are invoked in close proximity in time, there will be Read-Modify-Write from several contexts to the same variable, and the result will be intermittent. Fixed by protecting the bit-fields by the qp_mutex in the accessor functions. The consumer of timeout_set and min_rnr_timer_set is in rdma_init_qp_attr(), which is called with qp_mutex held for connected QPs. Explicit locking is added for the consumers of tos and tos_set. This commit depends on ("RDMA/cma: Remove unnecessary INIT->INIT transition"), since the call to rdma_init_qp_attr() from cma_init_conn_qp() does not hold the qp_mutex. Fixes: 2c1619edef61 ("IB/cma: Define option to set ack timeout and pack tos_set") Fixes: 3aeffc46afde ("IB/cma: Introduce rdma_set_min_rnr_timer()") Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> --- drivers/infiniband/core/cma.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c index e3f52c5..6b41527 100644 --- a/drivers/infiniband/core/cma.c +++ b/drivers/infiniband/core/cma.c @@ -2457,8 +2457,10 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog) if (IS_ERR(id)) return PTR_ERR(id); + mutex_lock(&id_priv->qp_mutex); id->tos = id_priv->tos; id->tos_set = id_priv->tos_set; + mutex_unlock(&id_priv->qp_mutex); id->afonly = id_priv->afonly; id_priv->cm_id.iw = id; @@ -2519,8 +2521,10 @@ static int cma_listen_on_dev(struct rdma_id_private *id_priv, cma_id_get(id_priv); dev_id_priv->internal_id = 1; dev_id_priv->afonly = id_priv->afonly; + mutex_lock(&id_priv->qp_mutex); dev_id_priv->tos_set = id_priv->tos_set; dev_id_priv->tos = id_priv->tos; + mutex_unlock(&id_priv->qp_mutex); ret = rdma_listen(&dev_id_priv->id, id_priv->backlog); if (ret) @@ -2567,8 +2571,10 @@ void rdma_set_service_type(struct rdma_cm_id *id, int tos) struct rdma_id_private *id_priv; id_priv = container_of(id, struct rdma_id_private, id); + mutex_lock(&id_priv->qp_mutex); id_priv->tos = (u8) tos; id_priv->tos_set = true; + mutex_unlock(&id_priv->qp_mutex); } EXPORT_SYMBOL(rdma_set_service_type); @@ -2595,8 +2601,10 @@ int rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout) return -EINVAL; id_priv = container_of(id, struct rdma_id_private, id); + mutex_lock(&id_priv->qp_mutex); id_priv->timeout = timeout; id_priv->timeout_set = true; + mutex_unlock(&id_priv->qp_mutex); return 0; } @@ -2632,8 +2640,10 @@ int rdma_set_min_rnr_timer(struct rdma_cm_id *id, u8 min_rnr_timer) return -EINVAL; id_priv = container_of(id, struct rdma_id_private, id); + mutex_lock(&id_priv->qp_mutex); id_priv->min_rnr_timer = min_rnr_timer; id_priv->min_rnr_timer_set = true; + mutex_unlock(&id_priv->qp_mutex); return 0; } @@ -3019,8 +3029,11 @@ static int cma_resolve_iboe_route(struct rdma_id_private *id_priv) u8 default_roce_tos = id_priv->cma_dev->default_roce_tos[id_priv->id.port_num - rdma_start_port(id_priv->cma_dev->device)]; - u8 tos = id_priv->tos_set ? id_priv->tos : default_roce_tos; + u8 tos; + mutex_lock(&id_priv->qp_mutex); + tos = id_priv->tos_set ? id_priv->tos : default_roce_tos; + mutex_unlock(&id_priv->qp_mutex); work = kzalloc(sizeof *work, GFP_KERNEL); if (!work) @@ -4092,8 +4105,11 @@ static int cma_connect_iw(struct rdma_id_private *id_priv, if (IS_ERR(cm_id)) return PTR_ERR(cm_id); + mutex_lock(&id_priv->qp_mutex); cm_id->tos = id_priv->tos; cm_id->tos_set = id_priv->tos_set; + mutex_unlock(&id_priv->qp_mutex); + id_priv->cm_id.iw = cm_id; memcpy(&cm_id->local_addr, cma_src_addr(id_priv), -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex 2021-06-22 13:20 ` [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex Håkon Bugge @ 2021-06-22 13:30 ` Haakon Bugge 0 siblings, 0 replies; 8+ messages in thread From: Haakon Bugge @ 2021-06-22 13:30 UTC (permalink / raw) To: Doug Ledford, Jason Gunthorpe, Leon Romanovsky Cc: OFED mailing list, Mark Zhang > On 22 Jun 2021, at 15:20, Håkon Bugge <haakon.bugge@oracle.com> wrote: > > The struct rdma_id_private contains three bit-fields, tos_set, > timeout_set, and min_rnr_timer_set. These are set by accessor > functions without any synchronization. If two or all accessor > functions are invoked in close proximity in time, there will be > Read-Modify-Write from several contexts to the same variable, and the > result will be intermittent. > > Fixed by protecting the bit-fields by the qp_mutex in the accessor > functions. > > The consumer of timeout_set and min_rnr_timer_set is in > rdma_init_qp_attr(), which is called with qp_mutex held for connected > QPs. Explicit locking is added for the consumers of tos and tos_set. > > This commit depends on ("RDMA/cma: Remove unnecessary INIT->INIT > transition"), since the call to rdma_init_qp_attr() from > cma_init_conn_qp() does not hold the qp_mutex. > > Fixes: 2c1619edef61 ("IB/cma: Define option to set ack timeout and pack tos_set") > Fixes: 3aeffc46afde ("IB/cma: Introduce rdma_set_min_rnr_timer()") > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com> Sorry, not my day. Overlooked the access to timeout_set/timeout in cma_resolve_iboe_route(). Have to send a v2. Håkon > --- > drivers/infiniband/core/cma.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c > index e3f52c5..6b41527 100644 > --- a/drivers/infiniband/core/cma.c > +++ b/drivers/infiniband/core/cma.c > @@ -2457,8 +2457,10 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog) > if (IS_ERR(id)) > return PTR_ERR(id); > > + mutex_lock(&id_priv->qp_mutex); > id->tos = id_priv->tos; > id->tos_set = id_priv->tos_set; > + mutex_unlock(&id_priv->qp_mutex); > id->afonly = id_priv->afonly; > id_priv->cm_id.iw = id; > > @@ -2519,8 +2521,10 @@ static int cma_listen_on_dev(struct rdma_id_private *id_priv, > cma_id_get(id_priv); > dev_id_priv->internal_id = 1; > dev_id_priv->afonly = id_priv->afonly; > + mutex_lock(&id_priv->qp_mutex); > dev_id_priv->tos_set = id_priv->tos_set; > dev_id_priv->tos = id_priv->tos; > + mutex_unlock(&id_priv->qp_mutex); > > ret = rdma_listen(&dev_id_priv->id, id_priv->backlog); > if (ret) > @@ -2567,8 +2571,10 @@ void rdma_set_service_type(struct rdma_cm_id *id, int tos) > struct rdma_id_private *id_priv; > > id_priv = container_of(id, struct rdma_id_private, id); > + mutex_lock(&id_priv->qp_mutex); > id_priv->tos = (u8) tos; > id_priv->tos_set = true; > + mutex_unlock(&id_priv->qp_mutex); > } > EXPORT_SYMBOL(rdma_set_service_type); > > @@ -2595,8 +2601,10 @@ int rdma_set_ack_timeout(struct rdma_cm_id *id, u8 timeout) > return -EINVAL; > > id_priv = container_of(id, struct rdma_id_private, id); > + mutex_lock(&id_priv->qp_mutex); > id_priv->timeout = timeout; > id_priv->timeout_set = true; > + mutex_unlock(&id_priv->qp_mutex); > > return 0; > } > @@ -2632,8 +2640,10 @@ int rdma_set_min_rnr_timer(struct rdma_cm_id *id, u8 min_rnr_timer) > return -EINVAL; > > id_priv = container_of(id, struct rdma_id_private, id); > + mutex_lock(&id_priv->qp_mutex); > id_priv->min_rnr_timer = min_rnr_timer; > id_priv->min_rnr_timer_set = true; > + mutex_unlock(&id_priv->qp_mutex); > > return 0; > } > @@ -3019,8 +3029,11 @@ static int cma_resolve_iboe_route(struct rdma_id_private *id_priv) > > u8 default_roce_tos = id_priv->cma_dev->default_roce_tos[id_priv->id.port_num - > rdma_start_port(id_priv->cma_dev->device)]; > - u8 tos = id_priv->tos_set ? id_priv->tos : default_roce_tos; > + u8 tos; > > + mutex_lock(&id_priv->qp_mutex); > + tos = id_priv->tos_set ? id_priv->tos : default_roce_tos; > + mutex_unlock(&id_priv->qp_mutex); > > work = kzalloc(sizeof *work, GFP_KERNEL); > if (!work) > @@ -4092,8 +4105,11 @@ static int cma_connect_iw(struct rdma_id_private *id_priv, > if (IS_ERR(cm_id)) > return PTR_ERR(cm_id); > > + mutex_lock(&id_priv->qp_mutex); > cm_id->tos = id_priv->tos; > cm_id->tos_set = id_priv->tos_set; > + mutex_unlock(&id_priv->qp_mutex); > + > id_priv->cm_id.iw = cm_id; > > memcpy(&cm_id->local_addr, cma_src_addr(id_priv), > -- > 1.8.3.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-22 15:44 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-22 13:20 [PATCH for-next 0/2] Fix RMW to bit-fields and remove one superfluous ib_modify_qp Håkon Bugge 2021-06-22 13:20 ` [PATCH for-next 1/2] RDMA/cma: Remove unnecessary INIT->INIT transition Håkon Bugge 2021-06-22 14:47 ` Jason Gunthorpe 2021-06-22 14:53 ` Haakon Bugge 2021-06-22 14:59 ` Jason Gunthorpe 2021-06-22 15:44 ` Haakon Bugge 2021-06-22 13:20 ` [PATCH for-next 2/2] RDMA/cma: Protect RMW with qp_mutex Håkon Bugge 2021-06-22 13:30 ` Haakon Bugge
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.