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;
MIME-Version: 1.0
References: <20220630094646.91837-1-haibo.li@mediatek.com> <20220630094646.91837-3-haibo.li@mediatek.com>
In-Reply-To: <20220630094646.91837-3-haibo.li@mediatek.com>
From:   Sami Tolvanen <samitolvanen@google.com>
Date:   Thu, 30 Jun 2022 09:16:53 -0700
Message-ID: <CABCJKudz5O5_-1q1H0o2DFRcnpMQDBPv4STUVa2bOE9ePaeyDA@mail.gmail.com>
Subject: Re: [PATCH 2/2] ANDROID: cfi: free old cfi shadow asynchronously
To:     Haibo Li <haibo.li@mediatek.com>
Cc:     xiaoming.yu@mediatek.com, Kees Cook <keescook@chromium.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        =?UTF-8?Q?Andr=C3=A9_Almeida?= <andrealmeid@igalia.com>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Juergen Gross <jgross@suse.com>,
        Tiezhu Yang <yangtiezhu@loongson.cn>,
        Aaron Tomlin <atomlin@redhat.com>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>, llvm@lists.linux.dev,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@lists.infradead.org>,
        Lecopzer Chen <lecopzer.chen@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Thu, Jun 30, 2022 at 2:47 AM Haibo Li <haibo.li@mediatek.com> wrote:
>
> Currenly, it uses synchronize_rcu() to wait old rcu reader to go away
> in update_shadow.In embedded platform like ARM CA7X,
> load_module blocks 40~50ms in update_shadow.
> When there are more than one hundred kernel modules,
> it blocks several seconds.
>
> To accelerate load_module,change synchronize_rcu to call_rcu.
>
> Signed-off-by: Haibo Li <haibo.li@mediatek.com>
> Signed-off-by: Lecopzer Chen <lecopzer.chen@mediatek.com>

Thanks for the patch! Please drop ANDROID: from the subject line,
that's only used in the Android kernel trees.

>  kernel/cfi.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 456771c8e454..a4836d59ca27 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -43,6 +43,8 @@ typedef u16 shadow_t;
>  struct cfi_shadow {
>         /* Page index for the beginning of the shadow */
>         unsigned long base;
> +       /* rcu to free old cfi_shadow asynchronously */
> +       struct rcu_head rcu;
>         /* An array of __cfi_check locations (as indices to the shadow) */
>         shadow_t shadow[1];
>  } __packed;
> @@ -182,6 +184,13 @@ static void remove_module_from_shadow(struct cfi_shadow *s, struct module *mod,
>         }
>  }
>
> +static void _cfi_shadow_free_rcu(struct rcu_head *rcu)

I think this can be simply renamed to free_shadow.

> +{
> +       struct cfi_shadow *old = container_of(rcu, struct cfi_shadow, rcu);
> +
> +       vfree(old);
> +}
> +
>  typedef void (*update_shadow_fn)(struct cfi_shadow *, struct module *,
>                         unsigned long min_addr, unsigned long max_addr);
>
> @@ -211,11 +220,10 @@ static void update_shadow(struct module *mod, unsigned long base_addr,
>
>         rcu_assign_pointer(cfi_shadow, next);
>         mutex_unlock(&shadow_update_lock);
> -       synchronize_rcu();
>
>         if (prev) {
>                 set_memory_rw((unsigned long)prev, SHADOW_PAGES);
> -               vfree(prev);
> +               call_rcu(&prev->rcu, _cfi_shadow_free_rcu);
>         }
>  }

It's probably better to keep the pages read-only until they're
actually released. Can you move the set_memory_rw call to the new
function?

Sami