Re: [PATCH 1/1] rcu/kvfree: Add runtime sleep check for single argument

[Uladzislau Rezki]

On Tue, Dec 06, 2022 at 09:45:11AM -0500, Joel Fernandes wrote:
> Hello Vlad,
> 
> > On Dec 6, 2022, at 7:24 AM, Uladzislau Rezki (Sony) <urezki@xxxxxxxxx> wrote:
> > 
> > A single argument can be invoked only from a sleepable
> > context. There is already a might_sleep() check to mitigate
> > such cases.
> > The problem is that it requires a kernel to
> > be built with a CONFIG_DEBUG_ATOMIC_SLEEP option.
> > 
> > In order to improve an extra runtime_assert_can_sleep()
> > function is introduced by this patch. It is a run-time
> > checking. Please note it only covers PREEMPT
> 
> I would call it preemptible kernels.
> 
> > kernels.
> 
> Also, It is not clear at all from the commit message about what we are checking and why. Neither is it clear why the might_sleep() is insufficient.
> 
> The whole point of doing this is, the purpose of might_sleep() is to check whether we are blocking in an atomic context. That will not help Eric issue which is totally different - we would like to know if we are using an API where we can block instead of an API where we do not need to, by providing additional rcu_head space.
> 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@xxxxxxxxx>
> > ---
> > kernel/rcu/tree.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index d155f2594317..bb798f07e768 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -3232,6 +3232,19 @@ add_ptr_to_bulk_krc_lock(struct kfree_rcu_cpu **krcp,
> >    return true;
> > }
> > 
> > +static void
> > +runtime_assert_can_sleep(void)
> > +{
> > +    if (!IS_ENABLED(CONFIG_PREEMPT_COUNT))
> > +        return;
> > +
> > +    if (preemptible())
> > +        return;
> 
> These 2 iffs can be combined into 2-3 lines in this function. No need to add more LOC than needed.
> 
It can be.

> > +
> > +    WARN_ONCE(1, "in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
> > +        in_atomic(), irqs_disabled(), current->pid, current->comm);
> > +}
> > +
> > /*
> >  * Queue a request for lazy invocation of the appropriate free routine
> >  * after a grace period.  Please note that three paths are maintained,
> > @@ -3257,8 +3270,10 @@ void kvfree_call_rcu(struct rcu_head *head, void *ptr)
> >     * only. For other places please embed an rcu_head to
> >     * your data.
> >     */
> > -    if (!head)
> > +    if (!head) {
> > +        runtime_assert_can_sleep();
> >        might_sleep();
> 
> runtime_assert_preemptible() is a better name with a comment about big comment on top of the new function about false negatives for non-preemptible kernels. Can sleep sounds like might sleep which just adds confusion and does not help code readers.
> 
> Also Paul raised a point about using 1-arg API in some sleepable contexts where the caller does not want to introduce new space for the head. Have we confirmed there are not any? If there are, the warning will fire for those, as false-positives.
> 
single argument is only for sleepable contexts. This is a rule.

--
Uladzislau Rezki