* [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
* [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
* 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
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.