Re: [PATCH v2 4/9] mm: percpu: Split memcg charging and kmem accounting

[Joshua Hahn]

> This is preparatory patch for upcoming per-memcg-per-node kmem
> accounting.

Hi Alex, I hope you are doing well. Thank you for the series!
And sorry that I took so long to review. I'm slowly going through the
series, so I expect to leave more comments in the future. 

> Percpu allocations charge memory before knowing which NUMA nodes the
> pages will land on. So we need to decouple the memcg charging from the
> kmem accounting:
> 
> 1. In the pre-alloc hook, obj_cgroup_precharge() reserves pages for
>    memcg limit enforcement without updating kmem stats.
> 2. In the post-alloc hook, obj_cgroup_account_kmem() accounts kmem
>    and places the sub-page remainder into the obj stock after the
>    allocation succeeds.
> 
> Because of that decoupling, we must not rely on the stock in the
> precharge function and always charge the necessary pages that will
> be accounted after the allocations happened.

I was thinking about this, I think the reality of the sitaution is
actually a bit better than the description makes it seem. We're still
using stock, just a different level so we're not losing any
fastpath at all. Previously we would use the objcg stock to do the
charge quickly, but if we're going straight to memcg charging, we'll
be using the memcg stock instead. Maybe a clarification here that
we're not relying on the obj_stock would help your case here : -)

> That means we may
> temporarily overcharge the memcg but the obj_stock draining will get
> things back to normal.

Nice, this is quite clean. We might hit memcg limits faster because
we overcharge up to PAGE_SIZE bytes but... I'm not sure if this is even
worth noting since the window is so small. 

I think in all the places where you use PAGE_ALIGN(x) >> PAGE_SHIFT
we can instead use PFN_UP(x)? Or maybe DIV_ROUND_UP(x, PAGE_SIZE)?
Just thought that this way we could avoid some of the two-line function
calls ; -)

> Signed-off-by: Alexandre Ghiti <alex@xxxxxxxx>

Anyways I think the split looks clean and makes sense to me. Sashiko
raised no concerns either, just wanted to note some nits. I imagine
there will be a v3 in the future? I think this patch looks good but
I will wait until the next version to leave my signatures in case
some of the context changes.

Thanks again, Alex! I hope you have a great day,
Joshua