On Wed, Aug 28, 2024 at 1:16 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
>
> On Wed, Aug 28, 2024 at 12:42:02PM GMT, Yosry Ahmed wrote:
> > On Wed, Aug 28, 2024 at 12:14 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 27, 2024 at 05:34:24PM GMT, Yosry Ahmed wrote:
> > > > On Tue, Aug 27, 2024 at 4:52 PM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
> > > [...]
> > > > > +
> > > > > +#define KMALLOC_TYPE (SLAB_KMALLOC | SLAB_CACHE_DMA | \
> > > > > + SLAB_ACCOUNT | SLAB_RECLAIM_ACCOUNT)
> > > > > +
> > > > > +static __fastpath_inline
> > > > > +bool memcg_slab_post_charge(void *p, gfp_t flags)
> > > > > +{
> > > > > + struct slabobj_ext *slab_exts;
> > > > > + struct kmem_cache *s;
> > > > > + struct folio *folio;
> > > > > + struct slab *slab;
> > > > > + unsigned long off;
> > > > > +
> > > > > + folio = virt_to_folio(p);
> > > > > + if (!folio_test_slab(folio)) {
> > > > > + return __memcg_kmem_charge_page(folio_page(folio, 0), flags,
> > > > > + folio_order(folio)) == 0;
> > > >
> > > > Will this charge the folio again if it was already charged? It seems
> > > > like we avoid this for already charged slab objects below but not
> > > > here.
> > > >
> > >
> > > Thanks for catchig this. It's an easy fix and will do in v3.
> > >
> > > > > + }
> > > > > +
> > > > > + slab = folio_slab(folio);
> > > > > + s = slab->slab_cache;
> > > > > +
> > > > > + /* Ignore KMALLOC_NORMAL cache to avoid circular dependency. */
> > > > > + if ((s->flags & KMALLOC_TYPE) == SLAB_KMALLOC)
> > > > > + return true;
> > > >
> > > > Would it be clearer to check if the slab cache is one of
> > > > kmalloc_caches[KMALLOC_NORMAL]? This should be doable by comparing the
> > > > address of the slab cache with the addresses of
> > > > kmalloc_cache[KMALLOC_NORMAL] (perhaps in a helper). I need to refer
> > > > to your reply to Roman to understand why this works.
> > > >
> > >
> > > Do you mean looping over kmalloc_caches[KMALLOC_NORMAL] and comparing
> > > the given slab cache address? Nah man why do long loop of pointer
> > > comparisons when we can simply check the flag of the given kmem cache.
> > > Also this array will increase with the recent proposed random kmalloc
> > > caches.
> >
> > Oh I thought kmalloc_caches[KMALLOC_NORMAL] is an array of the actual
> > struct kmem_cache objects, so I thought we can just check if:
> > s >= kmalloc_caches[KMALLOC_NORMAL][0] &&
> > s >= kmalloc_caches[KMALLOC_NORMAL][LAST_INDEX]
> >
> > I just realized it's an array of pointers, so we would need to loop
> > and compare them.
> >
> > I still find the flags comparisons unclear and not very future-proof
> > tbh. I think we can just store the type in struct kmem_cache? I think
> > there are multiple holes there.
>
> Do you mean adding a new SLAB_KMALLOC_NORMAL? I will wait for SLAB
> maintainers for their opinion on that. BTW this kind of checks are in
> the kernel particularly for gfp flags.
I meant maybe in new_kmalloc_cache() pass in 'type' to
create_kmalloc_cache() and store it in struct kmem_cache (we'd want a
KMALLOC_NONE a similar for non-kmalloc caches). Having a new flag like
SLAB_KMALLOC_NORMAL would also work.
Or maybe using the flags to deduce the kmalloc cache type is fine, but
in this case I think a well-documented helper that takes in a
kmem_cache and restores a type based on the combination of flags would
be better.
I just think in this case it's easy for the flags to change from under
us here, and the code is not very clear.
Hopefully the slab maintainers will tell us what they think here, my
concerns could very possibly be unfounded.
