Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <79ec0082-1813-77f2-f23e-5a10784c3212@intel.com>
Date:   Tue, 22 Nov 2022 13:13:28 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.5.0
Subject: Re: [PATCH v3 2/2] x86/resctrl: update task closid/rmid with
 task_call_func()
Content-Language: en-US
To:     Peter Newman <peternewman@google.com>
CC:     <fenghua.yu@intel.com>, <bp@alien8.de>, <derkling@google.com>,
        <eranian@google.com>, <hpa@zytor.com>, <james.morse@arm.com>,
        <jannh@google.com>, <kpsingh@google.com>,
        <linux-kernel@vger.kernel.org>, <mingo@redhat.com>,
        <tglx@linutronix.de>, <x86@kernel.org>
References: <20221115141953.816851-1-peternewman@google.com>
 <20221115141953.816851-3-peternewman@google.com>
 <85e9d499-5393-f388-7d23-aa68248e5037@intel.com>
 <CALPaoCjiDw5WaQG+yJHgKr9=WrNw606Z8oDwt47Yx+cWZBf6eA@mail.gmail.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <CALPaoCjiDw5WaQG+yJHgKr9=WrNw606Z8oDwt47Yx+cWZBf6eA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UGVvS2xlSVdaa3hmTWVGbFhTR0RFL1JwQXM5c0EzQ2xiYUxUdHRUMWJ2ZTFB?=
 =?utf-8?B?SUVMWVlOaFlwNEtsblJnTmlSdSswNjdGNDNSVSt6cnQzbkUydHVLS29SZjdj?=
 =?utf-8?B?alBmSHl2bWRENkRnR1d5TmhkUHJxWitLejh6L2RyUHpZNlRVM1luaDA5Zyt4?=
 =?utf-8?B?Y00vTDZIako3Ui9VZEdkQndybVYwbGYxSm9QckYxRWZHcHJTTjBTSkdqTzBM?=
 =?utf-8?B?QUdydWdRM2VRL2hOT25ZbjdCWWtLbTJEVXlicDlES1lKSWFJY2lCeU9LM2Ji?=
 =?utf-8?B?L2JNQVFhVWZhUVErbm5wOFRKOXAxQXBHTmlzMDhnN0ZaazRVY0liRDZFYzZm?=
 =?utf-8?B?eHJuZXZNd09nbzNZeVZYQWNSU1JVSGVOSmpKOHlVZnJSalBST3ZaaUU1Z3hZ?=
 =?utf-8?B?Z2pzS3hDTERHb0FrNnZVL2U1NjByVWpYUURLZk4vczZaSTl4RDFvUUJMZ0Uw?=
 =?utf-8?B?UUowTzdlS0x1Qm1OVGNXVytJcUp6YmJ5K251bHN6ZjVQdjV1SEdZZk9RK0I3?=
 =?utf-8?B?VERibmJEUjkrTkRBMmFJMWVmMFlRL3QzSm1SMCtVT0JwenNyWTNXU3J0bnBz?=
 =?utf-8?B?STF1RlZxNHJkVFYzMDFYbTNUYWNaMXRkV2JrbjAxSkgweVk1d2diOHVWYVkr?=
 =?utf-8?B?Mzd1OGpBS2w0Uk44T3A5bC9BeDluQ216L0lOVmZaWkVZeWdrSGhwMlNyYzNv?=
 =?utf-8?B?akd6MzVDV1F5VDRwSTROZ25OU0pMVzlQYW1ZNWFuSjRuOGU2Z1FFWlRyczRr?=
 =?utf-8?B?ZlU0VzNIcEhqMzhVUGVKSGVoNHV6eVZPSGJpNTFCS3ZsWWNvcmlKNFhQdzZu?=
 =?utf-8?B?aWd3ZHJmcjVOTmMvODI2U3k1VTlMZ1k3Wk1qYzcrZHg5b0JHRXBvMHBkMFdD?=
 =?utf-8?B?a2lnZHBKU1Fac3VZV2h4ZVVwYXpjRVQ3WGpKZ3I3MCt6dkVPY2RyTTNCZGNj?=
 =?utf-8?B?eG9GSTRMZ3p5SGpueEhFYUFBaXRvZzJENlpwN1NEdjVGSGJWRUxJYXZ6ZFI2?=
 =?utf-8?B?eWQ4ZEkwVjR2alZkSjU0MGUxMExOdHZrVFRXa2hxcm9KUWd0cXAybWNXOVEw?=
 =?utf-8?B?UHhyNVB6RkJ2MVZxNUNuYkNvdHJzR2cxYXhnc3QvUkhCa29xZWVuQUhNRk1U?=
 =?utf-8?B?dklRT0hKU1VHUnZJdU1WWG5YcDkxRENSWU05RDRaaHlEMTV5VkRzLzlpWkV1?=
 =?utf-8?B?Um0zUkpTcU1LeXMxUk9pSFpmYm5FV3J0dlluWHVMMGIvOUFwYmk0VEpwZlE0?=
 =?utf-8?B?VHgyaHp4aTNTU21FeHJaejZFRnhnUGkvNlVLbzUxbVhDTzFVdVhvWVpzS3lj?=
 =?utf-8?B?UXI1N2tSVjlMaUcvcGFGYjJoTmorSUk3RmlwTG8vQ2RpcXJkYlp6QitEeTZy?=
 =?utf-8?B?ZzV4Vy83UzRCckZNTExCUXlaWmdUSmtmd0VGcUcwUzRJNjNnbDA4MGFwYXFa?=
 =?utf-8?B?ZlJZUTlzT2p6YUEwTEdVUmJUQzhRSDlBOTJ0VnY0SnZ1R1hyWlpHeEF4VERi?=
 =?utf-8?B?ZHh3eHNaazNqNmEydjdZM2hvRzJpQ1JQcTZ0dC90K0RrQ1dxc1hLUEE2Qkpl?=
 =?utf-8?B?Z1J3VGJCcU9uY2dqVUgwSU1hOEtOUzBVZWxBQXN6MzllOWRWcDVVMXFreXhN?=
 =?utf-8?B?MWJ3d1RJWG9odlRPL2s4M1ZWeTg4N1F0UHZQazZYb09Qc2ZINEhiRkFmQUpC?=
 =?utf-8?B?NWZ3Q2FReVdrYk4wZUJlbTJzbGVMSHRGVTkrQVNKY0pYZ1JIRDRveG0vNTNH?=
 =?utf-8?B?d3FDM2RYWGozbnVsNkpadmMvWTBQaEpDMFJ4ZGdDLzNpUmR3OE1YbmZvc25C?=
 =?utf-8?B?NkxGVTArWDFzeXlmZU5VQzBoT1BROXVtcHgrNk42WFVhT3c4UURmSkxtVkFr?=
 =?utf-8?B?V1dnd0xhYm5FYUNoQXlDTHdSUm5PU2h0YmtndzlaeEhtaWRSaHFMRE5jL0h4?=
 =?utf-8?B?K3lnMGZxUm5LR2d4ZVVHTDFqdy9TVlN2eHVqT25SL1pyUnErTXhNN1NqWVJP?=
 =?utf-8?B?NmN6TnFhL3JIS0JBaXpUK3lIWDZpMVVrdVpQZ25tb0J2bVI1U1FFbDRJK1dM?=
 =?utf-8?B?WW9tazBHcXZqL1NFSUJZOXdEaEgyOGpHTGZlT1hmNXc0VnpMZUhqTi80bVRG?=
 =?utf-8?B?RU1kTXZiemJTQnZrVHdSMjI4VE9UWmkwOHRPd1A3eEpoVGsrMFF1c3BxSUxS?=
 =?utf-8?B?QkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 34a42b5e-e4dc-4a31-0923-08daccce687f
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Nov 2022 21:13:32.6705
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: vmh2JDuNKfm86IUOFV3AJr5dOwFhFdg3q9f2wrBgMjVfRoBDnA7l1ZrY40D9pTlwoU7P0hU7I7N752Xmst2foJXqknqb47XP2jJUGmNwPI0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6116
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@intel.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