Received-SPF: pass (google.com: domain of linux-kernel+bounces-104652-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e6e96b64-01b1-4e23-bb0b-45438f9a6cc4@suse.cz>
Date: Fri, 15 Mar 2024 17:52:50 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 23/37] mm/slab: add allocation accounting into slab
 allocation and free paths
Content-Language: en-US
To: Suren Baghdasaryan <surenb@google.com>
Cc: akpm@linux-foundation.org, kent.overstreet@linux.dev, mhocko@suse.com,
 hannes@cmpxchg.org, roman.gushchin@linux.dev, mgorman@suse.de,
 dave@stgolabs.net, willy@infradead.org, liam.howlett@oracle.com,
 penguin-kernel@i-love.sakura.ne.jp, corbet@lwn.net, void@manifault.com,
 peterz@infradead.org, juri.lelli@redhat.com, catalin.marinas@arm.com,
 will@kernel.org, arnd@arndb.de, tglx@linutronix.de, mingo@redhat.com,
 dave.hansen@linux.intel.com, x86@kernel.org, peterx@redhat.com,
 david@redhat.com, axboe@kernel.dk, mcgrof@kernel.org, masahiroy@kernel.org,
 nathan@kernel.org, dennis@kernel.org, jhubbard@nvidia.com, tj@kernel.org,
 muchun.song@linux.dev, rppt@kernel.org, paulmck@kernel.org,
 pasha.tatashin@soleen.com, yosryahmed@google.com, yuzhao@google.com,
 dhowells@redhat.com, hughd@google.com, andreyknvl@gmail.com,
 keescook@chromium.org, ndesaulniers@google.com, vvvvvv@google.com,
 gregkh@linuxfoundation.org, ebiggers@google.com, ytcoode@gmail.com,
 vincent.guittot@linaro.org, dietmar.eggemann@arm.com, rostedt@goodmis.org,
 bsegall@google.com, bristot@redhat.com, vschneid@redhat.com, cl@linux.com,
 penberg@kernel.org, iamjoonsoo.kim@lge.com, 42.hyeyoo@gmail.com,
 glider@google.com, elver@google.com, dvyukov@google.com,
 shakeelb@google.com, songmuchun@bytedance.com, jbaron@akamai.com,
 aliceryhl@google.com, rientjes@google.com, minchan@google.com,
 kaleshsingh@google.com, kernel-team@android.com, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, iommu@lists.linux.dev,
 linux-arch@vger.kernel.org, linux-fsdevel@vger.kernel.org,
 linux-mm@kvack.org, linux-modules@vger.kernel.org,
 kasan-dev@googlegroups.com, cgroups@vger.kernel.org
References: <20240306182440.2003814-1-surenb@google.com>
 <20240306182440.2003814-24-surenb@google.com>
 <1f51ffe8-e5b9-460f-815e-50e3a81c57bf@suse.cz>
 <CAJuCfpE5mCXiGLHTm1a8PwLXrokexx9=QrrRF4fWVosTh5Q7BA@mail.gmail.com>
From: Vlastimil Babka <vbabka@suse.cz>
In-Reply-To: <CAJuCfpE5mCXiGLHTm1a8PwLXrokexx9=QrrRF4fWVosTh5Q7BA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 3/15/24 16:43, Suren Baghdasaryan wrote:
> On Fri, Mar 15, 2024 at 3:58 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 3/6/24 19:24, Suren Baghdasaryan wrote:
>> > Account slab allocations using codetag reference embedded into slabobj_ext.
>> >
>> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>> > Co-developed-by: Kent Overstreet <kent.overstreet@linux.dev>
>> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
>> > Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>>
>> Nit below:
>>
>> > @@ -3833,6 +3913,7 @@ void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg,
>> >                         unsigned int orig_size)
>> >  {
>> >       unsigned int zero_size = s->object_size;
>> > +     struct slabobj_ext *obj_exts;
>> >       bool kasan_init = init;
>> >       size_t i;
>> >       gfp_t init_flags = flags & gfp_allowed_mask;
>> > @@ -3875,6 +3956,12 @@ void slab_post_alloc_hook(struct kmem_cache *s,        struct obj_cgroup *objcg,
>> >               kmemleak_alloc_recursive(p[i], s->object_size, 1,
>> >                                        s->flags, init_flags);
>> >               kmsan_slab_alloc(s, p[i], init_flags);
>> > +             obj_exts = prepare_slab_obj_exts_hook(s, flags, p[i]);
>> > +#ifdef CONFIG_MEM_ALLOC_PROFILING
>> > +             /* obj_exts can be allocated for other reasons */
>> > +             if (likely(obj_exts) && mem_alloc_profiling_enabled())

Could you at least flip these two checks then so the static key one goes first?

>> > +                     alloc_tag_add(&obj_exts->ref, current->alloc_tag, s->size);
>> > +#endif
>>
>> I think you could still do this a bit better:
>>
>> Check mem_alloc_profiling_enabled() once before the whole block calling
>> prepare_slab_obj_exts_hook() and alloc_tag_add()
>> Remove need_slab_obj_ext() check from prepare_slab_obj_exts_hook()
> 
> Agree about checking mem_alloc_profiling_enabled() early and one time,
> except I would like to use need_slab_obj_ext() instead of
> mem_alloc_profiling_enabled() for that check. Currently they are
> equivalent but if there are more slab_obj_ext users in the future then
> there will be cases when we need to prepare_slab_obj_exts_hook() even
> when mem_alloc_profiling_enabled()==false. need_slab_obj_ext() will be
> easy to extend for such cases.

I thought we don't generally future-proof internal implementation details
like this until it's actually needed. But at least what I suggested above
would help, thanks.

> Thanks,
> Suren.
> 
>>
>> >       }
>> >
>> >       memcg_slab_post_alloc_hook(s, objcg, flags, size, p);
>> > @@ -4353,6 +4440,7 @@ void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
>> >              unsigned long addr)
>> >  {
>> >       memcg_slab_free_hook(s, slab, &object, 1);
>> > +     alloc_tagging_slab_free_hook(s, slab, &object, 1);
>> >
>> >       if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
>> >               do_slab_free(s, slab, object, object, 1, addr);
>> > @@ -4363,6 +4451,7 @@ void slab_free_bulk(struct kmem_cache *s, struct slab *slab, void *head,
>> >                   void *tail, void **p, int cnt, unsigned long addr)
>> >  {
>> >       memcg_slab_free_hook(s, slab, p, cnt);
>> > +     alloc_tagging_slab_free_hook(s, slab, p, cnt);
>> >       /*
>> >        * With KASAN enabled slab_free_freelist_hook modifies the freelist
>> >        * to remove objects, whose reuse must be delayed.
>>