Re: [PATCH v4] alloc_tag: check mem_profiling_support in alloc_tag_init

[Suren Baghdasaryan <surenb@google.com>]

On Tue, May 13, 2025 at 12:44 AM Casey Chen <cachen@purestorage.com> wrote:
>
> If mem_profiling_support is false, for example by
> sysctl.vm.mem_profiling=never, alloc_tag_init should skip
> module tags allocation, codetag type registration and
> procfs init.
>
> Signed-off-by: Casey Chen <cachen@purestorage.com>
> Reviewed-by: Yuanyuan Zhong <yzhong@purestorage.com>
> ---
>  lib/alloc_tag.c | 34 +++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index 25ecc1334b67..e8bf68458fb9 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -244,17 +244,6 @@ static void shutdown_mem_profiling(bool remove_file)
>         mem_profiling_support = false;
>  }
>
> -static void __init procfs_init(void)
> -{
> -       if (!mem_profiling_support)
> -               return;
> -
> -       if (!proc_create_seq(ALLOCINFO_FILE_NAME, 0400, NULL, &allocinfo_seq_op)) {
> -               pr_err("Failed to create %s file\n", ALLOCINFO_FILE_NAME);
> -               shutdown_mem_profiling(false);
> -       }
> -}
> -
>  void __init alloc_tag_sec_init(void)
>  {
>         struct alloc_tag *last_codetag;
> @@ -762,19 +751,34 @@ static int __init alloc_tag_init(void)
>         };
>         int res;
>
> +       sysctl_init();
> +
> +       if (!mem_profiling_support) {
> +               pr_info("Memory allocation profiling is not supported!\n");
> +               return 0;
> +       }
> +
> +       if (!proc_create_seq(ALLOCINFO_FILE_NAME, 0400, NULL, &allocinfo_seq_op)) {
> +               pr_err("Failed to create %s file\n", ALLOCINFO_FILE_NAME);
> +               shutdown_mem_profiling(false);
> +               return -EAGAIN;

Again, I don't think EAGAIN is the right thing to return here. This
error code means the caller should retry later but retrying here is
not an option and would solve nothing. Looking at proc_create_seq(),
it seems the main two reasons it might fail is an invalid argument or
lack of memory. So, it should be either EINVAL or ENOMEM. We know that
arguments in our call are valid (they are hardcoded), so that leaves
us with ENOMEM. That would be more appropriate code to return here.
Once that's fixes you can add:

Acked-by: Suren Baghdasaryan <surenb@google.com>

> +       }
> +
>         res = alloc_mod_tags_mem();
> -       if (res)
> +       if (res) {
> +               pr_err("Failed to reserve address space for module tags\n");

nit, you could report errno here as well, just like you did in the
next error report:

pr_err("Failed to reserve address space for module tags, errno = %d\n", res);

> +               shutdown_mem_profiling(true);
>                 return res;
> +       }
>
>         alloc_tag_cttype = codetag_register_type(&desc);
>         if (IS_ERR(alloc_tag_cttype)) {
> +               pr_err("Allocation tags registration failed, errno = %ld\n", PTR_ERR(alloc_tag_cttype));
>                 free_mod_tags_mem();
> +               shutdown_mem_profiling(true);
>                 return PTR_ERR(alloc_tag_cttype);
>         }
>
> -       sysctl_init();
> -       procfs_init();
> -
>         return 0;
>  }
>  module_init(alloc_tag_init);
> --
> 2.49.0
>