Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Joel Fernandes <joel@joelfernandes.org>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 1/1] rcu/kvfree: Add runtime sleep check for single argument
Date:   Tue, 6 Dec 2022 09:45:11 -0500
Message-Id: <E8261077-6AFB-4FA6-AEAB-3BEFF5ABEC23@joelfernandes.org>
References: <20221206122357.309982-1-urezki@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, RCU <rcu@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <neeraj.iitr10@gmail.com>,
        Oleksiy Avramchenko <oleksiy.avramchenko@sony.com>
In-Reply-To: <20221206122357.309982-1-urezki@gmail.com>
To:     "Uladzislau Rezki (Sony)" <urezki@gmail.com>
Precedence: bulk

Hello Vlad,

> On Dec 6, 2022, at 7:24 AM, Uladzislau Rezki (Sony) <urezki@gmail.com> wro=
te:
>=20
> =EF=BB=BFA 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.
>=20
> 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 check=
ing 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 w=
hether we are blocking in an atomic context. That will not help Eric issue w=
hich is totally different - we would like to know if we are using an API whe=
re we can block instead of an API where we do not need to, by providing addi=
tional rcu_head space.

> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
> kernel/rcu/tree.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>=20
> 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 **krc=
p,
>    return true;
> }
>=20
> +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.

> +
> +    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 *p=
tr)
>     * 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 comme=
nt on top of the new function about false negatives for non-preemptible kern=
els. Can sleep sounds like might sleep which just adds confusion and does no=
t help code readers.

Also Paul raised a point about using 1-arg API in some sleepable contexts wh=
ere the caller does not want to introduce new space for the head. Have we co=
nfirmed there are not any? If there are, the warning will fire for those, as=
 false-positives.

Cheers,

 - Joel

> +    }
>=20
>    // Queue the object but don't yet schedule the batch.
>    if (debug_rcu_head_queue(ptr)) {
> --=20
> 2.30.2
>=20