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: <c0755752-ce84-b5f9-6ac6-97812e98b8c3@intel.com>
Date:   Fri, 5 May 2023 16:17:18 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.1
Subject: Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        James Morse <james.morse@arm.com>,
        "Babu Moger" <babu.moger@amd.com>
CC:     <x86@kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230420220636.53527-1-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230420220636.53527-1-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aXE4TDFPa1VkckxQdHg3SmtTQit6ZWJFK1JLTkl4MFJmWTZYSWhLbkI1YjZR?=
 =?utf-8?B?aG9CWGJBcWhteXdOSlBQdVliaitDNmtjMDJ1OGZQc1oyRWNhSFlvSWFtWDI1?=
 =?utf-8?B?a0J5TURCOEEveDk4VDBGbnlMc3QrMGNCMTU3aHFtdkQ4Mm9MSW1uM0wzSDha?=
 =?utf-8?B?SFdBZXI3NEI0OEQvZUxBMGNUdm5VaG9TYTVGdjM5ZWpjMWZ0TFVCU0Z5dUtk?=
 =?utf-8?B?UGxBVnpqSGFScDRHZUVEN1pZdUVKa1Zhc2Zvamh1Q0hjZVBaT3lhWm51MVBC?=
 =?utf-8?B?TXh0bzVEZUpsbnk0RGFNbU94NkFDUmJkS25FcFlEWTJYdXRPMXBQdDYrd2xr?=
 =?utf-8?B?RTd6WHA5aThUMU12Sk1weXVQci9OTnJNYWFwTHlNMkdGekh1Z0k3T2F0L2Er?=
 =?utf-8?B?ZmprcTVBN2dYaHoxTVFheWE5NzN2alpURWF1L3d6WGYvaE9yTGVaZjVKdXpV?=
 =?utf-8?B?d1kzSFJKOTl4ZzBRSkNRdiszOWYvTkNnVUc0b09hNkRXT3MyUWVuNjdML1Fl?=
 =?utf-8?B?cWxSOVVyU0FuZlQ5VXZWdzFKUDk2UG01L0ZOcXlHbGNIRDNwVmMxWnc2eGhL?=
 =?utf-8?B?QWpCZFhnWUNyTW92VElTMkxBeFpjWnRnTnFoL1VkcjNVWG0wNGhlOHAzWDJa?=
 =?utf-8?B?NE1VWGx3RG9lTUR3ZFBnejlEcjdhc1oyRitMWEVkRGJBdHVFYllzNGFPRVBX?=
 =?utf-8?B?a1RFMUFIT2lZUWNXZnhkeW5lcHlNL1BQT0d2TXJNS0p4TGxkVkcxYlZvWWhI?=
 =?utf-8?B?ZHZHRmJML1h3b1NhUm1qa0NacDdRY2c1WkhDSXNCd0VPRFE2eHczcUlDeUcw?=
 =?utf-8?B?NUNmYk9VeUh4cDl4Vk1sN2p5ZzdLUzJkb25tTWRQLzFObnRUSHZqWjMzYTJM?=
 =?utf-8?B?TnR0NzJabW5sbjlZbnZQSDdYK2NRNEdPRkh2MnFtLzNvQ0tPbXFhcWZRY0lQ?=
 =?utf-8?B?bWNzMm5Nak5FQzloYm16K04raitBUFJTNllNUUZaYXBOdlJYMC9xeXdrbjZZ?=
 =?utf-8?B?aGs4a3VVZGdMSno4aFNrZ21FL3pCYjJOMG1WcEsvalB1Zlp0U3dxclZ4RTlX?=
 =?utf-8?B?Y05EdVZQYkZpT29CYWJ0MlQrRDAvNWtURjBSZWlFMm9DQkRsUGFKaTJHWXN3?=
 =?utf-8?B?c2hYZGtJZEtac3VrUlhNcGtBM0N5MEJyenVjRmNkdmFvZ29DUFZrMlZwc0RG?=
 =?utf-8?B?VXRoaUZvVkJZVWpyV3J4SHFqQkhkQ1dwTXkva3Q2b3BjUUFxYm1MREVVNWxu?=
 =?utf-8?B?L2ZoaHlYUUg5N0lIK084ZDVETUMyVmluRStTUyt0eTlGTlIwaTA5ZEhSR0Y5?=
 =?utf-8?B?RTVYK0dpTTk2RVE5cjhod21CVzdRdUJEWHpwNDFkNzdMd1YxZHoxS0ptKzF3?=
 =?utf-8?B?Q250OXM0Q2FPTy9FRWdQV1dVbHBRZ2IyZUNEbE5VaXlmQnhPeHZ0bExwQytm?=
 =?utf-8?B?TjZtYU1CSDZ0UlFpaWpkamREaStVMlllbUpZb0NNVU1pTlJFOXRRS1NzOUZ3?=
 =?utf-8?B?aFA3eVMzNDFPYTZ2QkRLckJJcEVLZ0xLYmhuTUM3bGZaQnR1bzVaTUQ4cENL?=
 =?utf-8?B?L2MrdWZJUysyNHBuR1VXME5SWE9iREtaUEYrZXVaVU9OTVdXUGt2QUVGa2FL?=
 =?utf-8?B?REZhSVRtZFVjMlROTGxDSUdNR29ZOEh4OFNnQ0l6UHV3QnV0ZUxucjZKTENR?=
 =?utf-8?B?bGFrTHJnb011ZWNPR0JSWnRvcHFzQ3JPU0daOHFRRnF3d29NQUZUVkhPd3BD?=
 =?utf-8?B?bUU5eUY5eEFtRGVNQ3pLaXJCQ0NoR0VDZTFxaDdlc1lHS0V0b3dYb0JXbFJR?=
 =?utf-8?B?RlM1T1dIM0EwWHVxUkZQQ1RjdDBOZDI0Tjdud2xId3JjVTBiUC9ydElEeEtT?=
 =?utf-8?B?RTM5RTJDN2Q5QkoreGYrdWxKR2JDYlNZck85UWlQRTl0d0lZeFpwc3lxNmRP?=
 =?utf-8?B?WVpNM25jK2VycWtWMzZpN2NvM2pzR0dWdlpJRnM0RW1tdW9xWWNlWHZVZDgr?=
 =?utf-8?B?SUpLMlEycmtLaTN1T0VrMWk4NWwzU3pPcE12TTNmSU40dU9TNUIyRFRwV0x6?=
 =?utf-8?B?SHptekIvdlJ2ZTcwN1ZIQUh0Y3NzYkhreWVRenEyZ0tPWXU5bkk3alhwaXJE?=
 =?utf-8?B?WEQwazJBY2h1dTI4R1Y2aHpFRE5BNy9HaXlkRWlOVklCUnhKRFg1MDhnSEVY?=
 =?utf-8?B?THc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 04aff948-a913-4088-221c-08db4dbee03a
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2023 23:17:20.5128
 (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: JZp2UOnxKcl2fbpDJh7R+ap3PsDJmannzVwK4I/iaDeyYj7jpgelc5IEMMKPRegt/bsJWCq1kvjMEd37LbhDnsuJyrd3N1G1WM1NA0LabIc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5953
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.8 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,T_SCC_BODY_TEXT_LINE
        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 Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> This is very much proof of concept code at this stage. I have a few
> quality of service features that are hard to intergrate into the core

(integrate?)

> resctrl code because they are model specific, or have input parameters
> that do not fit neatly into the existing schemata model.

I would like to understand these two motivations better.

Regarding "because they are model specific": the features would remain
model specific no matter from where they are supported so it sounds to
me that you would like to move/contain the model specific logic to
the drivers? Why would this be a motivation since it seems that this
would still make things model specific. I do not think resctrl is
averse to model specific code when I consider the code like
cache_alloc_hsw_probe() and __check_quirks_intel().

Regarding "do not fit neatly into the existing schemata model": could
you please elaborate? If I understand correctly a driver would like
to take ownership of a line in the schemata file, I then look at
a driver as providing support for a resource. It looks like 
these new resources may not be "domain aware" so would require unique
parsing, but that is something we can do in resctrl, no? Something
like a unique "parse_line()" associated with each resctrl resource?

Considering the above it is not clear to me at this point why this
driver interface is needed. Why could each new driver not instead be
a resctrl resource?

> Also, as AMD, ARM, and now RISC-V are looking to share the core resctrl
> code, it might be helpful to have "driver" as a software layer for
> per-CPU architectural code to avoid cluttering the core.
> 
> None of my drivers are ready to post, so this series has a simple example
> driver that would meet the same debug requirements of Babu Moger's
> patch to expose the CLOSID/RMID in files in each directory:
> 
>   https://lore.kernel.org/all/168177449635.1758847.13040588638888054027.stgit@bmoger-ubuntu/
> 
> Doing this debug with a driver that can be loaded unloaded without
> having to unmount and remount the resctrl file system appears slightly
> more convenient that a "-o debug" option. But this example driver is
> really intended just as a toy example of what can be done.

The driver seems simple but I think it already shows that this can get
complicated with many sharp corners. If I understand correctly this driver
will add the "closid" and "rmid" files in every control and monitor group. This
driver thus assumes a system that supports both control and monitoring, but
that is not guaranteed. For robustness the "rmid" file should not appear
in a control group that does not support monitoring.

> 
> The series is broken into steps that add callback functions into various
> different parts of the resctrl hierarchy. That list of parts has been
> driven by the needs of the drivers that I want to write. The
> registration interface could be extended if there are additional
> hooks need for other drivers.

The boundaries of the resctrl and driver interface are not clear to me.
Looking at where the new driver API is created and how it is used in the
example code I see that this occurs in include/linux/resctrl.h. This is
the API that an architecture using resctrl is intended to use and
thus provides much more to the drivers that I'd expect it to want to or
be able to use based on this description. 

> 
> I'm looking for high level comments on the desireability of this approach

(desirability?)

> at this time. I don't expect any of this to be merged until I have some
> real drivers that use this to offer to upstream.

Some hints about scenarios that require this driver interface would be
helpful.

Apart from the high level comments above I looked briefly at the patches
and responded there where I have some high level comments/questions.

Reinette