Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <0cee68e9-e188-46e9-83a8-02259a9c081f@intel.com>
Date:   Fri, 3 Nov 2023 14:43:15 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] x86/resctrl: mba_MBps: Fall back to total b/w if local
 b/w unavailable
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: <20231025235046.12940-1-tony.luck@intel.com>
 <20231026200214.16017-1-tony.luck@intel.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231026200214.16017-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?dC9teGFFWnB4Y2h5TUNiTWlEOU90amtLMTRsVUFocE5BdzI5bnVSb1VVVGRH?=
 =?utf-8?B?eWJqRFk2bC95OGpRY3FnRHZsOFZCSERvbWdiSHN4eG4rNktzVE1oZDBFMGYz?=
 =?utf-8?B?M3hKSWVib1B4dk43UHpQYzJGTExNamYxMUNFZ0tSR2N0OEI3ektvTUs3TXo5?=
 =?utf-8?B?VkVzRzd3ZUgwVHNMZUV3STNKbGl4R09vcFBvSVRXWWtQQkJaa3AyTEZ6NkNv?=
 =?utf-8?B?b2NtSnhVVDBNMExicHk2dkpjSVZuMGlYZmVUbHRSTXJXTFFQdndRSGFrS1p6?=
 =?utf-8?B?UmYvdHZWajFremM3YjQ3SFlKYTAxZXN4N0k4NEFrVW1zVGsxdDM5NjNKMlJM?=
 =?utf-8?B?ZXRUOEI0djNCb0k1SW9xaTBhdUJtT3JLVllXWnpTYlJIdSs0RWJKc0xYZnZG?=
 =?utf-8?B?WlVEZmEyenhoRXRIU1JKMTc2YWw2eElrbGpPamV6QXZQUkVudnkzYUhuSXd4?=
 =?utf-8?B?NTlVZ3lacW5SdlJBT1kzYWsrQ2dBMUFvUE9TNStxbG1pSHZzZUVNT0pxYVRF?=
 =?utf-8?B?SmVJbVZLakRlRUltNGtPcXJqVUVvUzRSZ2pHcUFNNmJ0ZG90UHZZYXEzeUNP?=
 =?utf-8?B?UGNJaG9rYmpPQnh0VXJDVld4cHRKdFlpWXhDWEFTT0toK29sNCsvMHFIbExI?=
 =?utf-8?B?L0crOGd6ODFIUG1DZkZmanJ1U1liQ0dWdTBKekpBWEtuQ1JGL1FrY0NGamNu?=
 =?utf-8?B?SVJuQjZBczdTamppNVpuQ2IyLzRMdU1VbElrODFLQ1lreE1sTmx5ODFaTnFT?=
 =?utf-8?B?dVE0eENaenA4TnhzMEtLbEoxRkpNZ21hRXYzRmZxTXYzR1FobG1pbGNITWJJ?=
 =?utf-8?B?ZVNqcTg3Q2FPR2duVXpJaVl6dWdxWGdBWVkxa1hNTG5JVGNreTdqMVBWQU4z?=
 =?utf-8?B?ZFBBT0JQS085dFBYNWhJeHN1SnV2eURZVWNFOVFoL0RYRWtyNVNORkRkeUVH?=
 =?utf-8?B?aEt4TVZYWE05cmV5NGtTUGV2UXZLM01UOU15aXpHR1E4TkNkelViNFdnMU0w?=
 =?utf-8?B?b01TNHBMVzFSRzE5cGQ0UWJNT0RndWtSU2xvVmpuVzluTFF3VnB3SDRwVWdM?=
 =?utf-8?B?aFFhR3NrV25lY29lUG9XR3FaaWhIZTV3cDdEU1piRCtmNUhDOFEvczRzbzN6?=
 =?utf-8?B?d2NNUHlGR2MxWCtqL0plcVF0U2lXMkxUZjhvQW9OYVR1YVVVUm9rZFpXd2JO?=
 =?utf-8?B?MFovYUk3Nkc2b2FkbWtlSjJ5ZzNKVTAxcmtKZkdRMU52RTh4YStEWUVvSjhj?=
 =?utf-8?B?UExac3lmOFZQVDhSaWlwWUpHWGZvTkdOcWJmdWNZNDB2RFJ3eGFKTFRJa21U?=
 =?utf-8?B?Qm5XVU9iU3NQS2gxUWZjQ1lkak5iRldSbXd4a2RCMW16UXVNZmMwSjVoTkNz?=
 =?utf-8?B?R0swa3pYTERzTFgxVEVKSzZQRTBoTkgvN21iT21MQVc4cWljVmdOZ20vVnVn?=
 =?utf-8?B?aFZjVEJydzlGaFB6eSt1a3lsMXNkZVc3aUI5Zm83bzNXNHVyOHNleFBQQTdp?=
 =?utf-8?B?QS93STduWTZqaEJZWkFyWWRtYWRCNFU4eEIyRTJmL2tsSkFKOGMyVFV0bmxq?=
 =?utf-8?B?eklNYUtvejd3VjNZdTdrblFTS045c3Y5Yy96d2graW9BTWRkSkYxUTh2TFZK?=
 =?utf-8?B?SmdZRGJmdG1JTUFhNTlzOXk5MFRCc2xZZDhIbVlIeW51TVpRRDMxZkgyR0t0?=
 =?utf-8?B?VEk5OXp0dzdVeWNwZU5TS0JrSWJWbEsyTElaWEk0eVUyL2prR1VFZklLeWx4?=
 =?utf-8?B?MEtXem1xSm80VUd6Zmg0OWJLS3ZrYTVhUmRvaCtFVjNhOGRYNUlpeGhjTGVS?=
 =?utf-8?B?VGE0VmRRZEdRUzZaQktIMk1JYVoyb0JaMG1GNXg5Y3VYdCtZZG03b2x3cklu?=
 =?utf-8?B?OEZNSGlKK2k4VDErUlVITjRYSlk1djdhbGVVVnhkUUZWcmsyeGU1cjA3cTlw?=
 =?utf-8?B?NGVzMmtpN3FrMGdrVFdrVUNmWXhxa3VJMk5zdGE5SWxKbHZCMVp0ZDNuSVpD?=
 =?utf-8?B?MVdLdENiaWxyemNIdDRnclg4NGhhQkJiUmtoZ0RmYnA4eGliaWFJZVM5Qmls?=
 =?utf-8?B?bTgrVEJheGRKbzRJcDZHYzBPVHQwdk9DVXFpRENXRVdIcXpnbmt3WlBpOE1i?=
 =?utf-8?B?eENBSXlUVkorU0ZpWVBkQUNVeXBUS3BqTEYyMm5QamdGTUhIejhRaXdXK2Fu?=
 =?utf-8?B?ZkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 724f1f20-0fe4-4db4-b3db-08dbdcb5e48f
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2023 21:43:18.6142
 (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: gkCSI4CQxJETcVTithQ9y4/YJ514MPU2R/+mgIUK4oanPdawYoe4UBwWbN5R6631lmxOfK2gcjAoeHAONT7NW6zKxCLPIWzELIHNUhEqARo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4968
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.3 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Fri, 03 Nov 2023 14:43:36 -0700 (PDT)

Hi Tony,

On 10/26/2023 1:02 PM, Tony Luck wrote:
> On Intel the various resource director technology (RDT) features are all
> orthogonal and independently enumerated. Thus it is possible to have
> a system that  provides "total" memory bandwidth measurements without
> providing "local" bandwidth measurements.

This motivation is written in support of Intel systems but from what I
can tell the changes impact Intel as well as AMD.

> 
> If local bandwidth measurement is not available, do not give up on
> providing the "mba_MBps" feedback option completely, make the code fall
> back to using total bandwidth.

It is interesting to me that the "fall back" is essentially a drop-in
replacement without any adjustments to the data/algorithm.

Can these measurements be considered equivalent? Could a user now perhaps
want to experiment by disabling local bandwidth measurement to explore if
system behaves differently when using total memory bandwidth? What
would have a user choose one over the other (apart from when user
is forced by system ability)?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> Change since v2:
> 
> Babu doesn't like the global variable. So here's a version without it.
> 
> Note that my preference is still the v2 version. But as I tell newbies
> to Linux "Your job isn't to get YOUR patch upstream. You job is to get
> the problem fixed.".  So taking my own advice I don't really mind
> whether v2 or v3 is applied.
> 
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 43 ++++++++++++++++++--------
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  2 +-
>  2 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f136ac046851..29e86310677d 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -418,6 +418,20 @@ static int __mon_event_count(u32 rmid, struct rmid_read *rr)
>  	return 0;
>  }
>  
> +/*
> + * For legacy compatibility use the local memory bandwidth to drive
> + * the mba_MBps feedback control loop. But on platforms that do not
> + * provide the local event fall back to use the total bandwidth event
> + * instead.
> + */
> +static enum resctrl_event_id pick_mba_mbps_event(void)
> +{
> +	if (is_mbm_local_enabled())
> +		return QOS_L3_MBM_LOCAL_EVENT_ID;
> +
> +	return QOS_L3_MBM_TOTAL_EVENT_ID;
> +}

