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: <95c56b28-414d-b148-055f-b41e7da9401a@intel.com>
Date:   Thu, 20 Jul 2023 10:27:46 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v3 3/8] x86/resctrl: Add a new node-scoped resource to
 rdt_resources_all[]
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Peter Newman <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>,
        Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Babu Moger <babu.moger@amd.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <patches@lists.linux.dev>
References: <20230713163207.219710-1-tony.luck@intel.com>
 <20230713163207.219710-4-tony.luck@intel.com>
 <769521ee-6907-1613-442f-e2915a8ea554@intel.com>
 <ZLcY1MQr5/Ibnv7N@agluck-desk3>
 <076a76c9-ab57-cae2-e6e4-99d26921b578@intel.com>
 <ZLh9y8LQNkR/VhOq@agluck-desk3>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZLh9y8LQNkR/VhOq@agluck-desk3>
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?cFdpYURocXk2a1pCb1lXZ2pFZ3U1Z0o5MUNweEJEbWwwckRia2JrRS9YRVVj?=
 =?utf-8?B?VGR4L09ybXBRVzlHQUdWVnQxQXVFaTROcGM4YTlPeERraHdiaHJZT2FPaWZi?=
 =?utf-8?B?SDY0RnZKQUF2WkxscmJUTXNoYVBDUDVHKzJrZ1diZndKTXg1elY3VFp6OWI2?=
 =?utf-8?B?anQ4d3dNN0t0bmw1RzZWejgyc3FUYkVzZ3IyRkFFa3MvVFV6QXpwUDBWSjVR?=
 =?utf-8?B?c1J0SFRMbmZ5enZuSis5NkpaUnRiOXUxWHZLQTJKbXBudmwya20wRW45VFFV?=
 =?utf-8?B?MkhwZXY1R1hqRzBEQXpPRWpSanlhRzNRVWp5bDExMUhGN1ovRzUwZWs0emJB?=
 =?utf-8?B?aVpvbWpuYVFTVVBkSitJUGYwaGYyWXJmV2hTa0pnQVcwd1FDd1dIdzVHeHZh?=
 =?utf-8?B?aUNUeG8xM0E3azE0Vm9kOGVqRE1MNm5uNHdNU1F4RTJ6TSt2WGZmUk4xRkVB?=
 =?utf-8?B?QkZMQzZLay9EeFFGVXNabExQV1JxbUJKek54eGpldjFmc1RFaEJRejg5QTIr?=
 =?utf-8?B?Y2hxeGd0N0Y4SUdDZkFvcjVaZU0yU21EZWhtU0JUZWFSS3U3bGVUUkEyQTEx?=
 =?utf-8?B?Um1VRGlNbXdOWnZHOUltRnVuaWQzUjVZRnJ4N29SU2dVTXdRZDEyNzQ2MDcr?=
 =?utf-8?B?T2pIaDBUdk5nRUJjZXoxKzdDK2o1WUVJdTRxd0poaEVvRklBZlRaOVczb1pw?=
 =?utf-8?B?dGFMT3R1KzVHTVFMWEU1akxNV2twMVdxcUZmUmdzRzZNUkJGemlyR3kwSnFX?=
 =?utf-8?B?ZTh5SitsMWtqY3FublF0K3J3SnR1c3h5RTk4VTQxRnY3WmZ2TlpDK1Nhd1Z5?=
 =?utf-8?B?VjdpdEhoZzZLb2NxOHZ4MUtjeDZsOEh0elh2dXhPMkRXUzA5Q3dHdGRJZHoy?=
 =?utf-8?B?YXFuRjhrb3h6Y3R4Ump5azAvVXBNQm5kdDJCYVVqV2Izc05FclhWQUd1N1NL?=
 =?utf-8?B?NjRTZGI1UEIxQnY4WjQ4a0dUVVFocTMvWDIzcEZWaXVRN2QxQUhkWFZEYmt5?=
 =?utf-8?B?U0U1eUluSW8yUGJoVWRXcVh1cm9CNWhwZEl6ZEN6bHh4RGp6dUtJVDNyWno2?=
 =?utf-8?B?RFBaYzRQREFUd2RvK0VoaXhRZThoVVU2VjJLbkQwKzdYQlhscVQwbVg2bHN3?=
 =?utf-8?B?Q3FDL1Ixa3k1cnNlR3ZwcFY1M2lDMzBDNFpKUW9nUTY0WkQrQkhKOENKaEhl?=
 =?utf-8?B?bU9XMVpmSW5Ed0ZJVnNsVURtYzAzMGF6TUM3ekdDSzhoZnUxbVgvL3g4Z2pp?=
 =?utf-8?B?R3Q1bnNYeDhuYnQ2TElldTcrTDdVOGVyUnJMM3ZnV2RBTHZCV0VhSFV4UzYw?=
 =?utf-8?B?SXV5QVd3Um12c3hWeTRCKzJYOTUvMXIzVGpvOXdmeGkxMm1SZ2dYdjczcjJ0?=
 =?utf-8?B?dGlFNkFXd2NUb0UyNWxRNVFKcXhNWHlJOGlqdlVidDdGNWNvR1FFR1lhanBR?=
 =?utf-8?B?MlgxcXlISTRvNjNwVEFKU2NoUGZDWW9TNkVyc3FMVTArNEZzUUJ2djY2eE8z?=
 =?utf-8?B?OVJ6K0hiM25QaVRyN28zdmJPaXlqVFoybmJncnZOMnFQZ1VqK1M0U2dwWjVW?=
 =?utf-8?B?bFZtblU0ZGJzaXAxZzZINExEdjg0SlJrUXNVL2NZdDFZTFp0Q2NKZW1WdU5l?=
 =?utf-8?B?amgwV0lGSFQvOG5jeTdyRHhpUzVkeHVBWnlwWlJqQXZBem5BVGMzY1l4LzVG?=
 =?utf-8?B?UE5uQ0lIVHRiTkRpNkhCUnpwY2JOU0lhMW1uZXdNKzkrQmV3YXc4SGhqRVBy?=
 =?utf-8?B?dStING1Dd1lSODgremdhdnc0WTJ4K0cvYmxQMWdVbE9ya3pDVmdNZFgzK1cz?=
 =?utf-8?B?cmJUL2d3SElaZWZaeDYwV0RRcStVeHg3VFhIakhGTVhvY0xvSEZQQTVMUXBW?=
 =?utf-8?B?VEx2b3dFRXlGNjFGTEJOc3ZrRFh3dEQxZkNBcFU4RW9IMEZpOStla2dZcG5I?=
 =?utf-8?B?ell4VkNhRDZHSEpxNVY4SWdaWmV6MTZJVDV2WWF1SEhBMkMvczVwellIREp4?=
 =?utf-8?B?cUpBY0ROejZTcHo3czAzRTM1OStodHF3L0MxQzZnTXJ4VVhDWWQzRTlmMDBK?=
 =?utf-8?B?Y0JpOUN0YlU5RCtlZm4zSXhDRElobEgyLzllTDJwbHZUdUU1d1dSdXVqMVo4?=
 =?utf-8?B?TEEveGgvMnBHNlUxaTgzN1kxc2RQRkFSeGpOQlh4S0ZHTEhzamNtT2hKZXVQ?=
 =?utf-8?B?bFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7cf80e28-ec65-4666-f531-08db8946a371
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jul 2023 17:27:48.7171
 (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: IL/qiEm1cwnDB2zxAAwwHoa7EdJI6Lomxmt7gLul2thnu0KxSJR5QecA7IJySdjU/RIKfhZP7iYiJUDEJZN72XRX3zTq+PfHiKtJFLVy8fc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB6499
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 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 7/19/2023 5:20 PM, Tony Luck wrote:
> Here's a quick hack to see how things might look with
> separate domain lists in the "L3" resource.
> 
> For testing purposes on a non-SNC system I set ->mon_scope =
> MON_SCOPE_NODE, but made domain_add_cpu() allocate the mondomains
> list based on L3 scope ... just so I could check that I found all
> the places where monitoring needs to use the mondomains list.
> The kernel doesn't crash when running tools/testing/selftests/resctrl,
> and the tests all pass. But that doesn't mean I didn't miss something.
> 
> Some restructuring of control vs. monitoing initialization might
> avoid some of the code I duplicated in domain_add_cpu(). But this
> is intended just as a "Is this what you meant?" before I dig deeper.

Thank you for considering the approach. I find that this sample move
towards the idea while also highlighting what else can be considered.
I do not know if you already considered these ideas and found it flawed
so I will try to make it explicit so that you can point out to me where
things will fall apart.

The sample code introduces a new list "mondomains" that is intended to
be used when the monitoring scope is different from the allocation scope.
This introduces duplication when the monitoring and allocation scope is
different. Each list, "domains" and "mondomains" will host structures
that can accommodate both monitoring and allocation data, with the data
not relevant to the list going unused as it is unnecessarily duplicated.
Additionally this forces significant portions of resctrl to now always
consider whether the monitoring and allocation scope is different ...
note how this sample now has code like below scattered throughout.
   h = r->mon_scope ? &r->mondomains : &r->domains;

I also find the domain_add_cpu() becoming intricate as it needs to
navigate all the different scenarios.

This unnecessary duplication, new environment checks needed throughout,
and additional code complexities are red flags to me that this solution
is not well integrated.

To deal with these complexities I would like to consider if it may
make things simpler to always (irrespective of allocation and
monitoring scope) maintain allocation and monitoring domain lists.
Each list need only carry data appropriate to its use ... the allocation
list only has data relevant to allocation, the monitoring list only
has data relevant to monitoring. This is the struct rdt_domain related
split I mentioned previously.

Code could become something like:

resctrl_online_cpu()
{
	...
	for_each_alloc_capable_rdt_resource(r)
		alloc_domain_add_cpu(...)
	for_each_mon_capable_rdt_resource(r)
		mon_domain_add_cpu(...)
	...
}

This would reduce complication in domain_add_cpu() since each domain list
only need to concern itself with monitoring or allocation.

Even resctrl_online_domain() can be siplified significantly by
making it specific to allocation or monitoring. For example,
resctrl_online_mon_domain() would only and always just run
the monitoring related code.

With the separate allocation and monitoring domain lists there
may no longer be a need for scattering code with checks like:
   h = r->mon_scope ? &r->mondomains : &r->domains;
This would be because the code can directly pick the domain
list it is operating on.

What do you think? The above is just refactoring of existing
code and from what I can tell this would make supporting
SNC straight forward.

Reinette