Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   KeMeng Shi <shikemeng@huawei.com>
To:     <will@kernel.org>
CC:     <akpm@linux-foundation.org>, <james.morris@microsoft.com>,
        <gregkh@linuxfoundation.org>, <mortonm@chromium.org>,
        <will.deacon@arm.com>, <kristina.martsenko@arm.com>,
        <yuehaibing@huawei.com>, <malat@debian.org>,
        <j.neuschaefer@gmx.net>, <linux-kernel@vger.kernel.org>
Subject: Re: Re: Re: [PATCH] connector: report comm change event when modifying /proc/pid/task/tid/comm
Date:   Wed, 18 Sep 2019 22:43:21 -0400
Message-ID: <20190919024321.2675-1-shikemeng@huawei.com>
In-Reply-To: <20190917170737.dpchgliux4qi2qef@willie-the-truck>
References: <20190917170737.dpchgliux4qi2qef@willie-the-truck>
MIME-Version: 1.0
Content-Transfer-Encoding: 7BIT
Content-Type:   text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2019/9/18 at 1:08, Will Deacon wrote:
>On Tue, Sep 17, 2019 at 09:56:28AM -0400, KeMeng Shi wrote:
>>on 2019/9/17 at 5:10, Will Deacon wrote:
>>>The rough idea looks ok to me but I have two concerns:
>>>
>>>  (1) This looks like it will be visible to userspace, and this changes
>>>      the behaviour after ~8 years of not reporting this event.
>>This do bother for users who only care the comm change via prctl, but 
>>it also benefits users who want all comm changes. Maybe the best way 
>>is add something like config or switch to meet the both conditions 
>>above. In my opinion, users cares comm change event rather than how it
>>change.
>
>I was really just looking for some intuition as to how this event is currently
>used and why extending it like this is unlikely to break those existing users.

By listening these comm change events, user is able to monitor and control
specific threads that they are interested. For instance, a process control
daemon listening to proc connector and following comm value policies can
place specific threads to assigned cgroup partitions (quota from commit
 f786ecba415888 ("connector: add comm change event report to proc
 connector")).  It's harmless as user ignore the threads with names that
they are not interested.

>>>(2) What prevents proc_comm_connector(p) running concurrently with 
>>>itself  via the prctl()? The locking seems to be confined to set_task_comm().
>>To be honest, I did not consider the concurrence problem at beginning. 
>>And some comm change events may lost or are reported repeatly as follows:
>>set name via procfs	set name via prctl
>>set_task_comm
>>			set_task_comm
>>proc_comm_connector
>>			proc_comm_connector
>>Comm change event belong to procfs losts and the fresh comm change 
>>belong to prctl is reported twice. Actually, there is also concurrence 
>>problem without this update as follows:
>>set name via procfs	set name via prctl
>>			set_task_comm
>>set_task_comm
>>			proc_comm_connector
>>Comm change event from procfs is reported instead of prctl, this may 
>>bothers user who only care the comm change via prctl.
>
>Perhaps, although given that proc_comm_connector() is currently only
>called on the prctl() path, then it does at least provide the comm
>from the most recent prctl() invocation. With your path, the calls
>can go out of order, so I think that probably needs fixing.

It's indeed necessary to fix the concurrence problem. I will submit
a v2 patch when I fix this.

Thanks for your review and advise.
KeMeng Shi