Johannes, Hugh,
what do you think about this approach? If we want to stick with
split_page approach then we need to update the missing place Matthew has
pointed out.
On Tue 09-03-21 14:03:36, Michal Hocko wrote:
> On Tue 09-03-21 12:32:55, Matthew Wilcox wrote:
> > On Tue, Mar 09, 2021 at 10:02:00AM +0100, Michal Hocko wrote:
> [...]
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 913c2b9e5c72..d44dea2b8d22 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -3135,8 +3135,21 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
> > > if (memcg && !mem_cgroup_is_root(memcg)) {
> > > ret = __memcg_kmem_charge(memcg, gfp, 1 << order);
> > > if (!ret) {
> > > + int nr_pages = 1 << order;
> > > page->memcg_data = (unsigned long)memcg |
> > > MEMCG_DATA_KMEM;
> > > +
> > > + /*
> > > + * Compound pages are normally split or freed
> > > + * via their head pages so memcg_data in in the
> > > + * head page should be sufficient but there
> > > + * are exceptions to the rule (see __free_pages).
> > > + * Non compound pages would need to copy memcg anyway.
> > > + */
> > > + for (i = 1; i < nr_pages; i++) {
> > > + struct page * p = page + i;
> > > + p->memcg_data = page->memcg_data
> > > + }
> > > return 0;
> >
> > I would condition this loop on if (!(gfp & __GFP_COMP)), but yes, something
> > along these lines. I might phrase the comment a little differently ...
> >
> > /*
> > * Compound pages are treated as a single unit,
> > * but non-compound pages can be freed individually
> > * so each page needs to have its memcg set to get
> > * the accounting right.
> > */
>
> OK, I must have misunderstood your __free_pages fix then. I thought this
> was about compound pages. Btw. again I forgot about css ref counting so
> here is an updated version.
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 913c2b9e5c72..ec2c705f38fa 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3133,10 +3133,22 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
>
> memcg = get_mem_cgroup_from_current();
> if (memcg && !mem_cgroup_is_root(memcg)) {
> - ret = __memcg_kmem_charge(memcg, gfp, 1 << order);
> + int nr_pages = 1 << order;
> + ret = __memcg_kmem_charge(memcg, gfp, nr_pages);
> if (!ret) {
> page->memcg_data = (unsigned long)memcg |
> MEMCG_DATA_KMEM;
> + if (nr_pages > 1) {
> + /*
> + * comment goes here
> + */
> + for (i = 1; i < nr_pages; i++) {
> + struct page * p = page + i;
> + p->memcg_data = page->memcg_data
> + }
> + /* Head page reference from get_mem_cgroup_from_current */
> + css_get_many(&memcg->css, nr_pages - 1);
> + }
> return 0;
> }
> css_put(&memcg->css);
--
Michal Hocko
SUSE Labs
