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: <cdcfcd64-c76f-0d2d-6653-0229c956f2bc@intel.com>
Date:   Fri, 9 Dec 2022 15:54:28 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.5.1
Subject: Re: [PATCH v4 1/2] x86/resctrl: Update task closid/rmid with
 task_call_func()
To:     Peter Newman <peternewman@google.com>
CC:     <bp@alien8.de>, <derkling@google.com>, <eranian@google.com>,
        <fenghua.yu@intel.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: <e28c1f27-f320-511b-e5ea-c278a570d709@intel.com>
 <20221208223059.4086209-1-peternewman@google.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20221208223059.4086209-1-peternewman@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YngwaHUra2VabnVXdFhHZHJjY2UrYWZQQUNsZ2p5Q1ppTXpSUU5FWmFkb1Nz?=
 =?utf-8?B?TFo1ZGwzclMrNHNVb0dxTmZDalVCb2U4VFdtN2NUYldzY2VtSW1HWWZvdERL?=
 =?utf-8?B?Q3pMWTVYdDI4V1Jwc0wxMkJuK01pTW51YktqSXh6Smc1Q3V2bjdpWTVSbnEy?=
 =?utf-8?B?UkpnRUl6aWxnbU1ndVU2ZGtNVk1ody9mSzdhcHZwNGJwY002MTRWenU3N0RV?=
 =?utf-8?B?bG5ISzI4aW14MEhmTmdFWmV5cnVKNHJqeTRIeXlSQlZGSG9EWlpkMm5rNnBQ?=
 =?utf-8?B?T1FsdDR5dEtQbTNDckVSY01QekxLbHpVZnJ0MkFTeFRwalVkeWZzMWMwVHVj?=
 =?utf-8?B?L014UDdSdk83T1djUE9iaDlsc3BaMFYxTzFUc3RSMjJDMmpmZkFLd1dtZkZL?=
 =?utf-8?B?N21iYjJ2d0ZlbmJwMXhOQUtOYXRiUmJxZHlFOG5yYnN5bG13MGVWTXFxMmNy?=
 =?utf-8?B?bVZvT0gxSEFEUU5IbEx0M0RIYUh5QTFWVkhYcHZGM2hzZHo1OU9SUGdRbmE4?=
 =?utf-8?B?UXY0OHh0Q3ZkNGxXcy9KUVVuQysrcXhtWUJBWkJiaEF6WEJaN0NBUkdqczJJ?=
 =?utf-8?B?MUt3Q1hGbTdSazR6MVkzaTN1T1pIUWlPYndKYk8zQWdyMExlbE1STERQbk9D?=
 =?utf-8?B?NUllRHFPTmtHYmloY1lhWVFkdzVYSVJLa2ZlT0VDRVh4eGN3ODlSa0p4N0pG?=
 =?utf-8?B?NzNWVndjNE9Sa0FBS25Sd0R5SldVNDhRZmlCL1F6WVVDVHptZHlsanNLL1hi?=
 =?utf-8?B?QnhOYVdMR21pMmRsQXp0blRNcnIwSGh6c2thdHFjdTNqeFpKRnZRMThQTDcr?=
 =?utf-8?B?Z1Rnck1rOGVOaWtIYi9nVkQyb1d5TXBHelZoMjRrQmo1U3lnWDQ3aWR5T3Nj?=
 =?utf-8?B?Z2ZaeUN3R2FtOTJ4KzZjMTZnUzNMOWhrdVo3WjZVelBOaVZkcU9WSlFJR04x?=
 =?utf-8?B?eXd3WEtFTUlmTVdmM1ZvRGR6cVRkY00vWEFRZmpTVHg3MnVVb3FkT3ZUUzlQ?=
 =?utf-8?B?TVY0cWNMOFN2M1NpV1lmemFEMk5Nb0FXM2tENWlBNnVQb2ZNODQyWkwwbWhy?=
 =?utf-8?B?UWFSV29wL0oyc0QwRnYxOGd5RngydmIrbmREMm9VWDRlRHVxdXFDQ3JkNERy?=
 =?utf-8?B?bkhxUHpTY215TlZFaEV2b1puVFNqNzBXdHBSOWhic3pHcUFoSXR6Z2NKa282?=
 =?utf-8?B?MFF5eElkcHZsUFo1aWtQVVpTTlZxYmRPcDhVUU5xczYvbFR3bjkvZHhSMGtM?=
 =?utf-8?B?RktJWU1ndVVlR2M3UHBKbnM0QitqRDhEaWE5MHlOeG9kbFNienJNZEZNUXBo?=
 =?utf-8?B?Yit3TDFhZXhyVUZia2szVDBoTlpLVEZkZEZCQ3JNNmJVNFplNGwwQ3FxZTJp?=
 =?utf-8?B?ekJnRndlalhJd0ZnSE9hSWMxdnp0R2RaMXRZQU9vYmNTTnZvekRPZkZCYWor?=
 =?utf-8?B?Yi9RZEhRcGN6azFBQVB1cGZBY2NrdTJFWWl3SHdCQVp1d21MK1dRQ3UxWEZi?=
 =?utf-8?B?TVROMHJXdUc4RGFNUDg3VENSNGpLcCtEV3RCdlJJb1ErN0dHbFpWbUpwb0to?=
 =?utf-8?B?aFk2ejF0cEpnQ1E1bnlqTEp3MlM5b1NqblphYUtzbFIxaUl4N1FMbHBxSXFQ?=
 =?utf-8?B?UytpTklQTU1vVzdFdXVXeWdZMjAxNGhsUUh5WGRONVFsVjVLSWVFSENKSUlv?=
 =?utf-8?B?VlVtclpTYkU2OEQyTFAwOWpTSWZ4YXhjMTlwTEttdVRsWmg1OW5yZzJZOGlC?=
 =?utf-8?B?ZHYxRGVPL2F1MUwyaE1IS256cmR0eG1OaURpQVRZRGpwUU9EZnk2T1Y3OG42?=
 =?utf-8?B?NzUxdzlhOXlaWnFieUxWREI4UDVNcHJJMXlBb2c4QlN1eGMzY0svTlRqMVh3?=
 =?utf-8?B?dk84RmQ3Rk1yVVQvZnVDY1BldEF5NGlZU0NRSU0yMSsvblRWR3piV0Rydm1I?=
 =?utf-8?B?ZWFqaW9aMTZFa0J5K0ZyZEdsMmhQeis0czBremtPQ1AwVU5rK1pqTnBOMGtT?=
 =?utf-8?B?R0RaZERBbTR4S3FtQ3V4eWdaVWdnd1VieHBtMDBvUVVSQWFTdnQ3cVdaaU4v?=
 =?utf-8?B?Z21RWWUwT1d6VHFpN1FqRThUYi9yaTlPSEJqRVNmeDVBc2ZnWDFzMXNCN2Zk?=
 =?utf-8?B?dG12VUlReUtGdGFFRjZ6aFl2b1l0UGY0R1NVdTBXMWo4Y3ZVYUVGb3Zhemd3?=
 =?utf-8?B?ZUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 04fdc895-085a-4737-d046-08dada40b6e2
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2022 23:54:30.9926
 (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: /Rib65d79JtlRpBh71eqkHxCedPI2326v7YKLaIk7ieL6h5AqFIyamdh+nQPi/CRvTiqua3Id820+hgt6OIUTk2VB80DWt0fJQhRYbKi0ns=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7902
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 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 12/8/2022 2:30 PM, Peter Newman wrote:
> On Wed, Dec 7, 2022 at 7:41 PM Reinette Chatre <reinette.chatre@intel.com> wrote:
>> On 12/7/2022 2:58 AM, Peter Newman wrote:
>>>>>  2. resctrl_sched_in() loads t->{closid,rmid} before the calling context
>>>>>     switch stores new task_curr() and task_cpu() values.

...

> 
> Based on this, I'll just sketch out the first scenario below and drop
> (2) from the changelog. This also implies that the group update cases

ok, thank you for doing that analysis.

> can use a single smp_mb() to provide all the necessary ordering, because
> there's a full barrier on context switch for it to pair with, so I don't
> need to broadcast IPI anymore.  I don't know whether task_call_func() is

This is not clear to me because rdt_move_group_tasks() seems to have the
same code as shown below as vulnerable to re-ordering. Only difference
is that it uses the "//false" checks to set a bit in the cpumask for a
later IPI instead of an immediate IPI.

> faster than an smp_mb(). I'll take some measurements to see.
> 
> The presumed behavior is __rdtgroup_move_task() not seeing t1 running
> yet implies that it observes the updated values:
> 
> CPU 0                                   CPU 1
> -----                                   -----
> (t1->{closid,rmid} -> {1,1})            (rq->curr -> t0)
> 
> __rdtgroup_move_task():
>   t1->{closid,rmid} <- {2,2}
>   curr <- t1->cpu->rq->curr
>                                         __schedule():
>                                           rq->curr <- t1
>                                         resctrl_sched_in():
>                                           t1->{closid,rmid} -> {2,2}
>   if (curr == t1) // false
>     IPI(t1->cpu)

I understand that the test is false when it may be expected to be true, but
there does not seem to be a problem because of that. t1 was scheduled in with
the correct CLOSID/RMID and its CPU did not get an unnecessary IPI.

> In (1), CPU 0 is allowed to store {closid,rmid} after reading whether t1
> is current:
> 
> CPU 0                                   CPU 1
> -----                                   -----
> __rdtgroup_move_task():
>   curr <- t1->cpu->rq->curr
>                                         __schedule():
>                                           rq->curr <- t1
>                                         resctrl_sched_in():
>                                           t1->{closid,rmid} -> {1,1}
>   t1->{closid,rmid} <- {2,2}
>   if (curr == t1) // false
>    IPI(t1->cpu)

Yes, this I understand to be the problematic scenario.
 
> Please let me know if these diagrams clarify things.

They do, thank you very much.

Reinette