Received-SPF: pass (google.com: domain of linux-kernel+bounces-192651-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <700e3df4-4e10-4870-a1df-49d4616cbc45@intel.com>
Date: Tue, 28 May 2024 08:15:40 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 02/16] selftests/resctrl: Calculate resctrl FS derived
 mem bw over sleep(1) only
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC: <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>, "Babu
 Moger" <babu.moger@amd.com>, =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?=
	<maciej.wieczor-retman@intel.com>, LKML <linux-kernel@vger.kernel.org>,
	Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <skhan@linuxfoundation.org>
References: <20240520123020.18938-1-ilpo.jarvinen@linux.intel.com>
 <20240520123020.18938-3-ilpo.jarvinen@linux.intel.com>
 <04d0a5d6-82fa-4cc7-bd80-ee5cbd35f0c3@intel.com>
 <ea0c86b9-ae77-c2d9-b52b-239ae42603e8@linux.intel.com>
 <d8063ee7-1744-45a2-b6b9-506e68106baf@intel.com>
 <b029db88-2e09-0b4a-f46a-84b5e535f178@linux.intel.com>
 <904aa442-9fc5-c6dd-f367-07b197085f7b@linux.intel.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <904aa442-9fc5-c6dd-f367-07b197085f7b@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?M2JOSzN6K0lYQWlSekNFdklwNjEyNmdHQlpQaFRDWWtXNjNzRTF2aHBQL3FY?=
 =?utf-8?B?d0VXeXZtM0JHMHNzQitiZjVZcmJRdUlwSzBOeG4zVmtuRmE3MjAwcVZpek9Y?=
 =?utf-8?B?ejhjbVFNOER4bjhVZkNnUmp1amZ2dFdzZG9YY29hblFkRERGeXoyZTVFTlJT?=
 =?utf-8?B?L1d2UjcwVldXNDdjVlh4Rllkell5L3NTdkNnME9vNEw5eFFpVHBoY3AyQ3Ba?=
 =?utf-8?B?MDRaMThCMWF3OTJ0bElaclVEL2YvcEwvbGhRTE42VkQyVDlyUFg4NEFSWXlH?=
 =?utf-8?B?NE1Yb0R3ckM0RmlGa05qY2xWRlkrNmhhK1M5R29yRDROOVZxT0JMZGJ4VXFj?=
 =?utf-8?B?VHNwZXc2Q3pGZ3dmeWNrTzJ4Q0QrNXB3bnZCRDFXZVdoTmFINkZ1TTNkbGVJ?=
 =?utf-8?B?WWNENFYxaGVsVVNaOHZyempoWDAvWWczcHlhc3pWMmdJdWlLRk1HbzBSYk50?=
 =?utf-8?B?R21NSVpEM0hHNkRnQkxOcXR3YUdCMEF3QW9JaHU3MHRmU1g2dlNZekNkZWtN?=
 =?utf-8?B?S21uTnBxTEU3UmYzRExtU1V0TUhWa1dwMFZIeE1SdUNRSDNyVkUwQ2M5WU5B?=
 =?utf-8?B?cElET1VYZGIwRENZNGZPUnoyL2VON1lhbW9NK05jUXEra1pUWlYyVEdtSFV2?=
 =?utf-8?B?N3l1QjRkM0hxdGszU1dzSitlTE4yT1laZXNMOEZWc0NLWUJBMHRKeDZkMWNU?=
 =?utf-8?B?MEpvSklqSE9SMm9hSFZSQ1pyNG9iMHZFYkFaUGhvU09yaFcyU0pYN2o0ZzZX?=
 =?utf-8?B?VFF1VGljUWpid2FCdTZ0UkxRUFZLQjZ3eTRqaSt6VzBLK2pPT0l1VWNXMGlQ?=
 =?utf-8?B?Sjg2V0NtQksvMC9oc0l2OFhiQnFidWhGR1dwYS85MnhHTXJjaVc5MmhmMS9P?=
 =?utf-8?B?TThON0tscGo5MzJWUktCa205TTVnMVFLQ2RBRUNyT3ZEWTEvREhKTDdBcHpx?=
 =?utf-8?B?TnlIRXpBT3FHalVJenpwZHdrS2lBeVU5YWtsaVFhNGoyS0p3VWREME5LYU1T?=
 =?utf-8?B?T3NLRit2U2k3YW9BT2dMRG9SU21WdVc5UnRvM21mekNtWVhocXFUSXo5UHRK?=
 =?utf-8?B?b3gxcXVJY3gwOHRFTDNJQ0cvOVhqczM4bUc2dk00VEMzSU1keS9LWXcrenFi?=
 =?utf-8?B?ZUJ3Vm5sYlptQTE0K1g0SmUxL085S2dxUkFPemFOWjBFeFNIcCtHMitxN0M2?=
 =?utf-8?B?U0NscnRNSHEyUlUyeThGUC9PelFsaFZWRUZ4OG5ZTEt4TFRVUmJxVFp5NHcw?=
 =?utf-8?B?UG41T0tVTmhkWUtRYWZHVVdmSUNZUHF0VUlmWEVsUFc1ODQ1RnI0RjJRWFZW?=
 =?utf-8?B?cnIySkJmSFV6WVAxT01XVk44V2Y1WXJiVmJBR3JIQitDUXVRN3cveWNxcHdU?=
 =?utf-8?B?SjRmMElqc2F3MlQ5RHlSdTV2UHhVUGxzanAzN1B5c2c5Qy9VQStLTDA5ekhO?=
 =?utf-8?B?cTVQMzJEYzNMWmNFaVY0cHFOYmZFK2ZhdWVIRVh5QnBYbGl3aGNjMEt5WUln?=
 =?utf-8?B?SnVZZHN6dUM0eC82a1lVc3FJcXJFZWRBUkNaU2xwOTF0aVlBZVh0ZjMwQTFH?=
 =?utf-8?B?ZVdmNzNabDRyMDg3dnIxbGVRbXBkc1ZtSGgwckNab0hhUkJkZ0pYR2Z0S1F1?=
 =?utf-8?B?SktIT3ROSWF5WVVsTXJqRXFENDNEUTNBNzFmVEt4RjNmZVVPYVBTZGRQcDdx?=
 =?utf-8?B?M1FtOXJyOEZqaVYrVzdMeHFkbVE2SVVFNTdGMlRLcE1CL09iVDc4VDVhM3BC?=
 =?utf-8?B?aFMybm9CUVA2a2dtTlFTT09JRzRzWVFCVHhKZlNFVDlMNi9yR2pnTk05Mk5s?=
 =?utf-8?B?ZUxGZ3RUbDdQMldzRnBuQTVlNmFxSnJySGVUZWVITnVVZFBiUHIxbkxZYjIx?=
 =?utf-8?B?OTMxTE55N21veDB2YnZoQXhkRkFRK25JTmZBaWNXQVVQYVh1LzE4Zk92L3I2?=
 =?utf-8?B?bDNqbEEzZDFSQXdGdkFtTklTVUtNOHpjQlYxdkN0akpVM1NxUkV0cm9zTHVE?=
 =?utf-8?B?ZXNnSFhZN3cyTWNWNXloQVR5T3ZOK0NNUXVVaHlTOHpvSzBzUkFJRkdQa3Fy?=
 =?utf-8?B?NHZHTUYxSjNFZE0xbkgyek5PNzFyLy9UQzVOUUVMYmgxODZWUkxaLzlQbi83?=
 =?utf-8?B?eG5yUkpndjFsZEIxTUttWjMxbmdya1IvcXZPRmNPY21lTlo2NzNXT0tVb1Fv?=
 =?utf-8?B?YUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f9557854-7c2c-4ef9-ace4-08dc7f290ab1
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 May 2024 15:15:43.1375
 (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: HXeUo+djZuDDsoT5v3FURZQZOUyqbVU7DnOuQr0FqYKp/VGzhrwBOokguwtrW/gu7bnloxUxkauFo4SyLiZgVYpHXMlxMC58Kxw59IKl0FE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5031
X-OriginatorOrg: intel.com

Hi Ilpo,

On 5/28/24 3:19 AM, Ilpo Järvinen wrote:
> On Fri, 24 May 2024, Ilpo Järvinen wrote:
> 
>> On Fri, 24 May 2024, Reinette Chatre wrote:
>>> On 5/24/24 12:57 AM, Ilpo Järvinen wrote:
>>>> On Thu, 23 May 2024, Reinette Chatre wrote:
>>>>> On 5/20/24 5:30 AM, Ilpo Järvinen wrote:
>>>>>> For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
>>>>>> the measurement over a duration of sleep(1) call. The memory bandwidth
>>>>>> numbers from IMC are derived over this duration. The resctrl FS derived
>>>>>> memory bandwidth, however, is calculated inside measure_vals() and only
>>>>>> takes delta between the previous value and the current one which
>>>>>> besides the actual test, also samples inter-test noise.
>>>>>>
>>>>>> Rework the logic in measure_vals() and get_mem_bw_imc() such that the
>>>>>> resctrl FS memory bandwidth section covers much shorter duration
>>>>>> closely matching that of the IMC perf counters to improve measurement
>>>>>> accuracy. Open two the resctrl mem bw files twice to avoid opening
>>>>>> after the test during measurement period (reading the same file twice
>>>>>> returns the same value so two files are needed).
>>>>>
>>>>> I think this is only because of how the current reading is done, resctrl
>>>>> surely supports keeping a file open and reading from it multiple times.
>>>>>
>>>>> There seems to be two things that prevent current code from doing this
>>>>> correctly:
>>>>> (a) the fscanf() code does not take into account that resctrl also
>>>>>       prints a "\n" ... (this seems to be the part that may cause the same
>>>>>       value to be returned).
>>>>>       So:
>>>>> 	if (fscanf(fp, "%lu", mbm_total) <= 0) {
>>>>>       should be:
>>>>> 	if (fscanf(fp, "%lu\n", mbm_total) <= 0) {
>>>>> (b) the current reading does not reset the file position so a second
>>>>>       read will attempt to read past the beginning. A "rewind(fp)"
>>>>>       should help here.
>>>>
>>>> (b) cannot be the cause for returning the same value again. It would
>>>> not be able to reread the number at all if file position is not moved.
>>>
>>> I know. This was not intended to explain the duplicate answer but instead
>>> describe another change required to use current code in a loop. I
>>> specifically said in (a) that "(this seems to be the part that may cause
>>> the same value to be returned)".
>>>
>>>> I certainly tried with fseek() and it is when I got same value on the
>>>> second read which is when I just went to two files solution.
>>>>
>>>>> A small program like below worked for me by showing different values
>>>>> on every read:
>>>>>
>>>>> #include <stdio.h>
>>>>> #include <stdlib.h>
>>>>> #include <unistd.h>
>>>>>
>>>>> const char *mbm_total_path =
>>>>> "/sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes";
>>>>>
>>>>> int main(void)
>>>>> {
>>>>> 	unsigned long mbm_total;
>>>>> 	FILE *fp;
>>>>> 	int count;
>>>>>
>>>>> 	fp = fopen(mbm_total_path, "r");
>>>>> 	if (!fp) {
>>>>> 		perror("Opening data file\n");
>>>>> 		exit(1);
>>>>> 	}
>>>>> 	for (count = 0; count < 100; count++) {
>>>>> 		if (fscanf(fp, "%lu\n", &mbm_total) <= 0) {
>>>>> 			perror("Unable to read from data file\n");
>>>>> 			exit(1);
>>>>> 		}
>>>>> 		printf("Read %d: %lu\n",count ,mbm_total );
>>>>> 		sleep(1);
>>>>> 		rewind(fp);
>>>>> 	}
>>>>> 	fclose(fp);
>>>>> 	return 0;
>>>>> }
>>>>
>>>> Okay, so perhaps it's your explanation (a) but can libc be trusted to not
>>>> do buffering/caching for FILE *? So to be on the safe side, it would
>>>
>>> Coding with expectation that libc cannot be trusted sounds strange to me.
>>>
>>>> need to use syscalls directly to guarantee it's read the file twice.
>>>>
>>>> If I convert it into fds, fscanf() cannot be used which would complicate
>>>> the string processing by adding extra steps.
>>>>
>>>
>>> It is not clear to me why you think that fscanf() cannot be used.
>>
>> This was related to fscanf() not being able to read from an fd which is
>> different interface than what libc's FILE * is.

The part I am missing is why you believe syscalls are required. Could
you please elaborate why FILE * cannot be used?

>>
>>> Could you please elaborate what the buffering issues are?
>>
>> I'm pretty sure that by default libc does some buffering (even std*
>> streams are line buffered and others streams even more). I'm not entirely
>> sure about the extent of that buffering but here we need to always read
>> the up to date value from the file itself, not from some buffer.
>>
>> Maybe there never is any problem that the earlier read values are returned
>> from some libc buffer when lseek/rewind is used, I just don't know that
>> for sure. You seem to be more certain but I've not seen on what basis
>> (other than the anecdotial test you provided).

I demonstrated that it works. I have not heard a clear reason why this conclusion
is incorrect. The above remains vague to me and I cannot find a description of
a clear problem that can be studied.

>>
>>> It is not necessary to open and close the file every time a value needs
>>> to be read from it.
> 
> I'm bit unsure where to go with this. While I could change the code to
> match what you described, I realized with the two files approach there's
> no need to do even review/lseek() call during the measurement. It might
> not be very significant compared with the open that was there initially
> but it's still extra.
> 

We are discussing the resctrl selftests that will accompany the resctrl
filesystem in the kernel. When in doubt on how to interact with resctrl users
use the selftests as reference. Needing to open and close a resctrl file
every time a value needs to be read from it is not the correct guidance.

Reinette