Re: [PATCH 6/6] slab: Introduce kmalloc_nolock() and kfree_nolock().

[Shakeel Butt <shakeel.butt@linux.dev>]

On Mon, May 05, 2025 at 06:51:28PM -0700, Alexei Starovoitov wrote:
> On Mon, May 5, 2025 at 6:25 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > > > >               obj_exts[off].objcg = NULL;
> > > > > -             refill_obj_stock(objcg, obj_size, true, -obj_size,
> > > > > -                              slab_pgdat(slab), cache_vmstat_idx(s));
> > > > > +             if (unlikely(lock_held)) {
> > > > > +                     atomic_add(obj_size, &objcg->nr_charged_bytes);
> > > >
> > > > objcg->nr_charged_bytes is stats ignorant and the relevant stats need to
> > > > be updated before putting stuff into it.
> > >
> > > I'm not following.
> > > It's functionally equivalent to refill_obj_stock() without
> > > __account_obj_stock().
> > > And the stats are not ignored.
> > > The next __memcg_slab_free_hook() from good context will update
> > > them. It's only a tiny delay in update.
> > > I don't see why it's an issue.
> >
> > For the slab object of size obj_size which is being freed here, we need
> > to update NR_SLAB_RECLAIMABLE_B or NR_SLAB_UNRECLAIMABLE_B stat for the
> > corresponding objcg by the amount of obj_size. If we don't call
> > __account_obj_stock() here we will loose the context and information to
> > update these stats later.
> 
> Lose context?
> pgdat has to match objcg, so I don't think we lose anything.
> Later refill_obj_stock() will take objcg->nr_charged_bytes and
> apply to appropriate NR_SLAB_[UN]RECLAIMABLE_B.

refill_obj_stock() will not apply objcg->nr_charged_bytes to
NR_SLAB_[UN]RECLAIMABLE_B. This is confusing because per-cpu objcg stock
has two distinct functions i.e. (1) byte level charge cache and (2) slab
stats cache, though both of them happen to share objcg (this will change
when I add multi-objcg support).

Let me explain this with an example. Let's say in the task context,
Job-A frees a charged slab object on CPU-i and also assume CPU-i's
memcg_stock has Job-A's objcg cached. Next let's assume while CPU-i was
in refill_obj_stock(), it got interrupted by a nmi and within nmi there
is a slab object charged to Job-B got freed.

In the above scenario and with this patch, the kernel will put the
obj_size of the Job-B's freed slab object to Job-B's
objcg->nr_charged_bytes and just return. Please note that CPU-i still
has Job-A's objcg (charge and stats) cached. When CPU-i's memcg_stock
get drained later, it will update stats of Job-A which were cached on
it. We have lost the stat updates for Job-B's freed slab object.