Message-ID: <904aa442-9fc5-c6dd-f367-07b197085f7b@linux.intel.com>
Date: Tue, 28 May 2024 13:19:38 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.com>
cc: linux-kselftest@...r.kernel.org, Shuah Khan <shuah@...nel.org>, 
    Babu Moger <babu.moger@....com>, 
    Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>, 
    LKML <linux-kernel@...r.kernel.org>, Fenghua Yu <fenghua.yu@...el.com>, 
    Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v4 02/16] selftests/resctrl: Calculate resctrl FS derived
 mem bw over sleep(1) only

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.
> 
> > 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).
> 
> > 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.

-- 
 i.

Powered by blists - more mailing lists