Message-ID: <7b331383-e6b7-f94b-6af8-72d5d8d688bf@linux.alibaba.com>
Date:   Wed, 15 Mar 2023 23:06:26 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH] x86/resctrl: Only show tasks' pids in current pid
 namespace
To:     Reinette Chatre <reinette.chatre@intel.com>, fenghua.yu@intel.com
Cc:     tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, james.morse@arm.com, x86@kernel.org,
        hpa@zytor.com, linux-kernel@vger.kernel.org
References: <20230116071246.97717-1-shawnwang@linux.alibaba.com>
 <95fca7d1-d473-e9e2-b6c8-c4ae3d44d2df@intel.com>
From:   Shawn Wang <shawnwang@linux.alibaba.com>
In-Reply-To: <95fca7d1-d473-e9e2-b6c8-c4ae3d44d2df@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Reinette,

On 2/16/23 5:43 AM, Reinette Chatre wrote:
> Hi Shawn,
> 
> On 1/15/2023 11:12 PM, Shawn Wang wrote:
>> When writing a task id to the "tasks" file in an rdtgroup,
>> rdtgroup_tasks_write() treats the pid as a number in the current pid
>> namespace. But when reading the "tasks" file, rdtgroup_tasks_show() shows
>> the list of global pids from the init namespace. If current pid namespace
>> is not the init namespace, pids in "tasks" will be confusing and incorrect.
>>
>> To be more robust, let the "tasks" file only show pids in the current pid
>> namespace.
>>
> 
> Is it possible to elaborate more on the use case that this is aiming to
> address? It is unexpected to me that resource management is approached from
> within a container. My expectation is that the resource management and monitoring
> is done from the host.

We have a scenario where we only want to mount the resctrl filesystem under a specific container.
And We found that the pids in the tasks under resctrl are inconsistent with the pids obtained by top.

Besides, current rdtgroup_move_task() uses the find_task_by_vpid() to get the real pid.
Our modification is also to maintain symmetry with the rdtgroup_move_task().

>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 5993da21d822..9e97ae24c159 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -718,11 +718,15 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>>   static void show_rdt_tasks(struct rdtgroup *r, struct seq_file *s)
>>   {
>>   	struct task_struct *p, *t;
>> +	pid_t pid;
>>   
>>   	rcu_read_lock();
>>   	for_each_process_thread(p, t) {
>> -		if (is_closid_match(t, r) || is_rmid_match(t, r))
>> -			seq_printf(s, "%d\n", t->pid);
>> +		if (is_closid_match(t, r) || is_rmid_match(t, r)) {
>> +			pid = task_pid_vnr(t);
>> +			if (pid)
>> +				seq_printf(s, "%d\n", pid);
>> +		}
>>   	}
>>   	rcu_read_unlock();
>>   }
> 
> This looks like it would solve the stated problem. Does it slow down
> reading a tasks file in a measurable way?

We didn't test it, but it is proportional to the number of pids in the group.
In addition, only an if statement is added here, and actually the reading of
the tasks interface will not be called frequently, so it will not be a bottleneck.

Thanks,
Shawn