Re: [PATCH v3 2/2] x86/resctrl: update task closid/rmid with task_call_func()

[Reinette Chatre]

Hi Peter,

On 11/22/2022 7:17 AM, Peter Newman wrote:
> Hi Reinette,
> 
> On Mon, Nov 21, 2022 at 10:59 PM Reinette Chatre
> <reinette.chatre@xxxxxxxxx> wrote:
>> Patch description in subject should start with upper case.
>>
>> On 11/15/2022 6:19 AM, Peter Newman wrote:
>>> When determining whether running tasks need to be interrupted due to a
>>> closid/rmid change, it was possible for the task in question to migrate
>>> or wake up concurrently without observing the updated values.
>>
>> Mixing tenses can quickly become confusing. Please stick to imperative tone.
> 
> Sorry about this. Looking at how other bug fix commit messages are
> worded, they describe the bug as the present behavior of the kernel
> rather than how the kernel used to behave before the patch was applied.
> It seems I've been consistently wording problem statements as past
> behavior.

No problem. There is a good section in the tip handbook about changelogs. 

>>> +static void update_task_closid_rmid(struct task_struct *t,
>>> +                                 struct rdtgroup *rdtgrp)
>>> +{
>>> +     /*
>>> +      * Serialize the closid and rmid update with context switch. If this
>>> +      * function indicates that the task was running, then it needs to be
>>
>> What does "this function" refer to? Please replace with function name to be
>> specific since there are a few functions below.
> 
> Ok.
> 
>>
>> /was running/is running/?
> 
> The task is no longer locked here, so it doesn't seem correct to say "is
> running" when it could have already stopped running.

ok, how about "was running during its closid and rmid change" or similar?

> 
> Also, maybe related, I was considering moving the task_curr() call out
> of update_locked_task_closid_rmid() to avoid giving the misconception
> that the result is current, because the value is used after releasing
> the task's pi/rq lock.

I think task_curr() should stay within update_locked_task_closid_rmid(). It
is an appropriate usage per description of task_call_func() and from what I
understand it is the most accurate way to answer the question "was task
running while its closid/rmid changed?". It cannot be guaranteed that the
task is still running when the MSR changing code gets its change to run but
_update_task_closid_rmid() as well as resctrl_sched_in() have that covered.

Reinette