The Linux Kernel Mailing List
 help / color / mirror / Atom feed
From: Hao Ge <hao.ge@linux.dev>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Petr Pavlu <petr.pavlu@suse.com>,
	Daniel Gomez <da.gomez@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Aaron Tomlin <atomlin@atomlin.com>,
	linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [RFC PATCH 1/3] alloc_tag: skip percpu counter allocation when profiling is disabled
Date: Mon, 17 Aug 2026 13:05:36 +0800	[thread overview]
Message-ID: <e19d1b50-4d10-4833-8770-5003a0593518@linux.dev> (raw)
In-Reply-To: <CAJuCfpHAzhCC1Sx3uJr7kbhBzc7RFqwLhMyjo04hqZz=HBJH8Q@mail.gmail.com>

Hi Suren


On 2026/8/15 14:15, Suren Baghdasaryan wrote:
> On Thu, Aug 13, 2026 at 2:34 AM Hao Ge <hao.ge@linux.dev> wrote:
>>
>> After shutdown_mem_profiling() clears mem_profiling_support,
>> needs_section_mem() returns false, so later modules have their codetag
>> section placed as regular data and never enter the alloc_tag maple tree.
>> codetag_load_module() still called load_module(), which allocated a percpu
>> counter for every tag; release_module_tags() could not find these modules
>> on unload, so the counters leaked.
>>
>> Return CODETAG_MODULE_EXCLUDED from load_module() when profiling is off:
>> codetag_module_init() drops the module's cmod and no counters are
>> allocated. codetag_unload_module() now always calls free_section_mem(),
>> since an excluded module may still hold a reserved section.
>>
>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> 
> Thanks for the fix. I think it could be done simpler, see below.

Agree, will do in the next version.

> 
>> ---
>>  include/linux/codetag.h | 4 ++++
>>  lib/codetag.c           | 8 +++++---
>>  mm/alloc_tag.c          | 8 ++++++--
>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/codetag.h b/include/linux/codetag.h
>> index a25a085c2df1..88081c618673 100644
>> --- a/include/linux/codetag.h
>> +++ b/include/linux/codetag.h
>> @@ -52,6 +52,10 @@ struct codetag_type_desc {
>>  #endif
>>  };
>>
>> +/* module_load() return values */
>> +#define CODETAG_MODULE_LOAD    0       /* module loads with its tags */
>> +#define CODETAG_MODULE_EXCLUDED        1       /* module loads without its tags */
> 
> I see no reason for adding these special values. You could simply
> return -ENOTSUP when profiling is disabled.
> 
>> +
>>  struct codetag_iterator {
>>         struct codetag_type *cttype;
>>         struct codetag_module *cmod;
>> diff --git a/lib/codetag.c b/lib/codetag.c
>> index a9cda4c962a3..8506ecab9ea7 100644
>> --- a/lib/codetag.c
>> +++ b/lib/codetag.c
>> @@ -238,9 +238,10 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
>>         }
>>         up_write(&cttype->mod_lock);
>>
>> -       if (err < 0) {
>> +       if (err) {
>> +               /* Error or excluded: cmod is dropped, free it. */
> 
> IIUC here you want to call kfree() if profiling got disabled. If you
> return -ENOTSUP instead of CODETAG_MODULE_EXCLUDED then this condition
> does not need to change.
> 
>>                 kfree(cmod);
>> -               return err;
>> +               return err < 0 ? err : 0;
> 
> Here you can do:
>                     if (err && err != -ENOTSUP)
>                             return err;
>                    return 0;
> 
>>         }
>>
>>         return 0;
>> @@ -388,7 +389,8 @@ void codetag_unload_module(struct module *mod)
>>                         ++cttype->content_id;
>>                 }
>>                 up_write(&cttype->mod_lock);
>> -               if (found && cttype->desc.free_section_mem)
>> +               /* an excluded module may still hold section memory */
>> +               if (cttype->desc.free_section_mem)
>>                         cttype->desc.free_section_mem(mod, true);
>>         }
>>         mutex_unlock(&codetag_lock);
>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>> index 0a7b657fe2de..461fa87fbb0b 100644
>> --- a/mm/alloc_tag.c
>> +++ b/mm/alloc_tag.c
>> @@ -977,9 +977,13 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
>>         struct alloc_tag *stop_tag;
>>         struct alloc_tag *tag;
>>
>> +       /* Profiling disabled: load the module but exclude its tags. */
>> +       if (!mem_profiling_support)
>> +               return CODETAG_MODULE_EXCLUDED;
> 
> Return -ENOTSUP here.
> 
> 
> 
>> +
>>         /* percpu counters for core allocations are already statically allocated */
>>         if (!mod)
>> -               return 0;
>> +               return CODETAG_MODULE_LOAD;
>>
>>         start_tag = ct_to_alloc_tag(start);
>>         stop_tag = ct_to_alloc_tag(stop);
>> @@ -1002,7 +1006,7 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
>>                  */
>>                 kmemleak_ignore_percpu(tag->counters);
>>         }
>> -       return 0;
>> +       return CODETAG_MODULE_LOAD;
>>  }
>>
>>  static void replace_module(struct module *mod, struct module *new_mod)
>> --
>> 2.25.1
>>

  reply	other threads:[~2026-08-17  5:04 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-13  9:34 [RFC PATCH 0/3] alloc_tag: fix races and a leak around shutdown_mem_profiling() Hao Ge
2026-08-13  9:34 ` [RFC PATCH 1/3] alloc_tag: skip percpu counter allocation when profiling is disabled Hao Ge
2026-08-15  6:15   ` Suren Baghdasaryan
2026-08-17  5:05     ` Hao Ge [this message]
2026-08-13  9:34 ` [RFC PATCH 2/3] module: move codetag section placement decision to layout_sections() Hao Ge
2026-08-15  6:21   ` Suren Baghdasaryan
2026-08-17  5:37     ` Hao Ge
2026-08-13  9:34 ` [RFC PATCH 3/3] alloc_tag: remove /proc/allocinfo outside of mod_lock Hao Ge
2026-08-15  6:51   ` Suren Baghdasaryan
2026-08-17  5:41     ` Hao Ge

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e19d1b50-4d10-4833-8770-5003a0593518@linux.dev \
    --to=hao.ge@linux.dev \
    --cc=akpm@linux-foundation.org \
    --cc=atomlin@atomlin.com \
    --cc=da.gomez@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=petr.pavlu@suse.com \
    --cc=samitolvanen@google.com \
    --cc=surenb@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox