> On Jun 11, 2023, at 2:07 PM, Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> On Thu, Jun 08, 2023 at 04:07:13PM -0400, Chuck Lever wrote:
>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>
>> drivers/infiniband/core/cma.c:2090:13: warning: context imbalance in 'destroy_id_handler_unlock' - wrong count at exit
>> drivers/infiniband/core/cma.c:2113:6: warning: context imbalance in 'rdma_destroy_id' - unexpected unlock
>> drivers/infiniband/core/cma.c:2256:17: warning: context imbalance in 'cma_ib_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:2448:17: warning: context imbalance in 'cma_ib_req_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:2571:17: warning: context imbalance in 'cma_iw_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:2616:17: warning: context imbalance in 'iw_conn_req_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:3035:17: warning: context imbalance in 'cma_work_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:3542:17: warning: context imbalance in 'addr_handler' - unexpected unlock
>> drivers/infiniband/core/cma.c:4269:17: warning: context imbalance in 'cma_sidr_rep_handler' - unexpected unlock
>
> Strange, I was under impression that we don't have sparse errors in cma.c
They might show up only if certain CONFIG options are enabled.
For example, I have
CONFIG_LOCK_DEBUGGING_SUPPORT=y
CONFIG_PROVE_LOCKING=y
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> drivers/infiniband/core/cma.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index 10a1a8055e8c..35c8d67a623c 100644
>> --- a/drivers/infiniband/core/cma.c
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -2058,7 +2058,7 @@ static void _destroy_id(struct rdma_id_private *id_priv,
>> * handlers can start running concurrently.
>> */
>> static void destroy_id_handler_unlock(struct rdma_id_private *id_priv)
>> - __releases(&idprv->handler_mutex)
>> + __must_hold(&idprv->handler_mutex)
>
> According to the Documentation/dev-tools/sparse.rst
> 64 __must_hold - The specified lock is held on function entry and exit.
> 65
> 66 __acquires - The specified lock is held on function exit, but not entry.
> 67
> 68 __releases - The specified lock is held on function entry, but not exit.
Fair enough, but the warnings vanish with this patch. Something
ain't right here.
> In our case, handler_mutex is unlocked while exiting from destroy_id_handler_unlock().
Sure, that is the way I read the code too. However I don't agree
that this structure makes it easy to eye-ball the locks and unlocks.
Even sparse 0.6.4 seems to be confused by this arrangement.
Sometimes deduplication can be taken too far.
> Thanks
>
>> {
>> enum rdma_cm_state state;
>> unsigned long flags;
>> @@ -5153,7 +5153,6 @@ static void cma_netevent_work_handler(struct work_struct *_work)
>> event.status = -ETIMEDOUT;
>>
>> if (cma_cm_event_handler(id_priv, &event)) {
>> - __acquire(&id_priv->handler_mutex);
>> id_priv->cm_id.ib = NULL;
>> cma_id_put(id_priv);
>> destroy_id_handler_unlock(id_priv);
--
Chuck Lever
