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: <7f56cc04-021e-ff75-255a-dbfd80a3f6d7@intel.com>
Date:   Tue, 18 Jul 2023 13:37:24 -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 2/8] x86/resctrl: Remove hard code of RDT_RESOURCE_L3
 in monitor.c
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Shuah Khan" <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     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-3-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230713163207.219710-3-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?UUFxYjFWM1pwdkM3RGQvS2xPRUdvMGtWN3pDWlZHSTZBRHJIRjIySm93TUN3?=
 =?utf-8?B?MXlnMWZkMmNDWGRMWTVsTklBYkRSdlBTTnY5ZmFSeStTaW5SSndWdUo5KzdF?=
 =?utf-8?B?VkFDMFJoVU5WZFBFM3RCdWc4NW1tUGFaN1NOU0tBU0J6cDdpT08xWHhWaHdO?=
 =?utf-8?B?a2VLYmkwTDBYL0hCSUQrdEc2c2xSdXBxcjExR2w3YWt1akJiOFFrZEJpQmsw?=
 =?utf-8?B?TDA4b0xVVW9LR1JlMDU5K0FSRlR6Tld4QlM3NlVWN2lML2lWL2hERWRyTHhh?=
 =?utf-8?B?emladVFBcHh3Q1ZiL1NTNnZ2dnh4VkxObzU4R1NZZi9BajhCcEYzUyszcG1W?=
 =?utf-8?B?aUIvRi9CWlFiUzlwZHhhRGdSSEI3M0ZEbkNiVzFnUGZOZ2tvaGs2bEpQRGlv?=
 =?utf-8?B?U21qM1hmenA1U3FhRmprM0VyTEhMcGVJeHBlSlBuRy9OR3JQdWQwRXRkQ1A5?=
 =?utf-8?B?SzFwSWVOL2JyVEllSmRTWjFRcUppSTFhck9yck45Vi93cWJoaWlLcEtvNHM1?=
 =?utf-8?B?YTJWa25Bd3pVMEFhVUM2aTFnNThEbTM0VGZVMUhpVnhyNVZ4RWFyUGh3NS9D?=
 =?utf-8?B?a2NGWlh1UEMxaGc1YTZaRk5KaElkRTBKNys0V0tvTEdiN2ZvZGxKS0o2U1FD?=
 =?utf-8?B?TXRUQW8xMnhEVUVSb0FrOWVvcENXcmpYTDRuT3VpdUMzMmFjczNBdlZ2NDlR?=
 =?utf-8?B?djFKT2tKS3VJbmlIOEp0QnlPcUN6UCtUODIrUU4vUCtFUGlZNmhRZFJneTlD?=
 =?utf-8?B?YVFGdnRYUzVueGs0bUNMV1kramE4M3gyMkhoWjBPQkVnYXRIUGR5aEtIRWhq?=
 =?utf-8?B?b0E1Y3FodFYydjh1alpCRE1YUWdsWHozZGtRZFBxTExieFR0Y2U4V0MwWHJP?=
 =?utf-8?B?Y0MwN3RRbzR1UDZSbG5VM0I2Zk9NM3gvZTRqUURyRWVzbktiaE14VEJzMHIy?=
 =?utf-8?B?QUQzbml0Zmk4TkpWbCtiZjlGUm9MR3J0N2Zaa2wvZzlhbUxHK28wNGpwQURY?=
 =?utf-8?B?OHA4MkJ4TnJMUlI5S2gzWm8ySUQwUkhSOFEzVWNVMTFDdnVEUjNSbVBnZzE4?=
 =?utf-8?B?NXFpWDhsVzhrMS9FM0hFZGM0NGtHblk1ZS9EUmhmeExERE0xeTlwUzhVOFVL?=
 =?utf-8?B?aXA0VThNenhpSHdXUVMrd2l4bHNjdHpXTk5EeXlTcGZzbCt5d3hLdGU3V0xO?=
 =?utf-8?B?R0hlMHEzQUkrZjJoRWhTOWlsUUcrZ0pScC9JanR4TUhPamlocFRnUmJWWlJB?=
 =?utf-8?B?QVp2MlVxTC9oQWJZZ2xtSk8veTN2dEVLZzVIS09Obk9Mbmk2aWFmazJSVS9u?=
 =?utf-8?B?c2VaME4zRHRVMXhqYTZ6YTRXSlNrclZqL1VhajlxNFYvc3ZlUDBDS2VTNVFz?=
 =?utf-8?B?OFYvOUxza2QvdWNpUFJ5bjlQUWRNWlVNUlNPNDVLdFd2VDNxQW91RjdER3Zv?=
 =?utf-8?B?dFA3aXhPMGRlS3VNZ0szUWxNY1dVTVpJNkpLV0ZmN2t5OEpMeC8ydithbkl5?=
 =?utf-8?B?Vi8wSDRPcXU3RVIza0RtV2ZvTDlpcWdpUXk5UkNjNFlXZ3NWMHlzMnRrT1dL?=
 =?utf-8?B?Ti82cURSVTVTcUoyeEhCMlZMNncyY1lHRUxRVVUvNkNibWl4TzQ3Y3l4cFhL?=
 =?utf-8?B?K1ZjOGlTdmJ3NS9CdXVkTk5kZUZkdG1zekFCYzV1LzJYTlBCZXQwRkFjUklp?=
 =?utf-8?B?eDZZYUJmNHIzY0ROT1V4S2hremMvbHVxS1NxMXYvTlFCOVVFaFovUkNhK3VB?=
 =?utf-8?B?MHBmNUFPbmNoN3kyc1hFMC9aYkYwMG0vZ1VNN3lpYUpXSU01dXhJQ1o0YUlD?=
 =?utf-8?B?WldBTHBIUmp2ZVZFRy90N2Y4NGRJUSsrOUVsZzhVcUR3bEhKcGtvVnlMMmI1?=
 =?utf-8?B?TjZCNHhLcURETU83WDFyeXpJRkdHUThqa3ZtUU0rRlc3M0JpOFNVM25BeGZ2?=
 =?utf-8?B?bVU2UHMvclhVc3ExMkRyNytwRXRFS0ZEMnBjaWQ3M09SWjViRjE4bXJJMHE5?=
 =?utf-8?B?NFFicVZpTytXTmFnTDFpclVuNGFjWHp6SmVVU2NjN2gyVG10U21Sb0k0enUz?=
 =?utf-8?B?aDJONEdWVUc3eU9GQ1JPb2Z0K3ZzL3NDTDVibVlPdFh3Zkk5SllDNzV6U3hX?=
 =?utf-8?B?M0xvSUl3V3FLMWFkbllKVTVBbzdHSFFYZnRabkZqWjJGSmIyY3RIaHNmZUE1?=
 =?utf-8?B?M3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 73ac9017-c7ac-4676-8964-08db87cecca6
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2023 20:37:27.0838
 (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: q8pVogmtUI+QdEL3fu+qSnh+k+qHTGo4QjkGwVoCLlvZvh/xy8HNUcfTOKwKDNvzupiMVkSJn838rM61BBCGqGbfQD9mnSoeMWC1f9lvMFE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7576
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 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_BLOCKED,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 Tony,

Regarding subject and change: Why is the focus on just monitor.c?
Hardcoding of RDT_RESOURCE_L3 as monitoring resource is done
elsewhere also (rdtgroup.c:rdt_get_tree()) - why not treat all
hardcoding?

On 7/13/2023 9:32 AM, Tony Luck wrote:

...

> @@ -759,9 +761,9 @@ static struct mon_evt mbm_local_event = {
>  /*
>   * Initialize the event list for the resource.
>   *
> - * Note that MBM events are also part of RDT_RESOURCE_L3 resource
> - * because as per the SDM the total and local memory bandwidth
> - * are enumerated as part of L3 monitoring.
> + * Monitor events can either be part of RDT_RESOURCE_L3 resource,
> + * or they may be per NUMA node on systems with sub-NUMA cluster
> + * enabled and are then in the RDT_RESOURCE_NODE resource.
>   */
>  static void l3_mon_evt_init(struct rdt_resource *r)
>  {
> @@ -773,6 +775,8 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>  		list_add_tail(&mbm_total_event.list, &r->evt_list);
>  	if (is_mbm_local_enabled())
>  		list_add_tail(&mbm_local_event.list, &r->evt_list);
> +
> +	mon_resource = r;
>  }
>  

This does not seem like the right place for this initialization.
mon_evt_init() has a single job that the function comment clearly
states: "Initialize the event list for the resource". What does
the global mon_resource have to do with the event list?

Would get_rdt_mon_resources() not be more appropriate?

Although, looking ahead it is not clear to me why this is needed.
I'll try to focus my responses to the individual patches in this
regard.

>  int __init rdt_get_mon_l3_config(struct rdt_resource *r)

Reinette