Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <38421428-84cb-b67e-f3ce-b7a0233e016b@amd.com>
Date:   Thu, 7 Dec 2023 17:34:19 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH 00/15] x86/resctrl : Support AMD QoS RMID Pinning feature
Content-Language: en-US
To:     Reinette Chatre <reinette.chatre@intel.com>, babu.moger@amd.com,
        corbet@lwn.net, fenghua.yu@intel.com, tglx@linutronix.de,
        mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
        James Morse <james.morse@arm.com>
Cc:     x86@kernel.org, hpa@zytor.com, paulmck@kernel.org,
        rdunlap@infradead.org, tj@kernel.org, peterz@infradead.org,
        seanjc@google.com, kim.phillips@amd.com, jmattson@google.com,
        ilpo.jarvinen@linux.intel.com, jithu.joseph@intel.com,
        kan.liang@linux.intel.com, nikunj@amd.com,
        daniel.sneddon@linux.intel.com, pbonzini@redhat.com,
        rick.p.edgecombe@intel.com, rppt@kernel.org,
        maciej.wieczor-retman@intel.com, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, eranian@google.com,
        peternewman@google.com, dhagiani@amd.com
References: <20231201005720.235639-1-babu.moger@amd.com>
 <d97cbeba-af6d-4b64-b5c8-32dc437a67b6@intel.com>
 <71e85bf3-a451-4adf-ad5e-d39f7935efa0@amd.com>
 <e36699cf-c73e-401b-b770-63eba708df38@intel.com>
 <5ce67d8f-e207-4029-8fb3-0bc7deab1e9f@amd.com>
 <390bbb7b-e709-4290-9bef-eed373e469b2@intel.com>
 <a6338e5d-ab94-8da8-3127-3ce90997bf65@amd.com>
 <c1ccdfa6-ff8a-4602-bb06-f49bf34629ff@intel.com>
