[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <79ec0082-1813-77f2-f23e-5a10784c3212@intel.com>
Date: Tue, 22 Nov 2022 13:13:28 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: Peter Newman <peternewman@...gle.com>
CC: <fenghua.yu@...el.com>, <bp@...en8.de>, <derkling@...gle.com>,
<eranian@...gle.com>, <hpa@...or.com>, <james.morse@....com>,
<jannh@...gle.com>, <kpsingh@...gle.com>,
<linux-kernel@...r.kernel.org>, <mingo@...hat.com>,
<tglx@...utronix.de>, <x86@...nel.org>
Subject: Re: [PATCH v3 2/2] x86/resctrl: update task closid/rmid with
task_call_func()
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@...el.com> 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
Powered by blists - more mailing lists