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: <3e6a0f42-86a3-18f3-1b6c-0e98791324e4@intel.com>
Date:   Thu, 25 Aug 2022 14:24:36 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.2.0
Subject: Re: [PATCH v3 06/10] x86/resctrl: Introduce mon_configurable to
 detect Bandwidth Monitoring Event Configuration
To:     <babu.moger@amd.com>, <fenghua.yu@intel.com>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <bp@alien8.de>
CC:     <eranian@google.com>, <dave.hansen@linux.intel.com>,
        <x86@kernel.org>, <hpa@zytor.com>, <corbet@lwn.net>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <bagasdotme@gmail.com>, "Luck, Tony" <tony.luck@intel.com>
References: <166117559756.6695.16047463526634290701.stgit@bmoger-ubuntu>
 <166117582698.6695.3458866624522799072.stgit@bmoger-ubuntu>
 <575bf1d7-8780-8c54-851c-e9849d1815f8@intel.com>
 <5d650c6c-2f17-6013-f63f-49a182961494@amd.com>
 <93d1a26f-559f-63f7-d2b1-e8831a9df62c@intel.com>
 <d970a472-1da3-19d2-bacc-05f4ed02992b@amd.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <d970a472-1da3-19d2-bacc-05f4ed02992b@amd.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?OFUrbUlhN3Jna3ZHNnBmWlQrMk0ydDNtcktQOTN6S1pCRVlDS010N3JNenlE?=
 =?utf-8?B?ZG9NVzFIdWEwMERnNG44RWRjRVZZdWU3ejVvd2VSZkZVbHVJcFhsbkhpWmtr?=
 =?utf-8?B?eXN4SCt2TVlrMWRlZ0cxSjBoSUhsZktWWU9BQjU3TzMyVnhaK1hKTXpCVCt2?=
 =?utf-8?B?ZFVUY2tXbmdpTU5SVlNZaFZUMGxkL0wzaktsYXZxT3NFZzg2N3ZJa3JHVm1Q?=
 =?utf-8?B?Rzd4VkdvT2hFYnJUYkhrRzlPaCtZT0J3a3RpNGJJM3dQaGR5RVpRSDZmUlFU?=
 =?utf-8?B?RSsxS3F4MTN6ODlhUi8vNzNOcWphVXFiM2dQVlVpQTBFbktwV3VKRUltQ3ly?=
 =?utf-8?B?RjJlOGpOUG1KL1NiaUdNdVlFTUtKdVVkZU0rWlJSQ2wvZFNSb1hBMml5d2d3?=
 =?utf-8?B?TEdQUEFYSXFiTW1RQU9kKzJwVWh3elpTcmJJQlhjSkZ3ZzlOeEJacHdzUnk0?=
 =?utf-8?B?Y3g4ZlhCL1dSZkpWNVIzQXFOaDFicUNEZ0Z1Ym9aWjFEeFNUQzZmeDRTU1ZS?=
 =?utf-8?B?clBRUktTcDdNdUZzMFJjTlgwNGVNNU1oaUl2ajdycUdjT0Evak9yZjVybGJC?=
 =?utf-8?B?YTdpTUlPUlFiamZ3UXMwemdlUGI0ZzB2UUpvZnh6Vk5PNlpobjBGcUNVSVRV?=
 =?utf-8?B?MlJINGxpbUxNM2ZPYUNmcnVrTDFDVCsxNTJJZ25UOVRQR0I3b0NWMWE5NXRp?=
 =?utf-8?B?WUQwTmFPNkVJbFBqYWJWa2p2NHY2TEpmWlVmUERxMFZ2N0ZDZHVZL1dRT0Fy?=
 =?utf-8?B?byt6eEpBVGpQRjQxOHMrYmZCdDFhM1VXcDJBV3JkejE1Zkg0TCs1VGRETkNE?=
 =?utf-8?B?MWZCcHJNeWg4N1kvcTRtUEU1bVZpR0J4NjF2clZIUEQyVVNQd2RUcCtqY1lE?=
 =?utf-8?B?K2JPQS8zYzdSQUlIU1BVTnYrTThaaUFFcyswT3I0dzVHMjA3cjkyK2FEOVpC?=
 =?utf-8?B?aWNQMTE0eTYxb0s2b2NiNkFmL2t6NTBTVkdKTk1wejdENXQxZ3JBL1U1ZW5p?=
 =?utf-8?B?NVVIbFFUKzV0TGRjZ3VhME5lM01wcG5aeG5VbXVwTHFqSDdSQUJvSlgzTVNT?=
 =?utf-8?B?V1gwTWdNMmNTc0xIOW02b0pkZUZodEpTekdWb29ueG1PWXQ3QS91OVozbnVY?=
 =?utf-8?B?d29YVzJVNHpGNUpuOWhVMU1IODNUWitwSkhPNHRnb3l4SUFXQXJza1BFcktl?=
 =?utf-8?B?ek1qaHR4Zy9yTEgvVmtibVF5a1B5cnBpc0dFNVdldTVLSG9PTTA0VDJ1MEZj?=
 =?utf-8?B?a3A2dkFmWnVSR0FJckhXcENHNlMxM0h3VlpEbjJwcUFaMlVZaWljeUdmR0dS?=
 =?utf-8?B?SFhKR0xTTUZUZ1o0NDdPaUZsaUN4OHlZaUROWjhiRml1WTdMVFMvaDVwMGYv?=
 =?utf-8?B?NlZKQnJmS0xIcTBDUHpSZGhkUTgzQjFVcVdtYmNuNVRlQ0JtMzcvZ3hnUXhN?=
 =?utf-8?B?dHhoU2xNQURzTnR1QWxTbmVTN0RJOVJNNHZ4SmUzOUlKRjBINUNrUng2anVj?=
 =?utf-8?B?anYvbzJaV2pYTlpPWEgrNzdqQmRrOUk3R1NwVmlsZ2tQZ05ac2phdFVEUWRZ?=
 =?utf-8?B?TG56WEplQW1DUUNtbWRLejhUcEhESmJrKzRrcDAwb2JVa204U3hDc2NBaVFi?=
 =?utf-8?B?K1VxOGFSVmlwYWpVSzVVaFRnY3hpN3BBUzVqL1ZlMlJPVnhrUjlPclRhVXF5?=
 =?utf-8?B?YVRPR0o2OXV3QXhBKy9UYzRkQWp3WDY4Y2piY1VQS3FGMDU3MGdrUDFwUEty?=
 =?utf-8?B?VGlNRXFWZ3VmdzhMTHdGTEQva25QdElSYVpodWd5UWlDQ203cFZYWGxyTVY0?=
 =?utf-8?B?TXZ3K1pNVTluWU83MXVITVVWNzdKVmdZVDY0VmlHQ3lFOVRIdVFsQTg0ZkVI?=
 =?utf-8?B?T015c1dBVWVkdDRpQk0zNzExcTh2YzZMR1g2b1JZRUFyUlZJZUhMbWdUeGV1?=
 =?utf-8?B?MzZNaU1CekFYOS9XcUJtLzRNOXRPNHlqUit3eEFBTGNRY2M0UllUbzdaNE5V?=
 =?utf-8?B?OTRvNHV3Qkkrdm9CcUM5TTFjSGdiNFpOMkZyWEhTZzFlRzhRS0N2WXFpdklu?=
 =?utf-8?B?Y2tBYTY5ZE5BZS90ejRUd0Y3UTRrK0lWbGJTa2c2YkZOTm5vRXJQM242cHVN?=
 =?utf-8?B?WDY0ZnZldmR6TzV5S3RWWDBsYmFSeGhaT3ZudDhxak9ZQy9mclB5ZGZySks5?=
 =?utf-8?B?MXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 260a8903-604a-4e5c-f5e7-08da86e03955
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Aug 2022 21:24:42.1889
 (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: DNExxcJuMo/9hfnfdcvDnssrplcOYieJHLb66qAuZ64vqRGQJPNyCCMbU3IhvSfCnPCZQPM71K00yg03C2IvM4B9kg3Bm4s4DuxnBhNM93I=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4041
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,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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 Babu,

On 8/25/2022 1:44 PM, Moger, Babu wrote:
> 
> On 8/25/2022 10:56 AM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/25/2022 8:11 AM, Moger, Babu wrote:
>>> On 8/24/22 16:15, Reinette Chatre wrote:
>>>> On 8/22/2022 6:43 AM, Babu Moger wrote:
>>>>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>>>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>>>>> are configurable when this feature is present.
>>>>>
>>>>> Set mon_configurable if the feature is available.
>>>>>
>> ...
>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> index fc5286067201..855483b297a8 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>>>>>       return 0;
>>>>>   }
>>>>>   +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>>>>> +                     struct seq_file *seq, void *v)
>>>>> +{
>>>>> +    struct rdt_resource *r = of->kn->parent->priv;
>>>>> +
>>>>> +    seq_printf(seq, "%d\n", r->mon_configurable);
>>>> Why is this file needed? It seems that the next patches also introduce
>>>> files in support of this new feature that will make the actual configuration
>>>> data accessible - those files are only created if this feature is supported.
>>>> Would those files not be sufficient for user space to learn about the feature
>>>> support?
>>> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
>>> the information about all the monitoring features. As this is one of the
>>> mon features, I have added it there. Also, this can be used from the
>>> utility(like pqos or rdtset) to check if the configuring the monitor is
>>> supported without looking at individual files. It makes things easier.
>> I understand the motivation. My concern is that this is a resource wide
>> file that will display a binary value that, if true, currently means two
>> events are configurable. We need to consider how things can change in the
>> future. We should consider that this is only the beginning of monitoring
>> configuration and need this interface to be ready for future changes. For
>> example, what if all of the monitoring events are configurable? Let's say,
>> for example, in future AMD hardware the "llc_occupancy" event also becomes
>> configurable, how should info/L3_MON/configurable be interpreted? On some
>> machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
>> configurable and on some machines it would mean that mbm_total_bytes,
>> mbm_local_bytes, and llc_occupancy are configurable. This does not make
>> it easy for utilities.
>>
>> So, in this series the info directory on a system that supports BMEC
>> would look like:
>>
>> info/L3_MON/mon_features:llc_occupancy
>> info/L3_MON/mon_features:mbm_total_bytes
>> info/L3_MON/mon_features:mbm_local_bytes
>> info/L3_MON/configurable:1
>>
>> Would information like below not be more specific?
>> info/L3_MON/mon_features:llc_occupancy
>> info/L3_MON/mon_features:mbm_total_bytes
>> info/L3_MON/mon_features:mbm_local_bytes
>> info/L3_MON/mon_features:mbm_total_config
>> info/L3_MON/mon_features:mbm_local_config
> 
> Hi Reinette,
> 
>  Yes. That is more specific.
> 
> So, basically your idea is to print the information from mon_evt structure if mon_configarable is set in the resource structure.
> 
> Some thing like ..
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 83c8780726ff..46c6bf3f08e3 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1194,8 +1194,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
>         struct rdt_resource *r = of->kn->parent->priv;
>         struct mon_evt *mevt;
> 
> -       list_for_each_entry(mevt, &r->evt_list, list)
> +       list_for_each_entry(mevt, &r->evt_list, list) {
>                 seq_printf(seq, "%s\n", mevt->name);
> +               if (r->mon_configurable)
> +                       seq_printf(seq, "%s\n", mevt->config);
> +       }
> 
>         return 0;
>  }
> 
> Is that the idea?


I do not see why struct rdt_resource->configurable is needed. Again, this
is a resource wide property with an implicit meaning related to only two
event counters. Again, what if AMD later makes the llc_occupancy event counter
configurable? How can resctrl know, using "r->mon_configurable" whether
it should print mevt->config? 

How about:
		if (mevt->config)
			seq_printf(seq, "%s\n", mevt->config);

As I mentioned in [1], mevt->config can be set in rdt_get_mon_l3_config()
based on a check on the BMEC feature instead of hardcoded as it is now.
Or, if the string manipulation is of concern the hardcoding of mevt->config
(perhaps then mevt->config_name) could remain and a new mevt->configurable
could be set from rdt_get_mon_l3_config() and then the above could be:

		if (mevt->configurable)
			seq_printf(seq, "%s\n", mevt->config_name);

Reinette

[1] https://lore.kernel.org/lkml/c5777707-746e-edab-2ce2-3405ff55be56@intel.com/