On 2/17/21 9:56 PM, Johannes Weiner wrote:
>> static inline void uncharge_gather_clear(struct uncharge_gather *ug)
>> @@ -6849,7 +6850,13 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)
>> * exclusive access to the page.
>> */
>>
>> - if (ug->memcg != page_memcg(page)) {
>> + if (ug->memcg != page_memcg(page) ||
>> + /*
>> + * Update soft limit tree used in v1 cgroup in page batch for
>> + * the same node. Relevant only to v1 cgroup with a soft limit.
>> + */
>> + (ug->dummy_page && ug->nid != page_to_nid(page) &&
>> + ug->memcg->soft_limit != PAGE_COUNTER_MAX)) {
>
> Sorry, I used weird phrasing in my last email.
>
> Can you please preface the checks you're adding with a
> !cgroup_subsys_on_dfl(memory_cgrp_subsys) to static branch for
> cgroup1? The uncharge path is pretty hot, and this would avoid the
> runtime overhead on cgroup2 at least, which doesn't have the SL.
>
> Also, do we need the ug->dummy_page check? It's only NULL on the first
> loop - where ug->memcg is NULL as well and the branch is taken anyway.
>
> The soft limit check is also slightly cheaper than the nid check, as
> page_to_nid() might be out-of-line, so we should do it first. This?
>
> /*
> * Batch-uncharge all pages of the same memcg.
> *
> * Unless we're looking at a cgroup1 with a softlimit
> * set: the soft limit trees are maintained per-node
> * and updated on uncharge (via dummy_page), so keep
> * batches confined to a single node as well.
> */
> if (ug->memcg != page_memcg(page) ||
> (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
> ug->memcg->soft_limit != PAGE_COUNTER_MAX &&
> ug->nid != page_to_nid(page)))
>
Johannes,
Thanks for your feedback. Since Michal has concerns about the overhead
this patch could incur, I think we'll hold the patch for now. If later
on Michal think that this patch is a good idea, I'll incorporate these
changes you suggested.
Tim