From:   "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <c1ccdfa6-ff8a-4602-bb06-f49bf34629ff@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VGhTR0RqYnhkYncrcWd5b0JXTHJsU1R6bThGQ3BwZXFCWTZyNXlmSGtHV29R?=
 =?utf-8?B?RGRuV09uK1Q4SjgyYlBaQjBKVlA2N0orTkJ6N0tQVTdVdjBERkd6bDJTd3VR?=
 =?utf-8?B?VGFxQk1QSGdFcHMzSkF5MUw5QWs1bnp0WHBxdmhXTmw3M1EwSUZ3bWNMTjVv?=
 =?utf-8?B?UzJkR3R1VmwvM0NXYlRVY0RwRlFzTTVNSWhEVUd5cmdUeElnTis1eWxkL0E4?=
 =?utf-8?B?cTl0aUZFbXFJcFg4QXpXZHNsSUIzNFBSRGZDcVRMLzhnMDdUa0cwQjc3ckVI?=
 =?utf-8?B?Z3Z5aXJtQ2pOMDRnZ3Bzd3ZUcm5OZGh6R2w4RGUvYndYU1gxS1hzaU14bFhE?=
 =?utf-8?B?R1hzQkVQb2xtU0pKOUU5YzNqeldoZDU5VnhqaHdpWnpkL3ZqeGNhVnpNVURl?=
 =?utf-8?B?R0VvK1NiVTRBeFBEZWdpbm5PSWgvUkV0REhZM0JscDNFOHZQblByc1MrZWZL?=
 =?utf-8?B?c0F1Tmw2b0lraG5PL25HYzE4QXhLVm9iT1FUMEw3SFhYUWM5Mzc5SXRFbnht?=
 =?utf-8?B?NXBtV2lzQ2dDdTZyNEJKYmUrbFhEQ2U2N013RFZ5SkJwZExsQnNkZ0M0ME8x?=
 =?utf-8?B?WnAwd01IOCtYQlg4ZjZjWmRhbENkb043WEhVT3k0cGJuZ2VtVHJRalBKNWJF?=
 =?utf-8?B?SzVGa2hhNGhuSXlGUyswUFJJbnlsN1RGL0hRbW9NUGhSU25PbDRXTGZ1cXph?=
 =?utf-8?B?eWV3OWdGY29ZR3ZJNy9vR2xWaFJ2a1ZlZTFQT3Rzc09ubEt0STREZmpVVm9D?=
 =?utf-8?B?NklENVN5YnhpZ2Y5cDM4NkE4RXBYb1o2NFRxc1dMVk1MNlA4YjBONzBKS2Jk?=
 =?utf-8?B?b0xuTkZ0aHJsY2tNTzBJR1BXMTgwcHNoSng1QXJNelh6QkdDeFlsOW55cTNu?=
 =?utf-8?B?YUU0YlN4Z0llbzA2Tmt0bEJ0dlN4dnhYc0NnS2lxNHVEZFpuOFVneFF3QlJP?=
 =?utf-8?B?b0FPQzFBR0Zyc2Y4Qnc3TTNud0NVZlRXUU9wTWpFb3haV0tMWEJ6YkRtbzFl?=
 =?utf-8?B?Q0ZjM3MzTG1OTFJWd2xKKy93WVpPMWtCeVYzNUJ3QzVvYVRQUmYzd0hCYUhK?=
 =?utf-8?B?Q2RUbUJvUXY3aURtK2lsc01MQy9ub2k5N3VWVmp0WmhSRTJxZ1FUaFRwRGFU?=
 =?utf-8?B?VDNQenNhZVR1aTYrSnpUVS9xNFJHMlkrL1RnQ052VkdZSHFhVFZ4aEtkSHQ2?=
 =?utf-8?B?SGJJTnhjbmxtMytsU1pwMTRwVS91d0Y3a040alhDSDZyMnVQK25heWl0aktY?=
 =?utf-8?B?bXkxOG96Z0VWV3hqUXhCM1VmZDRSTlBQQlZFZkM4TjlDS0Q0aml4VG83OWI1?=
 =?utf-8?B?aHFjeUtYZnByaEhMQVBtc0VqR1Y1R0EvT3NNMWZ4NnJoOU1SV0tmamE1QzhT?=
 =?utf-8?B?Z0lmZWVMRUpKanFyMjlQcXdDQVhnNVRKRnBicDFWTmJ2UnJ6ZisrdzluOHF6?=
 =?utf-8?B?eDNOeTh1NWxKZnNFZGd0c0NWWGc5RWVOdURHbHF6b3V5cHhsTUI3UHZXV3lk?=
 =?utf-8?B?S1d1eUxiTDcxZ3ZDeXNsQlNRaTFBK2I1aHJUc1ArQjQ1Ym9EYS9DeHk0M20v?=
 =?utf-8?B?QXdqQ2VCcVhvc1dETWRyTWhqUzMxbktyRnhwQVFVWUlOZ0FKeTIyRWh5MFRP?=
 =?utf-8?B?cW53WDI0V1N5UHVJUjJvSnBYUm05RzBCM2JxMEIybTZEVlgrcUZXLzB0ZGt6?=
 =?utf-8?B?ZHVGKzUwNFljbWtwSkJBdCs3L1pBejNKVXhVb3lxcFJ5dFRXeWMyRy82SDlS?=
 =?utf-8?B?OWx2TjNtYlZHOW9lek5yZ0dnSjZySmdQNG1Td0ZJVlgxVVduTmhEdmJoT1M0?=
 =?utf-8?B?K0lmK3dhL0hhWk9KTVNvMTdmdjVsWDJRQk5ZSkEvSGZ3Z2lHTzllazY5TGdD?=
 =?utf-8?B?UUU5dFVkbXcvSFhOMkpPOTA1bFI5UkxRSEh1ZXhPY1QzWWUyczE3akVlS3gr?=
 =?utf-8?B?bVJPem1jVTYxQzE1cE1XTzdNOTA5S1k2OXdBNzZZSDN6Q0M2N0hHRnFnSXpJ?=
 =?utf-8?B?T2F4Rnp1N0J4UUFjNEpYZjhrSEo2YzF6SEVQZTl5L3c5QTNKN2JEL0xYR2VU?=
 =?utf-8?B?M3kyMkFYWmc1ZWpUM054TUxDS2FnSjFJY3M1ODZKQzgwVjlqdDNUa29NOVVz?=
 =?utf-8?B?S25tZ1d4K1J3eDdleTZUS0t6WElJUHRLd1lIaFlSSzVQSnJySjUzNmo5NGFK?=
 =?utf-8?Q?zkyCyBKD6xeLrkBzw8Tc6EfsV74yiUF6A/nwP/OMEV+l?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 730cb20b-ab43-4ab2-38c8-08dbf77d0a2f
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2023 23:34:21.8532
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TRhRGPUt3XOxvoVzIJ895j2owWDXENQrqMhDnMR1TW1F7JTqPyIYpr/k+Bvk6O6M
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5111
X-Spam-Status: No, score=-3.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,
	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 groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Thu, 07 Dec 2023 15:34:39 -0800 (PST)

