Re: [PATCH v3 2/2] vmscan,cgroup: apply mems_effective to reclaim

[Waiman Long <llong@redhat.com>]

On 4/21/25 7:15 PM, Shakeel Butt wrote:
> On Mon, Apr 21, 2025 at 06:59:20PM -0400, Gregory Price wrote:
>> On Mon, Apr 21, 2025 at 10:39:58AM -0700, Shakeel Butt wrote:
>>> On Sat, Apr 19, 2025 at 08:14:29PM -0400, Waiman Long wrote:
>>>> On 4/19/25 2:48 PM, Shakeel Butt wrote:
>>>>> On Sat, Apr 19, 2025 at 01:38:24AM -0400, Gregory Price wrote:
>>>>>> +bool cpuset_node_allowed(struct cgroup *cgroup, int nid)
>>>>>> +{
>>>>>> +	struct cgroup_subsys_state *css;
>>>>>> +	unsigned long flags;
>>>>>> +	struct cpuset *cs;
>>>>>> +	bool allowed;
>>>>>> +
>>>>>> +	css = cgroup_get_e_css(cgroup, &cpuset_cgrp_subsys);
>>>>>> +	if (!css)
>>>>>> +		return true;
>>>>>> +
>>>>>> +	cs = container_of(css, struct cpuset, css);
>>>>>> +	spin_lock_irqsave(&callback_lock, flags);
>>>>> Do we really need callback_lock here? We are not modifying and I am
>>>>> wondering if simple rcu read lock is enough here (similar to
>>>>> update_nodemasks_hier() where parent's effective_mems is accessed within
>>>>> rcu read lock).
>>>> The callback_lock is required to ensure the stability of the effective_mems
>>>> which may be in the process of being changed if not taken.
>>> Stability in what sense? effective_mems will not get freed under us
>>> here or is there a chance for corrupted read here? node_isset() and
>>> nodes_empty() seems atomic. What's the worst that can happen without
>>> callback_lock?
>> Fairly sure nodes_empty is not atomic, it's a bitmap search.
> For bitmaps smaller than 64 bits, it seems atomic and MAX_NUMNODES seems
> smaller than 64 in all the archs.

RHEL sets MAX_NUMNODES to 1024 for x86_64. So it is not really atomic 
for some distros. In reality, it is rare to have a system with more than 
64 nodes (nr_node_ids <= 64). So it can be considered atomic in most cases.


>
> Anyways I am hoping that we can avoid taking a global lock in reclaim
> path which will become a source of contention for memory pressure
> situations.

It is a valid conern. I will not oppose to checking effective_mems 
without taking the callback_lock, but we will have to take rcu_read_lock 
to make sure that the cpuset structure won't go away and clearly 
document that this is an exceptional case as it is against our usual 
rule and the check may be incorrect in some rare cases.

Cheers,
Longman