Can there be a WARN here to catch the unlikely event that
!is_mbm_total_enabled()?
This may mean the caller (in update_mba_bw()) needs to move
to code protected by is_mbm_enabled().

One option to consider is to have a single "get_mba_mbps_state()"
call (similar to V1) that determines the eventid as above and
then calls get_mbm_state() to return a pointer to mbm_state in one
call. Starting to seem like nitpicking but I'd thought I'd mention it
since it seemed a way to have V1 solution with request to use
get_mbm_state() addressed.

> +
>  /*
>   * mbm_bw_count() - Update bw count from values previously read by
>   *		    __mon_event_count().
> @@ -431,9 +445,11 @@ static int __mon_event_count(u32 rmid, struct rmid_read *rr)
>   */
>  static void mbm_bw_count(u32 rmid, struct rmid_read *rr)
>  {
> -	struct mbm_state *m = &rr->d->mbm_local[rmid];
> +	enum resctrl_event_id mba_mbps_evt_id = pick_mba_mbps_event();
>  	u64 cur_bw, bytes, cur_bytes;
> +	struct mbm_state *m;
>  
> +	m = get_mbm_state(rr->d, rmid, mba_mbps_evt_id);
>  	cur_bytes = rr->val;
>  	bytes = cur_bytes - m->prev_bw_bytes;
>  	m->prev_bw_bytes = cur_bytes;

It should not be necessary to pick the event id again. It is available
within the struct rmid_read parameter. 

Reinette