Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <46d61a99-bb8a-a558-809a-60b085f33d6c@intel.com>
Date:   Mon, 4 Apr 2022 13:43:14 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.7.0
Subject: Re: [PATCH v3 07/21] x86/resctrl: Create mba_sc configuration in the
 rdt_domain
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        Jamie Iles <jamie@nuviainc.com>,
        "D Scott Phillips OS" <scott@os.amperecomputing.com>,
        <lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
        <tan.shaopeng@fujitsu.com>
References: <20220217182110.7176-1-james.morse@arm.com>
 <20220217182110.7176-8-james.morse@arm.com>
 <01651414-9d4a-409d-9db7-b4b6dde72829@intel.com>
 <d49bdfad-df5e-77e1-4834-266dcd1b9055@arm.com>
 <91a43681-524e-c12d-612d-259e51bde12c@intel.com>
 <f5f8166e-1831-1fa0-123c-53d3632451a8@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <f5f8166e-1831-1fa0-123c-53d3632451a8@arm.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?VUp0MVE2V0JFMFlXaVFFcVpPcU52WjlNdEFOZkF1ajNZQ1VwUWc0eVd0MFpi?=
 =?utf-8?B?YnVQSWhic3YxRTNSL05CU3A0azNwZllQT2RWa1VnMW0xUGJNeE5vSTcwV2xR?=
 =?utf-8?B?dkk1cDJCQnQzdkw1R2pJQm9IUUp3VkZDRjY1bGUyY3ZCeG1KdEttQlk2elND?=
 =?utf-8?B?UTNrZXcxV0ovU25ZdnRPWG5paVdxRU5wMHBuQ2kvNGZHQTRNajhYRU9FOVJC?=
 =?utf-8?B?YnNMb3hZUzNQaENmTTZQNGM5NHZ6TzhRWUg3QzBGYnNxeFNYWFNtcUh2QnlQ?=
 =?utf-8?B?OTVDWE5IenhDOGo2Y1RiM05sT2lCck80eUZtakNCWnhtYW02SzJHQ3ZyRFlI?=
 =?utf-8?B?RkxMci9LRjV4NSt2SW5ucFpOdTgyd0JIUEJVQy95RlhzSGVNRjNYZHJqcEZ1?=
 =?utf-8?B?WUkzUW11MFNGQkl4M3p1d0R1L0w2bzBGNUZ0cmtxNWp3NG5lUm83SEQrWjVH?=
 =?utf-8?B?RG9KMUwvSEZIZTlrQUhpM1M3ckNmV1dLTWlvamJ2K1NVVUxTSzZscVhvM0x0?=
 =?utf-8?B?c2lGVFVrUkhoY1JaNVlsUlI1WVRTaWE0bG5MMFVWVldzTFl1MytCczR6K3RF?=
 =?utf-8?B?cEQxKzU2ZFdWT2JheE51Z1BYOGdYaGRNMFdPelN6cy8yMkV1Szc5a2x3OTFz?=
 =?utf-8?B?eVhlb2tmeEdaV294eEFKZVQrMHhHVEtqKzdpUStlRVhHdXJoSHBpL0twcTB1?=
 =?utf-8?B?S2QyYXZmc3JHQ2RzTEZVTktHZS80NWRiUFkxNnVPV3BiRDBHZlBoMStFN2VM?=
 =?utf-8?B?WE9Nb0Rkakl5TWJLWWVtbXRBWnViTWVVQ3V4bVMvVTB6NkpoWWRHb1lCT25w?=
 =?utf-8?B?djRad3RSeXNrcVBObTYxUURES1JXRWFONGZNbDIxeXRIRjJ0MXFNejM0MU9O?=
 =?utf-8?B?RnpLdmZQWFo5SjJncHZHSmJ4TEdRZnRsZGVobnNST2VlNWpZR2JNLzUreEI5?=
 =?utf-8?B?OE5FVnBQbVV2eGFHNUdwbThtWTNzeDEyWVRaL3NyTFExM0JkbTh2R1hKOEtU?=
 =?utf-8?B?eHRIWTFYeVc4bzJNb3V3YzR5VFBUZzkwUWtOWXU4dzgrYmFJSGkxRzR1aFV4?=
 =?utf-8?B?NUd1ZGQ3MFdSU0hzNlRXQy8wVlFrVkQ3QWVkNFFic3BqUHpDZnRuODJmRStj?=
 =?utf-8?B?K0RlNSt4U0t5dXoxMkRXU004Yk54TEFSRE5PRXJkdFdUU2RjcXBuWW5PWkFq?=
 =?utf-8?B?aTN1anI3T04xSEpzazZocTVQYVR4UVZDYkFXVEJtb2QvN1FMdDllVHQ0ZDE1?=
 =?utf-8?B?M0RHcDZ1cmlPR1RSZGtRbmw4ck1iQi9BaVRVMlpYVXFTTTlCVm1SVTV0K2F2?=
 =?utf-8?B?cExFU3lmOHovVXJMOTlYOWlLK1NYeU10OWpxaDk2QTduRWRGWEkzL2Yyb3lo?=
 =?utf-8?B?RktoTFVieTZmVE4vTFV3OHNMaVJhRENUMFFWWWFnZGNDSFhKODF1djJScmFt?=
 =?utf-8?B?NzZQMzdWWVdwOGNiQ2JvV2VodzRxOG1yVG10V1lSNmdkenhvczRDaUU2dTZI?=
 =?utf-8?B?eGtlSjRVenI3aW1qUitkRlBwSSswbjhpalF3djRGaG1KSE1qT00vZ1lSZHNU?=
 =?utf-8?B?N0lyR29WZm5tOHNHdGg2ekRLM2VLY3E5MXlBR05maVJ5c1EzZytXMkZBc1Ri?=
 =?utf-8?B?RURLKzhwUzgwbmF1MGJWa0xrYzJXVytVRFlVVkZNSUVuN1FieHArMjB1Y01p?=
 =?utf-8?B?Z2RoT3o4N1hBSFRRY1VpRU1KUWhGb2FNOCtyOWtCeEhnbXFORWRxdmxzZFp6?=
 =?utf-8?B?QXZMWTA2WDNmYmd0WnZhMmp0S0lKUUc2SDBjTWJHeWZESUljMWVzNGNJTm5J?=
 =?utf-8?B?OS9aR20yVDVraEZZa1BaekVBRDE2dWdMbjBJSjlwanJLUVZDOWFUSms0bFlT?=
 =?utf-8?B?WWYwZGNQZis1ZHFtVml1N255Y1F3eDMvVWhiSGNsR1h5VzBybEtaZk5wczlL?=
 =?utf-8?B?aXVMbzBmblllcTlQQ2N2WU5vSTVjci83OXlaR2s2bFR5aDRmbThOWU93T2l0?=
 =?utf-8?B?aktNWWZjYlNxV2k3Q0JkVnl0NnBvZU8zbDJ4a05odStMdXBOcmZaT1lsa3Uz?=
 =?utf-8?B?eTkzQU5sRmwwS0RjRVozUXBlUFl4blZodlJ2d0dzbkR2eWxYQ3dyRGJrZGox?=
 =?utf-8?B?MndJYVNXeVh5ZEN6NStSWUplVGxKbjFzd3FjU1ZtQ01JdHB4Z0ZVQlNycCtk?=
 =?utf-8?B?aFlsWlBuSDV5TU9FQmxDY0ZMM0NEaWt2NThOZnNaQUZoWS9abFpWbXY1Wi9B?=
 =?utf-8?B?UVhsZnNxVUQzeGx2T01UbnRFT3Jtd255VVMvaElVV3VRZ3EyajU1RUNnaHAr?=
 =?utf-8?B?Z1hWdm1TTXBHK2VLZWRnZ2s2M2x4OUl3eGp2b0dja2FsMjArbm5tcWJ6MVh0?=
 =?utf-8?Q?LzPak13kRDVeHhLs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 51f28da5-688b-4df8-b20d-08da167bc040
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2022 20:43:19.1099
 (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: eLPz2bbgqBhJ940nLzrmPiTgXfJQRKacg+BetNjJN12NPqYDI6f9Pz3GObrXM5k7G15UjR4g+3kysk61JeaCTa8boyrZQW6YYmS+VVhWKmM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4566
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 James,

On 4/4/2022 9:35 AM, James Morse wrote:
> Hi Reinette,
> 
> On 4/1/22 23:54, Reinette Chatre wrote:
>> On 3/30/2022 9:43 AM, James Morse wrote:
>>> On 16/03/2022 21:50, Reinette Chatre wrote:
>>>> On 2/17/2022 10:20 AM, James Morse wrote:
>>>>> To support resctrl's MBA software controller, the architecture must provide
>>>>> a second configuration array to hold the mbps_val[] from user-space.
>>>>>
>>>>> This complicates the interface between the architecture specific code and
>>>>> the filesystem portions of resctrl that will move to /fs/, to allow
>>>>> multiple architectures to support resctrl.
>>>>>
>>>>> Make the filesystem parts of resctrl create an array for the mba_sc
>>>>> values when is_mba_sc() is set to true. The software controller
>>>>> can be changed to use this, allowing the architecture code to only
>>>>> consider the values configured in hardware.
> 
> [...]
> 
>>>> Considering that no domain belonging to RDT_RESOURCE_MBA will have this array this
>>>> always ends up being a null pointer de-reference.
>>>
>>> Ugh. I'm not sure how I managed to miss that. Thanks for debugging it!
>>>
>>> That loop was added to reset the array when the filesystem is mounted, as it may hold
>>> stale values from a previous mount of the filesystem. Its currently done by
>>> reset_all_ctrls(), but that function should really belong to the architecture code.
>>>
>>> Because mbm_handle_overflow() always passes a domain from the L3 to update_mba_bw(), I
>>> think the cleanest thing to do is move the reset to a helper that always operates on the
>>> L3 array. (and leave some breadcrumbs in the comments).
> 
>> I think this points to more than a need to reset the correct array on mount/unmount ... or
>> perhaps I am not understanding this correctly?
>>
>> As the analysis above shows the mbps_val array only exists for rdt_domains associated
>> with RDT_RESOURCE_L3 but yet mbps_val will contain the MB value provided by user space
>> associated with RDT_RESOURCE_MBA.
> 
> I've finally got my head round what is going on here: I've muddled up whether mon_capable
> is a resource or system property. mba_sc depends on the L3 being mon_capable, but the
> configuration should be associated with MBA (wherever that is).
> (basically ignore my previous reply!)
> 
> The creation of the mbps_val[] should depend on supports_mba_mbps(), which uses
> is_mbm_enabled() to check whether the L3 is mon_capable. I'll check the rid too
> to make it clear its only MBA that has this.
> The call to allocate the domain in resctrl_online_domain() should be above the mon_capable
> check. (which is still needed to avoid the work guarded by is_mbm_enabled() and friends
> running for each domain).
> 
> 
> Thanks,
> 
> James
> 
> -----------------------%<-----------------------
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e3c90f33baf2..ad0411eb2147 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3345,6 +3345,14 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  
>         lockdep_assert_held(&rdtgroup_mutex);
>  
> +       if (is_mbm_enabled() && r->rid == RDT_RESOURCE_MBA) {
> +               err = mba_sc_domain_allocate(r, d);
> +               if (err) {
> +                       domain_destroy_mon_state(d);
> +                       return err;
> +               }
> +       }
> +
>         if (!r->mon_capable)
>                 return 0;
>  
> @@ -3352,12 +3360,6 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>         if (err)
>                 return err;
>  
> -       err = mba_sc_domain_allocate(r, d);
> -       if (err) {
> -               domain_destroy_mon_state(d);
> -               return err;
> -       }
> -
>         if (is_mbm_enabled()) {
>                 INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
>                 mbm_setup_overflow_handler(d, MBM_OVERFLOW_INTERVAL);
> -----------------------%<-----------------------

Thank you. Having mbps_val be part of RDT_RESOURCE_MBA makes things clear to me.

In the snippet you provide you may need to check the error path cleanup, I do not think
domain_destroy_mon_state(d) would be needed after moving the allocation earlier.

Reinette