Message-ID: <7a0dfc9f-6d21-fce4-4b83-72bb171454d3@gmail.com>
Date:   Wed, 15 Mar 2023 14:05:19 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2 13/14] RDMA/rxe: Rename kfree_rcu() to
 kvfree_rcu_mightsleep()
Content-Language: en-US
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     Zhu Yanjun <zyjzyj2000@gmail.com>, Jason Gunthorpe <jgg@ziepe.ca>,
        Leon Romanovsky <leon@kernel.org>,
        Devesh Sharma <devesh.s.sharma@oracle.com>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Zhu Yanjun <yanjun.zhu@linux.dev>, linux-rdma@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230315181902.4177819-1-joel@joelfernandes.org>
 <20230315181902.4177819-13-joel@joelfernandes.org>
 <e704127e-1bfe-f351-db95-bfea6916e8f9@gmail.com>
 <CAEXW_YTB+LBG+oL02c0JfmAzoGSkZDM=QW1EXKbO3f-g0i4ddA@mail.gmail.com>
From:   Bob Pearson <rpearsonhpe@gmail.com>
In-Reply-To: <CAEXW_YTB+LBG+oL02c0JfmAzoGSkZDM=QW1EXKbO3f-g0i4ddA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/15/23 13:41, Joel Fernandes wrote:
> On Wed, Mar 15, 2023 at 2:38 PM Bob Pearson <rpearsonhpe@gmail.com> wrote:
>>
>> On 3/15/23 13:19, Joel Fernandes (Google) wrote:
>>> The k[v]free_rcu() macro's single-argument form is deprecated.
>>> Therefore switch to the new k[v]free_rcu_mightsleep() variant. The goal
>>> is to avoid accidental use of the single-argument forms, which can
>>> introduce functionality bugs in atomic contexts and latency bugs in
>>> non-atomic contexts.
>>>
>>> There is no functionality change with this patch.
>>>
>>> Link: https://lore.kernel.org/rcu/20230201150815.409582-1-urezki@gmail.com
>>> Acked-by: Zhu Yanjun <zyjzyj2000@gmail.com>
>>> Reviewed-by: Bob Pearson <rpearsonhpe@gmail.com>
>>> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
>>> Fixes: 72a03627443d ("RDMA/rxe: Remove rxe_alloc()")
>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> ---
>>>  drivers/infiniband/sw/rxe/rxe_mr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/infiniband/sw/rxe/rxe_mr.c b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> index b10aa1580a64..ae3a100e18fb 100644
>>> --- a/drivers/infiniband/sw/rxe/rxe_mr.c
>>> +++ b/drivers/infiniband/sw/rxe/rxe_mr.c
>>> @@ -731,7 +731,7 @@ int rxe_dereg_mr(struct ib_mr *ibmr, struct ib_udata *udata)
>>>               return -EINVAL;
>>>
>>>       rxe_cleanup(mr);
>>> -     kfree_rcu(mr);
>>> +     kfree_rcu_mightsleep(mr);
>>>       return 0;
>>>  }
>>>
>> Please replace kfree_rcu_mightsleep() by kfree(). There is no need to delay the kfree(mr).
> 
> I thought you said either was Ok, but yeah that's fine with me. I was
> trying to play it safe ;-). An extra GP may not hurt, but not having
> one when it is needed might. I will update it to just be kfree.

Thanks. The reason to not add the pause is that it really isn't required and will just make
people think that it is. With the current state of the driver the mr cleanup code will disable
looking up the mr from its rkey or lkey and then wait until all the references to the mr are dropped
before calling kfree. This will always work (unless a new bug is introduced) so no reason to
add the rcu delay.

Bob
> 
> <quote>
>>   rxe_cleanup(mr);
>> - kfree_rcu(mr);
>> + kfree_rcu_mightsleep(mr);
>>   return 0;
>>  }
>>
> 
> I would prefer just
> - kfree_rcu(mr);
> + kfree(mr);
> 
> but either one will work.
> Bob
> </quote>
> 
>  - Joel