Hi Reinette,

On 12/7/2023 5:26 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/7/2023 3:07 PM, Moger, Babu wrote:
>> On 12/7/2023 1:29 PM, Reinette Chatre wrote:
>>> On 12/7/2023 8:12 AM, Moger, Babu wrote:
>>>> On 12/6/23 12:49, Reinette Chatre wrote:
>>>>> On 12/6/2023 7:40 AM, Moger, Babu wrote:
>>>>>> On 12/5/23 17:17, Reinette Chatre wrote:
>>>>>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>>>>>>>> b. Mount with ABMC support
>>>>>>>>      #umount /sys/fs/resctrl/
>>>>>>>>      #mount  -o abmc -t resctrl resctrl /sys/fs/resctrl/
>>>>>>>>      
>>>>>>> hmmm ... so this requires the user to mount resctrl, determine if the
>>>>>>> feature is supported, unmount resctrl, remount resctrl with feature enabled.
>>>>>>> Could you please elaborate what prevents this feature from being enabled
>>>>>>> without needing to remount resctrl?
>>>>>> Spec says
>>>>>> "Enabling ABMC: ABMC is enabled by setting L3_QOS_EXT_CFG.ABMC_En=1 (see
>>>>>> Figure 19-7). When the state of ABMC_En is changed, it must be changed to
>>>>>> the updated value on all logical processors in the QOS Domain.
>>>>>> Upon transitions of the ABMC_En the following actions take place:
>>>>>> All ABMC assignable bandwidth counters are reset to 0.
>>>>>> The L3 default mode bandwidth counters are reset to 0.
>>>>>> The L3_QOS_ABMC_CFG MSR is reset to 0."
>>>>>>
>>>>>> So, all the monitoring group counters will be reset.
>>>>>>
>>>>>> It is technically possible to enable without remount. But ABMC mode
>>>>>> requires few new files(in each group) which I added when mounted with "-o
>>>>>> abmc". Thought it is a better option.
>>>>>>
>>>>>> Otherwise we need to add these files when ABMC is supported(not when
>>>>>> enabled). Need to add another file in /sys/fs/resctrl/info/L3_MON to
>>>>>> enable the feature on the fly.
>>>>>>
>>>>>> Both are acceptable options. Any thoughts?
>>>>> The new resctrl files in info/ could always be present. For example,
>>>>> user space may want to know how many counters are available before
>>>>> enabling the feature.
>>>>>
>>>>> It is not yet obvious to me that this feature requires new files
>>>>> in monitor groups.
>>>> There are two MBM events(total and local) in each group.
>>>> We should provide an interface to assign each event independently.
>>>> User can assign only one event in a group. We should also provide an
>>>> option assign both the events in the group. This needs to be done at
>>>> resctrl group level.
>>> Understood. I would like to start by considering how (if at all) existing
>>> files may be used, thus my example of using mbm_total_bytes, before adding
>>> more files.
>>>
>>>
>>> ...
>>>
>>>>>>>>      #cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes
>>>>>>>>      Unavailable
>>>>>>> I believe that "Unavailable" already has an accepted meaning within current
>>>>>>> interface and is associated with temporary failure. Even the AMD spec states "This
>>>>>>> is generally a temporary condition and subsequent reads may succeed". In the
>>>>>>> scenario above there is no chance that this counter would produce a value later.
>>>>>>> I do not think it is ideal to overload existing interface with different meanings
>>>>>>> associated with a new hardware specific feature ... something like "Disabled" seems
>>>>>>> more appropriate.
>>>>>> Hardware still reports it as unavailable. Also, there are some error cases
>>>>>> hardware can report unavailable. We may not be able to differentiate that.
>>>>> This highlights that this resctrl feature is currently latched to AMD's
>>>>> ABMC. I do not think we should require that this resctrl feature is backed
>>>>> by hardware that can support reads of counters that are disabled. A counter
>>>>> read really only needs to be sent to hardware if it is enabled.
>>>>>
>>>>>>> Considering this we may even consider using these files themselves as a
>>>>>>> way to enable the counters if they are disabled. For example, just
>>>>>>> "echo 1 > /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes" can be used
>>>>>> I am not sure about this. This is specific to domain 0. This group can
>>>>>> have cpus from multiple domains. I think we should have the interface for
>>>>>> all the domains(not for specific domain).
>>>>> Are the ABMC registers not per CPU? This is unclear to me at this time
>>>>> since changelog of patch #13 states it is per-CPU but yet the code
>>>>> uses smp_call_function_any().
>>>> Here are the clarifications from hardware engineer about this.
>>>>
>>>> # While configuring the counter, should we have to write (L3_QOS_ABMC_CFG)
>>>> on all the logical processors in a domain?
>>>>
>>>> No.  In order to configure a specific counter, you only need to write it
>>>> on a  single logical processor in a domain.  Configuring the actual ABMC
>>>> counter is a side-effect of the write to this register.  And the actual
>>>> ABMC counter configuration is a  global state.
>>>>
>>>> "Each logical processor implements a separate copy of these registers"
>>>> identifies that if you write a 5 to L3_QOS_ABMC_CFG on C0T0, you will not
>>>> read a 5 from the L3_QOS_ABMC_CFG register on C1T0.
>>> Thank you for this information. Would reading L3_QOS_ABMC_DSC register on
>>> C1T0 return the configuration written to L3_QOS_ABMC_CFG on C0T0 ?
>> Yes. Because the counter counter configuration is global. Reading L3_QOS_ABMC_DSC will return the configuration of the counter specified by
>>
>> QOS_ABMC_CFG[CtrID].
>
> To confirm, when you say "global" you mean within a domain?

Yes. That is correct.


>
>>> Even so, you do confirm that the counter configuration is per domain. If I
>>> understand correctly the implementation in this series assumes the counters
>>> are programmed identically on all domains, but theoretically the system can support
>>> domains with different counter configurations. For example, if a resource group
>>> is limited to CPUs in one domain it would be unnecessary to consume the other
>>> domain's counters.
>> Yes. It is programmed on all the domains. Separating the domain
>> configuration will require more changes. I am not planning to address
>> in this series.
> That may be ok. The priority is to consider how users want to interact with this
> feature and create a suitable interface to support this. This version may not
> separate domain configuration, but we do not want to create an the interface that
> prevents such an enhancement in the future. Especially since it is already known
> that hardware supports it.

Yes. Understood.

Thanks

Babu