Received-SPF: pass (google.com: domain of linux-kernel+bounces-167054-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <ab2a6a4b-3740-47c6-9443-e6bb7a0c1adb@intel.com>
Date: Thu, 2 May 2024 16:21:33 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v3 00/17] x86/resctrl : Support AMD Assignable
 Bandwidth Monitoring Counters (ABMC)
To: <babu.moger@amd.com>, Peter Newman <peternewman@google.com>
CC: <corbet@lwn.net>, <fenghua.yu@intel.com>, <tglx@linutronix.de>,
	<mingo@redhat.com>, <bp@alien8.de>, <dave.hansen@linux.intel.com>,
	<x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
	<rdunlap@infradead.org>, <tj@kernel.org>, <peterz@infradead.org>,
	<yanjiewtw@gmail.com>, <kim.phillips@amd.com>, <lukas.bulwahn@gmail.com>,
	<seanjc@google.com>, <jmattson@google.com>, <leitao@debian.org>,
	<jpoimboe@kernel.org>, <rick.p.edgecombe@intel.com>,
	<kirill.shutemov@linux.intel.com>, <jithu.joseph@intel.com>,
	<kai.huang@intel.com>, <kan.liang@linux.intel.com>,
	<daniel.sneddon@linux.intel.com>, <pbonzini@redhat.com>,
	<sandipan.das@amd.com>, <ilpo.jarvinen@linux.intel.com>,
	<maciej.wieczor-retman@intel.com>, <linux-doc@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <eranian@google.com>, <james.morse@arm.com>
