Re: [Sched-tools] [PATCH v3] rtapp: Add broader support for specifying cpusets

Hi,

On 24/02/17 17:20, Stephen Boyd wrote:

From: Olav Haugan ohaugan@codeaurora.org

Allow setting of cpusets at all levels of the spec.

Signed-off-by: Olav Haugan ohaugan@codeaurora.org Tested-by: Dietmar Eggemann dietmar.eggemann@arm.com Reviewed-by: Dietmar Eggemann dietmar.eggemann@arm.com Tested-by: Viresh Kumar viresh.kumar@linaro.org Signed-off-by: Stephen Boyd stephen.boyd@linaro.org

Apart from the very minor comments below the patch looks good to me, so

Acked-by: Juri Lelli juri.lelli@arm.com

Could you please create a pull request on the github repo so that we can easily merge this?

[...]

diff --git a/doc/examples/tutorial/example7.json b/doc/examples/tutorial/example7.json new file mode 100644 index 000000000000..093e4ab3a93b --- /dev/null +++ b/doc/examples/tutorial/example7.json @@ -0,0 +1,40 @@ +{

• /*

• * Simple use case which creates a thread with 3 phases each running for
  

• * 1.5ms. In phase 1 the thread will be affined to CPU 0, in phase 2 it
  

• * will be affined to CPU 1 and in the third phase it will be affined to
  

• * CPUs 0-3 (inherit from "cpus" at task level).
  

• */
  

• "tasks" : {

• "thread0" : {
  

• 	"cpus" : [0,1,2,3],
  

Do you think it would be simpler to put "cpus" : [2] here so that it's easier to check if default affinity is applied when looking at trace with kernelshark (task usually stays on CPU1 one during next phase otherwise).

• 	"phases" : {
  

• 		"start" : {
  

Maybe just call them "phase{1,2,3}"? Whay these names otherwise?

• 			"cpus" : [0],
  

• 			"loop" : 1,
  

• 			"run" : 1500
  

• 		},
  

• 		"stop" : {
  

• 			"cpus" : [1],
  

• 			"loop" : 1,
  

• 			"run" : 1500
  

• 		},
  

• 		"next" : {
  

• 			"loop" : 1,
  

• 			"run" : 1500
  

• 		}
  

• 	}
  

• }
  

• },
• "global" : {

• "duration" : 2,
  

• "calibration" : "CPU0",
  

• "default_policy" : "SCHED_OTHER",
  

• "pi_enabled" : false,
  

• "lock_pages" : false,
  

• "logdir" : "./",
  

• "log_basename" : "rt-app1",
  

• "ftrace" : true,
  

• "gnuplot" : true
  

• }

+}

[...]

+static void create_cpuset_str(cpuset_data_t *cpu_data) +{

• unsigned int cpu_count = CPU_COUNT_S(cpu_data->cpusetsize,

• 					cpu_data->cpuset);
  

• unsigned int i;
• unsigned int idx = 0;
• /* Assume we can go up to 9999 cpus. Each cpu would take up to 4 + 2

Start comment on new line please.

Also, do we actually want to assert cpu_count < 10000 and exit otherwise?

• * bytes (4 for the number and 2 for the comma and space). 2 bytes
  

• * for beginning bracket + space and 2 bytes for end bracket and space
  

• * and finally null-terminator.
  

• */
  

• unsigned int size_needed = cpu_count * 6 + 2 + 2 + 1;
• cpu_data->cpuset_str = malloc(size_needed);
• strcpy(cpu_data->cpuset_str, "[ ");
• idx += 2;
• for (i = 0; i < 10000 && cpu_count; ++i) {

• unsigned int n;
  

• if (CPU_ISSET(i, cpu_data->cpuset)) {
  

• 	--cpu_count;
  

• 	if (size_needed <= (idx + 1)) {
  

• 		log_error("Not enough memory for array");
  

• 		exit(EXIT_FAILURE);
  

• 	}
  

• 	n = snprintf(&cpu_data->cpuset_str[idx],
  

• 				size_needed - idx - 1, "%d", i);
  

• 	if (n > 0) {
  

• 		idx += n;
  

• 	} else {
  

• 		log_error("Error creating array");
  

• 		exit(EXIT_FAILURE);
  

• 	}
  

• 	if (size_needed <= (idx + 1)) {
  

• 		log_error("Not enough memory for array");
  

• 		exit(EXIT_FAILURE);
  

• 	}
  

• 	if (cpu_count) {
  

• 		strncat(cpu_data->cpuset_str, ", ",
  

• 					size_needed - idx - 1);
  

• 		idx += 2;
  

• 	}
  

• }
  

• }
• strncat(cpu_data->cpuset_str, " ]", size_needed - idx - 1);

+}

+static void set_thread_affinity(thread_data_t *data, cpuset_data_t *cpu_data) +{

• int ret;
• cpuset_data_t *actual_cpu_data = &data->cpu_data;
• if (data->def_cpu_data.cpuset == NULL) {

• /* Get default affinity */
  

• cpu_set_t cpuset;
  

• unsigned int cpu_count;
  

• unsigned int cpu = 0;
  

• ret = pthread_getaffinity_np(pthread_self(),
  

• 				    sizeof(cpu_set_t), &cpuset);
  

• if (ret != 0) {
  

• 	errno = ret;
  

• 	perror("pthread_get_affinity");
  

• 	exit(EXIT_FAILURE);
  

• }
  

• cpu_count = CPU_COUNT(&cpuset);
  

• data->def_cpu_data.cpusetsize = CPU_ALLOC_SIZE(cpu_count);
  

• data->def_cpu_data.cpuset = CPU_ALLOC(cpu_count);
  

• memcpy(data->def_cpu_data.cpuset, &cpuset,
  

• 				data->def_cpu_data.cpusetsize);
  

• create_cpuset_str(&data->def_cpu_data);
  

• data->curr_cpu_data = &data->def_cpu_data;
  

• }
• /* Order of preference:

Ditto.

• * 1. Phase cpuset
  

• * 2. Task level cpuset
  

• * 3. Default cpuset
  

• */

White space damage?

• if (cpu_data->cpuset != NULL)

• actual_cpu_data = cpu_data;
  

• if (actual_cpu_data->cpuset == NULL)

• actual_cpu_data = &data->def_cpu_data;
  

• if (!CPU_EQUAL(actual_cpu_data->cpuset, data->curr_cpu_data->cpuset))
• {

• log_debug("[%d] setting cpu affinity to CPU(s) %s", data->ind,
  

• 	actual_cpu_data->cpuset_str);
  

• ret = pthread_setaffinity_np(pthread_self(),
  

• 				actual_cpu_data->cpusetsize,
  

• 				actual_cpu_data->cpuset);
  

• if (ret != 0) {
  

• 	errno = ret;
  

• 	perror("pthread_setaffinity_np");
  

• 	exit(EXIT_FAILURE);
  

• }
  

• data->curr_cpu_data = actual_cpu_data;
  

• }

+}

As said, no need to respin a new version IMHO, a pull request once comments above have been addressed or considered would work, but Vincent or others could have additional comments.

Thanks a lot for this.

Best,

- Juri