linux-kernel - Re: [PATCH 2/6] selftests/resctrl: Ensure measurements skip initialization of default benchmark

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0ae6d28f-0646-48b2-a4e7-17e2d14f6dd5@intel.com>
Date: Fri, 30 Aug 2024 09:00:17 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>, <tony.luck@...el.com>,
	<peternewman@...gle.com>, <babu.moger@....com>,
	Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>,
	<linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/6] selftests/resctrl: Ensure measurements skip
 initialization of default benchmark

Hi Ilpo,

Thank you for taking a look.

On 8/30/24 3:56 AM, Ilpo Järvinen wrote:
> On Thu, 29 Aug 2024, Reinette Chatre wrote:
> 

...

>> @@ -684,11 +622,13 @@ int resctrl_val(const struct resctrl_test *test,
>>   		const char * const *benchmark_cmd,
>>   		struct resctrl_val_param *param)
>>   {
>> -	struct sigaction sigact;
>> -	int ret = 0, pipefd[2];
>> -	char pipe_message = 0;
>> -	union sigval value;
>> -	int domain_id;
>> +	int domain_id, operation = 0, memflush = 1;
>> +	size_t span = DEFAULT_SPAN;
>> +	unsigned char *buf = NULL;
>> +	cpu_set_t old_affinity;
>> +	bool once = false;
>> +	int ret = 0;
>> +	pid_t ppid;
>>   
>>   	if (strcmp(param->filename, "") == 0)
>>   		sprintf(param->filename, "stdio");
>> @@ -699,111 +639,80 @@ int resctrl_val(const struct resctrl_test *test,
>>   		return ret;
>>   	}
>>   
>> -	/*
>> -	 * If benchmark wasn't successfully started by child, then child should
>> -	 * kill parent, so save parent's pid
>> -	 */
>>   	ppid = getpid();
>>   
>> -	if (pipe(pipefd)) {
>> -		ksft_perror("Unable to create pipe");
>> +	/* Taskset test to specified CPU. */
>> +	ret = taskset_benchmark(ppid, uparams->cpu, &old_affinity);
> 
> Previously only CPU affinity for bm_pid was set but now it's set before
> fork(). Quickly checking the Internet, it seems that CPU affinity gets
> inherited on fork() so now both processes will have the same affinity
> which might make the other process to interfere with the measurement.

Setting the affinity is intended to ensure that the buffer preparation
occurs in the same topology as where the runtime portion will run.
This preparation is done before the work to be measured starts.

This does tie in with the association with the resctrl group and I
will elaborate more below ...

> 
>> +	if (ret)
>> +		return ret;
>>   
>> -		return -1;
>> +	/* Write test to specified control & monitoring group in resctrl FS. */
>> +	ret = write_bm_pid_to_resctrl(ppid, param->ctrlgrp, param->mongrp);
> 
> Previously, this was done for bm_pid but now it's done for the parent. I'm
> not sure how inheritance goes with resctrl on fork(), will the forked PID
> get added to the list of PIDs or not? You probably know the answer :-).

Yes. A process fork()ed will land in the same resctrl group as its parent.

> Neither behavior, however, seems to result in the intended behavior as we
> either get interfering processes (if inherited) or no desired resctrl
> setup for the benchmark process.

There are two processes to consider in the resource group, the parent (that
sets up the buffer and does the measurements) and the child (that runs the
workload to be measured). Thanks to your commit da50de0a92f3 ("selftests/resctrl:
Calculate resctrl FS derived mem bw over sleep(1) only") the parent
will be sleeping while the child runs its workload and there is no
other interference I am aware of. The only additional measurements
that I can see would be the work needed to actually start and stop the
measurements and from what I can tell this falls into the noise.

Please do keep in mind that the performance counters used, iMC, cannot actually
be bound to a single CPU since it is a per-socket PMU. The measurements have
thus never been as fine grained as the code pretends it to be.

>> +	if (ret)
>> +		goto reset_affinity;
>> +
>> +	if (param->init) {
>> +		ret = param->init(param, domain_id);
>> +		if (ret)
>> +			goto reset_affinity;
>>   	}
>>   
>>   	/*
>> -	 * Fork to start benchmark, save child's pid so that it can be killed
>> -	 * when needed
>> +	 * If not running user provided benchmark, run the default
>> +	 * "fill_buf". First phase of "fill_buf" is to prepare the
>> +	 * buffer that the benchmark will operate on. No measurements
>> +	 * are needed during this phase and prepared memory will be
>> +	 * passed to next part of benchmark via copy-on-write. TBD
>> +	 * how this impacts "write" benchmark, but no test currently
>> +	 * uses this.
>>   	 */
>> -	fflush(stdout);
> 
> Please don't remove fflush() in front of fork() as it leads to duplicating
> messages.
> 

Indeed. Shaopeng just fixed this for us. Thank you!.

Reinette