References: <cover.1711674410.git.babu.moger@amd.com>
 <CALPaoCjZ3oLdKymJjASt0aqtd0GGOme7LavvYOtPYTb_rA-mYQ@mail.gmail.com>
 <b35dc4e9-7e8b-42ed-9a51-ae50d521cf4b@amd.com>
 <CALPaoChxYoJx8eR48EkSKf-hu2p2myQJLZEhj_Pq6O4R15-=5A@mail.gmail.com>
 <6edffe1b-e9a9-4995-8172-353efc189666@amd.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <6edffe1b-e9a9-4995-8172-353efc189666@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UkhheUUyWVZOMXlPbkxDUVZhdTFJc1Uya3pxNDYrRHJmRXdNMXZibitVTTQx?=
 =?utf-8?B?bUxTOGZ3SlRoT0JwWTA2QkFvU0szeUdzeW54V1BWdUhvTWdHNWhHZ0xLc0FC?=
 =?utf-8?B?WmkxQUhveU41aTI1V1drVSt6dXR5eHhDdnFFajBVa2FWOVA1NnhsZ0dNT0N5?=
 =?utf-8?B?YmZtZVN2N1RUVFFvVGwwbDZOalBUSWlrQjlRVlVlY1ZJRmRMRVdIOHdXWCtl?=
 =?utf-8?B?ellDTFV0cVZheDB1ODJzUjJwMUdPQzFEZVg3NGRJeGYxeTFYaWNES0lrNEtB?=
 =?utf-8?B?YkRSdndENzB0WG9LNm9vVnhsVVRMUFRMaG9mcm85a2ROSVhObjlpUkJ4TVZZ?=
 =?utf-8?B?eEdiaU1lblpjRUUybDhyVkx1aFh0ZEhaM3YxTTkxNGRDbTAvaXMyNzlweXUr?=
 =?utf-8?B?bzN1S3N5czJSdmhLcHVzMnFwUTZhRUVyOHQ2RUxSVmVHQy91MlZkaUI2VjJl?=
 =?utf-8?B?Z2VzZ2xGTnJxak8zZzdhcUhUTEZXaEZ4aHFka0tzVHV5eE5rYlBHWnh0dnVJ?=
 =?utf-8?B?bmxSTldMNHRmd01YYS8wcVlHNWtqOUs2Q3hoQ1F6SHUyZTZtTCtVNUhaNUxN?=
 =?utf-8?B?WmVqL0NaS3Z6Z0tMQXZtcmFydmhtSk91eTR1eE81L2dBSFFmZnFTRlQ5eVQy?=
 =?utf-8?B?cmVVNG1HN1NrR3BQQ2wzYldCbnNYSGFJTU1QZGF0eXEvVWVzRExKQk1wbEY4?=
 =?utf-8?B?Sk1ZSnJaRDZxV3A5a2lqUEZkeFZ0ZmE4VGhOSDhMOVlzTm5RQzVKZDNKeHFz?=
 =?utf-8?B?VDIvQStHTnJiUHVBclhKVXNQejVKSm95bmVLc2pJelVTS3NXUFIwQytxbXp0?=
 =?utf-8?B?cW9hTEtobXBmT1Y2VTRUanhrZk9BS25BNHl2VE4vanRrMGt2c1VXbzJrTGs2?=
 =?utf-8?B?NU9FVExRN295ZmxORHdLSzNlWU4wc3pvUzRXa2JvcUZ3eFpxeUNtekR3dGs2?=
 =?utf-8?B?RU9aKzRXaVdiUFdlUVZKYUFOTWxSNDFGeEdhWmtGL09vZWplcklUU3BDK3R0?=
 =?utf-8?B?ak1tYkU2R21lWFZwWjFxSXViaXg4WXRqRG8rYkpKV2NrSkx6LzduaU1kbUFx?=
 =?utf-8?B?Q0hRTS8zNFhOZENDMklDNkFEa24xdVBvM0tqZ3o1L3lEdUF6cDlOZ1IrZFpM?=
 =?utf-8?B?Rm4wV3NQc3NYRjNOVDBrZU5iempqblYzOTdwS2RkZVBjdlR1WWVRa1dTNnF1?=
 =?utf-8?B?R296TlluWXQ3QmxibiszOUlHZjdVSXZ4SFQ5MDkzck41RHNoWFQyd3V5akth?=
 =?utf-8?B?bzh6SUtpU3JCQjVYWDhVdk9KWkkwdXI4ckZYWGtJem81eU5iSEtMRzNyaUVU?=
 =?utf-8?B?My84OGVoZHM5bGx6TG1zNGhzd2ZyanFkU3BMcm1GRnk0QnpXZFJmci85KzN1?=
 =?utf-8?B?bVp3d3o3TnA5Vjg0bmJKMVlmUTJrN0kwdXltdnRiVjVjRG90dkNzMlAySzBD?=
 =?utf-8?B?ajN6T21XSlg0YUVNVTdUOFZqbXZXa2R0OGNaQUlJcndCY25KdDN4alFUVDBH?=
 =?utf-8?B?OC9FRlhtZTF6MHV3M3RQWGgyb29jVEUwczliRFdJMDQxem8yTWZmVlVDaXBH?=
 =?utf-8?B?alpSMzNBNVFaNXowSTVnWjZSZHNZV1FSODNRVG5NUkFWd2NETUpiRXF0aG1t?=
 =?utf-8?B?ZWFHU3VOaG1taUpCZmsrUG5wdnA5M0gxWC9vTmsxTG90MXg2UmxUWUc0TXZ0?=
 =?utf-8?B?R0t1OXl0Rkt0RnlVVDVMMHBGNEpNcUxVeHBob2poeXhpNURXZldGTnVIYlB5?=
 =?utf-8?B?M3pnVllqTWhsLzN6Y2loaG9rOUd1MGgxVDFuS3I3NFgxOHRWU05Hc0I5b1ZB?=
 =?utf-8?B?WTZJSkJHQTBncDRsQ3l2QlI3OWc1aDVLU2RWaFRqT283TTlSMDlVWnpSWjZq?=
 =?utf-8?B?VWpDN3YxekVQN1lJeVE0Zm44aEpDTEZtNzJ1M1p0RjN2NDRSaElNWVgvVGI1?=
 =?utf-8?B?U1RjRkFvMG1MTUZqQnlRUGpxKzEwWkJLWHJZczBzdVRPMFBucVFjVlZXQ3pJ?=
 =?utf-8?B?U0J6bU1yWjhTTGcwb09aV29ZaXpHb1h5NGhjamIxZmlWOWxFcXRPQ3FrWFVD?=
 =?utf-8?B?VXNLRVpmdkV6Mytza2JkZmxXL3RPdTh4OGxsVDZjbVc1cGw4VGhlc0JIUkxa?=
 =?utf-8?B?MTA1NENyV2pDTzhwbnNaVG4vdjk1dC93NUNvL00wYkFJenpZOFdnTWh3cEpC?=
 =?utf-8?B?dlE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dfd75d29-6196-4d5b-71f1-08dc6afe9d5f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2024 23:21:37.5834
 (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: ZzNpp0BdRDQmPIrbPOiZpsspDPI5KXc7+0W3UDg6SXZSY/Kpx4hD64aIpKCbA+ygW7ZIrNqG4gDpKwhDJ5xJvsvfsVdDaYufNQ+e7PozfPQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7003
X-OriginatorOrg: intel.com

Hi Peter and Babu,

On 5/2/2024 1:14 PM, Moger, Babu wrote:
> On 5/2/24 12:50, Peter Newman wrote:
>> On Thu, May 2, 2024 at 9:25 AM Moger, Babu <babu.moger@amd.com> wrote:
>>> On 5/1/24 12:48, Peter Newman wrote:
..

>>>> I chose to make this a mount option to simplify the management of the
>>>> monitor tracking data structures. They are simply allocated at mount
>>>> time and deallocated and unmount.
>>>
>>> Initially I added it as an mount option.
>>> Based on our earlier discussion, we decided to use the assign feature by
>>> default if hardware supports it. Users don't have to worry about the details.
>>>>
>>>> I called the option "mon_assign": The mount option parser calls
>>>> resctrl_arch_mon_assign_enable() to determine whether the
>>>> implementation supports assignment in some form. If it returns an
>>>> error, the mount fails. When successful, the assignable monitor count
>>>> is made non-zero in the appropriate rdt_resource, triggering the
>>>> behavior change in the FS layer.
>>>>
>>>> I'm still not sure if it's a good idea to enable monitor assignment by
>>>> default. This would be a major disruption in the MBM usage model
>>>> triggered by moving software between AMD CPU models. I thought the
>>>
>>> Why will it be a disruption? Why do you think mount option will solve the
>>> problem? As always, there will be option to go back to legacy mode. right?
>>>
>>>> safest option was to disallow creating more monitoring groups than
>>>> monitors unless the option is selected. Given that nobody else
>>>
>>> Current code allows to create more groups, but it will report "Monitor
>>> assignment failed" when it runs out of monitors.
>>
>> Ok that should be fine then.
>>
>> However, I don't think it's necessary to support dynamically changing
>> the usage model of monitoring groups without remounting. I believe it
>> makes it more difficult for the FS code to generically manage monitor
>> assignment.
> 
> Are you suggesting to enable ABMC by default when available?

I do think ABMC should be enabled by default when available and it looks
to be what this series aims to do [1]. The way I reason about this is
that legacy user space gets more reliable monitoring behavior without
needing to change behavior.

I thought there was discussion about communicating to user space
when an attempt is made to read data from an event that does not
have a counter assigned. Something like below but I did not notice this
in this series.

# cat /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
Unassigned

> 
> Then provide the mount option switch back to legacy mode?
> I am fine with that if we all agree on that.

Why is a mount option needed? I think we should avoid requiring a remount
unless required and I would like to understand why it is required here.

Peter: could you please elaborate what you mean with it makes it more
difficult for the FS code to generically manage monitor assignment?

Why would user space be required to recreate all control and monitor
groups if wanting to change how memory bandwidth monitoring is done?

From this implementation it has been difficult to understand the impact
of switching between ABMC and legacy.

Reinette

[1] https://lore.kernel.org/lkml/e898059f3c182886b1c16353be7db76d9b852b02.1711674410.git.babu.moger@amd.com/