Re: [PATCH v7 2/3] btrfs: always uses root memcgroup for filemap_add_folio()

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/7/20 03:43, Michal Hocko 写道:
[...]
>
>>> +	set_active_memcg(old_memcg);
>>
>> It looks correct. But it's going through all dance to set up
>> current->active_memcg, then have the charge path look that up,
>> css_get(), call try_charge() only to bail immediately, css_put(), then
>> update current->active_memcg again. All those branches are necessary
>> when we want to charge to a "real" other cgroup. But in this case, we
>> always know we're not charging, so it seems uncalled for.
>>
>> Wouldn't it be a lot simpler (and cheaper) to have a
>> filemap_add_folio_nocharge()?
>
> Yes, that would certainly simplify things. From the previous discussion
> I understood that there would be broader scopes which would opt-out from
> charging. If this is really about a single filemap_add_folio call then
> having a variant without doesn't call mem_cgroup_charge sounds like a
> much more viable option and also it doesn't require to make any memcg
> specific changes.
>

Talking about skipping mem cgroup charging, I still have a question.

[MEMCG AT FOLIO EVICTION TIME]
Even we completely skip the mem cgroup charging, we cannot really escape
the eviction time handling.

In fact if we just skip the mem_cgroup_charge(), kernel would crash when
evicting the folio.
As in lru_gen_eviction(), folio_memcg() would just return NULL, and
mem_cgroup_id(memcg) would trigger a NULL pointer dereference.

That's why I sent out a patch fixing that first:
https://lore.kernel.org/linux-mm/e1036b9cc8928be9a7dec150ab3a0317bd7180cf.1720572937.git.wqu@suse.com/

I'm not sure if it's going to cause any extra problems even with the
above fix.

And just for the sake of consistency, it looks more sane to have
root_mem_cgroup for the filemap_add_folio() operation, other than leave
it empty, especially since most filemaps still need proper memcg handling.


[REALLY EXPENSIVE?]
Another question is, is the set_active_memcg() and later handling really
that expensive?

set_active_memcg() is small enough to be an inline function, so is the
active_memcg(), css_get() and the root memcg path of try_charge().

Later commit part is not that expensive either, mostly simple member or
per-cpu assignment.

According to my very little knowledge about mem cgroup, most of the
heavy lifting part is in the slow path of try_charge_memcg().

Even with all the set_active_memcg(), the whole extra overhead still
look very tiny.
And it should already be a big win for btrfs to opt-out the regular
charging routine.

Thanks,
Qu