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: <72d7ca0d-7235-2ac5-4639-38d27aafa222@intel.com>
Date:   Fri, 1 Apr 2022 15:55:13 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.7.0
Subject: Re: [PATCH v3 19/21] x86/resctrl: Rename and change the units of
 resctrl_cqm_threshold
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        Jamie Iles <jamie@nuviainc.com>,
        "D Scott Phillips OS" <scott@os.amperecomputing.com>,
        <lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
        <tan.shaopeng@fujitsu.com>
References: <20220217182110.7176-1-james.morse@arm.com>
 <20220217182110.7176-20-james.morse@arm.com>
 <87c00fe2-e4fc-b006-f608-3dc2a209ed77@intel.com>
 <1d4220ef-277d-fbb0-edb7-14f09bae0c23@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <1d4220ef-277d-fbb0-edb7-14f09bae0c23@arm.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?S1Y4THpnZW9CM0tIdVhNczNJemNGekVhMHovRzM5UnJMSDFXWjNudThxc2x2?=
 =?utf-8?B?a2ZKNnVtS25aNHROK3pzRy9PV3lUL20zSkhQajdEMHdJQms0QlRjbXJDQU1E?=
 =?utf-8?B?OHFZbGZhWXR0M0NKbk0wNWdoa1NEUk1ENk5DOEhpbWFOMVpyUDlJQ0pGQ3lW?=
 =?utf-8?B?QndsNk1TSzYzZS9rM29zU1dKRnMwWlV3eFRmeG1FUVBXRVlNbVFaaC9SelNz?=
 =?utf-8?B?QkFjQkRlZ3g3OWEzbEMvVTEzM0gzcmZPTUJ0NUxsU0VEQnNvWDhZckQ3U2ZK?=
 =?utf-8?B?L1Rha2l5OFp3L3pzY0o0SjVTcVMvUno5aWVWOE1mTEhVbDA3d01JcWlQUTFj?=
 =?utf-8?B?SlIyUnVIdGdONjZWZFRhY1N4cjFtTDI2VnNoZURVMmRZaFVVd0N3Q2hyRFJh?=
 =?utf-8?B?MlA2RUVkVDluaW5aQy96enpYTUc1cXJ5NHJES2llNS9oUTZ2MHFXUFhkMkd3?=
 =?utf-8?B?bXltRVk4a1JmL05wK0oydk90NUkxVDl6WFdPc1BYV0pqbm1YRVVwNWJ3Y2pt?=
 =?utf-8?B?eUx1dTNBVUM1MUE4elJEOVBuNDAzeUtlT2VCK1JDb05OekwvUDU4VUNtOHFL?=
 =?utf-8?B?dEd4M3U5VGI3R3Z6TmJmaisvSzlaZWFWcW9FVWRRcGpVUHJZb1praEQ3eGIy?=
 =?utf-8?B?Q3VJbFhpdzlYQVNYWUxnRWZaQjVnWHgwdjB3RktqcHo0RTYwWlhCUWdYRUMr?=
 =?utf-8?B?WlgybnEyWjBxZ1gwTy9IcXNYNXQ3bnc2NkhBMWFySS8zak9GeGVIa2pxaTZ0?=
 =?utf-8?B?NWYxYzdBQk1VVnB5K245dE9YSFFZamROcXB2QWc5NjNYV3dDeEUzZVRPVXd1?=
 =?utf-8?B?SFBPOWFWY2tXMEhOOEx0ditEdHQ4N3ZzMlpkSkczdmM4K0pSZitweEVCbkxk?=
 =?utf-8?B?WHhOY0hvQzlSelh6bXBDdjVJelBqS1dOQTV3eW4vQ0JndW4xRnNrbkg0VEtG?=
 =?utf-8?B?QVRwYWxiSDBKYlVHL3VtOHA4ZUVtSm5OUUlyMmwvNWhtbmU4eWw4UUtIV1FF?=
 =?utf-8?B?S3EzNkVIOW10bmFud1V1eTdUb0RwbjZOSEN3bXBmTlp3Y25yd09lL0M3c2xq?=
 =?utf-8?B?RzYvS2ZZdldBbi8wdTdjd1FLUjF6UGhSaTY5bEtMUC9JeGVvRUtDK05GWUVB?=
 =?utf-8?B?S3M3TE1tSU1tRDF3ZmN3RTMvUUJjVGplZFViQlVIVWxnTythSElGa2Y0aHIy?=
 =?utf-8?B?NXF6cG1BWjZpWmNMVFE3MTh4SGdlWDJlNzRSNkorbkREaElBSnpUYm5LTDNP?=
 =?utf-8?B?TktjYWNJSENaZlZtRkxFWFdtM2c3R1JkU2VmY3poWnZoY0g0VTBVOWlkZENU?=
 =?utf-8?B?ekVsVGQyL1dsZmZpRGJob24wTnlpRS94RHdwYk5oSzcwQUxEVjU5VDRrbGZU?=
 =?utf-8?B?a3JVM2ZFZGN4bHNHdVlvNGUxWnhYZ2VPNHBVZmRDZ2FJdSsyY0hueVRGQU83?=
 =?utf-8?B?TFB6akpnQjZxY01VQUVjeFNLUW93ZXErOUJkdWs1b3lRNWdZdXd1cTN0cGRP?=
 =?utf-8?B?KytTNlhzeTlJQ3pKNVhmZzdiSXk2NFExUlVOeU9yZyt6UlBiazFlZ2RGc0Fi?=
 =?utf-8?B?N3VTdy8zNDdldklWSCsxdVR1L2pocktVQ2lpQVNXRFVvY1QyS0tiblQ4eFBP?=
 =?utf-8?B?RTA1VmpWM2FOL2U1R29ENXZ6UVdXbUhINGpBZVNJY3BKeXliME92SlpINW55?=
 =?utf-8?B?YXlxSlF5V1N2Qnd3OFl5YjFyb1ErUkMyNlJVNjI4ck03MVgyZlFzZ01SM050?=
 =?utf-8?B?VTF3NFBiM1Zyb0thVGM3SDRGTmlYOEVBaHdZb0ZVWlJvVHdNWGdKNm9GRmhn?=
 =?utf-8?B?b1NicDRrSDhxV0pHTDFvaXp3cHRMTlg3K2FWM3RLdUNxbHJrQnlTMDdqRk1F?=
 =?utf-8?B?cjdENHRTZm9meTE1Z0gvaXNtN2J4RUdOU0JXZ0pjeXpwMmRDbHY0S2hQRVNL?=
 =?utf-8?B?VSs1a0VMR3R3RWk3UUtJbUEwdzdHY3FOdXdOb25zUGlPclZtWEtkWjh6Y2xZ?=
 =?utf-8?B?OUl4amtlTG5sRGtBYk1yTm9NeHZ2aEJ6TkxZYis2ZGVHdEtXenF3a3cyRkgz?=
 =?utf-8?B?NkZSY3lQd1VYclE1azNkbFR6bHdwTmo0bFdsTElPT0c5WlAveHM2UXJrakpP?=
 =?utf-8?B?ZzUyMzZMbklwN3ZhZnZEazkxempVeTdZd2t3WS9RRGNDVGpKUmI0eSsvbytt?=
 =?utf-8?B?bkl5czFFL1dwWmc5cVlYL2NqV2s1aDR2OERmREh1N0xzeEZ2Q2h5d2RDR0pK?=
 =?utf-8?B?Mzk4Zi9TdnNCVEJDYU5FVEYxSDdxSnVCSlZRUTNJamoxMDF5T3FXaUVqWFJH?=
 =?utf-8?B?WUgydVVWdkEvZTA4WFZYMDdNZ2lUVE14WkpKTGZiYk44UGNCaUR0aXlhSDlO?=
 =?utf-8?Q?uoWJP4hZ8J8GIyFY=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c26e22f-babd-47cd-8494-08da1432b096
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Apr 2022 22:55:17.1483
 (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: uAegrgXXlhwTnzX332cpdA/qZYnFKa+eNjSJOsnqNePkMnZJSy7gUaTohyz0xNWccEKyR6TZnUy6vIj7HC5p1JrGD7dXSavPW01vpDvm1no=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB4859
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.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_LOW,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 James,

On 3/30/2022 9:45 AM, James Morse wrote:
> Hi Reinette,
> 
> On 17/03/2022 17:00, Reinette Chatre wrote:
>> On 2/17/2022 10:21 AM, James Morse wrote:
>>> resctrl_cqm_threshold is stored in a hardware specific chunk size,
>>> but exposed to user-space as bytes.
>>>
>>> This means the filesystem parts of resctrl need to know how the hardware
>>> counts, to convert the user provided byte value to chunks. The interface
>>> between the architecture's resctrl code and the filesystem ought to
>>> treat everything as bytes.
>>>
>>> Change the unit of resctrl_cqm_threshold to bytes. resctrl_arch_rmid_read()
>>> still returns its value in chunks, so this needs converting to bytes.
>>> As all the callers have been touched, rename the variable to
>>> resctrl_rmid_realloc_threshold, which describes what the value is for.
> 
>>> @@ -762,10 +763,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>>>  	 *
>>>  	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
>>>  	 */
>>> -	resctrl_cqm_threshold = cl_size * 1024 / r->num_rmid;
>>> -
>>> -	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
>>> -	resctrl_cqm_threshold /= hw_res->mon_scale;
>>> +	resctrl_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid;
>>>  
>>>  	ret = dom_data_init(r);
>>>  	if (ret)
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 7ec089d72ab7..93b3697027df 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -1030,10 +1030,7 @@ static int rdt_delay_linear_show(struct kernfs_open_file *of,
>>>  static int max_threshold_occ_show(struct kernfs_open_file *of,
>>>  				  struct seq_file *seq, void *v)
>>>  {
>>> -	struct rdt_resource *r = of->kn->parent->priv;
>>> -	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>> -
>>> -	seq_printf(seq, "%u\n", resctrl_cqm_threshold * hw_res->mon_scale);
>>> +	seq_printf(seq, "%u\n", resctrl_rmid_realloc_threshold);
>>>  
>>>  	return 0;
>>>  }
>>
>>
>> This change has some user visible impact that I am still digesting but thought
>> that I would share for your consideration.
>>
>> As seen in the above two snippets, the original code did:
>>
>> resctrl_cqm_threshold /= hw_res->mon_scale; /* resctrl_cqm_threshold used internally */
>>
>> resctrl_cqm_threshold * hw_res->mon_scale; /* this is displayed to user */
>>
>> The original loss due to truncation during the division is not recovered
>> when the value is displayed to the user the user may see significant differences
>> before and after this patch.
>>
>> I tried this out on a system with a large cache and the before and after 
>> information is significant:
>> Before this patch:
>> info/L3_MON/max_threshold_occupancy:147456
>>
>> After this patch:
>> info/L3_MON/max_threshold_occupancy:196608
> 
> Hmm. I hadn't considered that information would be lost by the current way of doing this.
> It looks like this happens because num_rmid isn't necessarily a power of 2.
> 
> 
>> As I understand this change indeed represents the information more accurately but
>> I found it noteworthy that this is not just a simple "change the units" and
>> may thus have broader impact and may indeed result in different behavior that
>> should be considered.
> 
> I agree it more accurately reflects resctrl's calculation of "the number
> of lines tagged per RMID if all RMIDs have the same number of lines", but if that
> produces a number the hardware will never actually measure, then the rounding is still
> happening, but somewhere else.
> 
> I think the right thing to do is round resctrl_rmid_realloc_threshold down to the nearest
> multiple of hw_res->mon_scale in rdt_get_mon_l3_config(). This way the filesystem parts
> still handle things in bytes, and the architecture code provides the quantised value that
> will actually get measured. Its this value that should be reported to user-space.
> 
> It doesn't look like the 'Upscaling Factor' is guaranteed to be a power of 2, so I can't
> use the round_down() helpers.
> 
> I've added this to the commit message:
> | Neither r->num_rmid nor hw_res->mon_scale are guaranteed to be a power
> | of 2, so the existing code introduces a rounding error from resctrl's
> | theoretical fraction of the cache usage. This behaviour is kept as it
> | ensures the user visible value matches the value read from hardware
> | when the rmid will be reallocated.
> 
> and the hunk below, which fixes it for me.
> 
> 
> 
> Thanks,
> 
> James
> 
> ---------------%<---------------
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index b18e227d585c..fb81d650c457 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -753,6 +753,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>         unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>         struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>         unsigned int cl_size = boot_cpu_data.x86_cache_size;
> +       u64 threshold;
>         int ret;
> 
>         hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale;
> @@ -771,7 +772,15 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>          *
>          * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
>          */
> -       resctrl_rmid_realloc_threshold = cl_size * 1024 / r->num_rmid;
> +       threshold = cl_size * 1024 / r->num_rmid;
> +
> +       /*
> +        * Because num_rmid may not be a power of two, round the value
> +        * to the nearest multiple of hw_res->mon_scale so it matches a
> +        * value the hardware will measure. mon_scale may not be a power of 2.
> +        */
> +       threshold /= hw_res->mon_scale;
> +       resctrl_rmid_realloc_threshold = threshold * hw_res->mon_scale;
> 
>         ret = dom_data_init(r);
>         if (ret)
> ---------------%<---------------

Thank you for the added explanation. From what I can tell this also restores current
behavior.

Reinette