All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powercap/drivers/idle_injection: Add an idle injection framework
@ 2018-05-10 10:34 Daniel Lezcano
  2018-05-10 12:26 ` [PATCH V2] " Daniel Lezcano
  2018-05-11  6:11 ` [PATCH] " kbuild test robot
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel Lezcano @ 2018-05-10 10:34 UTC (permalink / raw)
  To: rjw
  Cc: viresh.kumar, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

Initially, the cpu_cooling device for ARM was changed by adding a new
policy inserting idle cycles. The intel_powerclamp driver does a
similar action.

Instead of implementing idle injections privately in the cpu_cooling
device, move the idle injection code in a dedicated framework and give
the opportunity to other frameworks to make use of it.

The framework relies on the smpboot kthreads which handles via its
mainloop the common code for hotplugging and [un]parking.

The idle injection is registered with a name and a cpumask. It will
result in the creation of the kthreads on all the cpus specified in
the cpumask with a name followed by the cpu number. No idle injection
is done at this time.

The idle + run duration must be specified via the helpers and then the
idle injection can be started at this point.

The kthread will call play_idle() with the specified idle duration
from above and then will schedule itself. The latest CPU is in charge
of setting the timer for the next idle injection deadline.

The task handling the timer interrupt will wakeup all the kthreads
belonging to the cpumask.

This code was previously tested with the cpu cooling device and went
through several iterations. It results now in splitted code and API
exported in the header file. It was tested with the cpu cooling
device with success.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/Kconfig          |   8 +
 drivers/powercap/Makefile         |   1 +
 drivers/powercap/idle_injection.c | 330 ++++++++++++++++++++++++++++++++++++++
 include/linux/idle_injection.h    |  62 +++++++
 4 files changed, 401 insertions(+)
 create mode 100644 drivers/powercap/idle_injection.c
 create mode 100644 include/linux/idle_injection.h

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 85727ef..1ed23e3 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -29,4 +29,12 @@ config INTEL_RAPL
 	  controller, CPU core (Power Plance 0), graphics uncore (Power Plane
 	  1), etc.
 
+config IDLE_INJECTION
+	bool "Idle injection framework"
+	depends on CPU_IDLE
+	default n
+	---help---
+	   This enable support for the idle injection framework. It
+	   provides a way to force idle periods on a set of specified
+	   CPUs for power capping.
 endif
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index 0a21ef3..c3bbfee 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o
+obj-$(CONFIG_IDLE_INJECTION) += idle_injection.o
diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
new file mode 100644
index 0000000..120171f
--- /dev/null
+++ b/drivers/powercap/idle_injection.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drivers/powercap/idle_injection.c
+ *
+ * Copyright 2018 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * The idle injection framework propose a way to force a cpu to enter
+ * an idle state during a specified amount of time for a specified
+ * period.
+ *
+ */
+#include <linux/cpu.h>
+#include <linux/freezer.h>
+#include <linux/hrtimer.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smpboot.h>
+
+#include <uapi/linux/sched/types.h>
+
+/**
+ * struct idle_injection_thread - task on/off switch structure
+ * @tsk: a pointer to a task_struct injecting the idle cycles
+ * @should_run: a integer used as a boolean by the smpboot kthread API
+ */
+struct idle_injection_thread {
+	struct task_struct *tsk;
+	int should_run;
+};
+
+/**
+ * struct idle_injection_device - data for the idle injection
+ * @smp_hotplug_thread: a pointer to a struct smp_hotplug_thread
+ * @timer: a hrtimer giving the tempo for the idle injection
+ * @count: an atomic to keep track of the last task exiting the idle cycle
+ * @idle_duration_ms: an atomic specifying the idle duration
+ * @run_duration_ms: an atomic specifying the running duration
+ */
+struct idle_injection_device {
+	struct smp_hotplug_thread *smp_hotplug_thread;
+	struct hrtimer timer;
+	atomic_t idle_duration_ms;
+	atomic_t run_duration_ms;
+	atomic_t count;
+};
+
+static DEFINE_PER_CPU(struct idle_injection_thread, idle_injection_thread);
+static DEFINE_PER_CPU(struct idle_injection_device *, idle_injection_device);
+
+/**
+ * idle_injection_wakeup - Wake up all idle injection threads
+ * @ii_dev: the idle injection device
+ *
+ * Every idle injection task belonging to the idle injection device
+ * and running on an online CPU will be wake up by this call.
+ */
+static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
+{
+	struct idle_injection_thread *iit;
+	struct cpumask *cpumask = ii_dev->smp_hotplug_thread->cpumask;
+	int cpu;
+
+	for_each_cpu_and(cpu, cpumask, cpu_online_mask) {
+		iit = per_cpu_ptr(&idle_injection_thread, cpu);
+		iit->should_run = 1;
+		wake_up_process(iit->tsk);
+	}
+}
+
+/**
+ * idle_injection_wakeup_fn - idle injection timer callback
+ * @timer: a hrtimer structure
+ *
+ * This function is called when the idle injection timer expires which
+ * will wake up the idle injection tasks and these ones, in turn, play
+ * idle a specified amount of time.
+ *
+ * Always returns HRTIMER_NORESTART
+ */
+static enum hrtimer_restart idle_injection_wakeup_fn(struct hrtimer *timer)
+{
+	struct idle_injection_device *ii_dev =
+		container_of(timer, struct idle_injection_device, timer);
+
+	idle_injection_wakeup(ii_dev);
+
+	return HRTIMER_NORESTART;
+}
+
+/**
+ * idle_injection_fn - idle injection routine
+ * @cpu: the CPU number the tasks belongs to
+ *
+ * The idle injection routine will stay idle the specified amount of
+ * time
+ */
+static void idle_injection_fn(unsigned int cpu)
+{
+       struct idle_injection_device *ii_dev;
+       struct idle_injection_thread *iit;
+       int run_duration_ms, idle_duration_ms;
+
+       ii_dev = per_cpu(idle_injection_device, cpu);
+
+       iit = per_cpu_ptr(&idle_injection_thread, cpu);
+
+       /*
+	* Boolean used by the smpboot mainloop and used as a flip-flop
+	* in this function
+	*/
+       iit->should_run = 0;
+
+       atomic_inc(&ii_dev->count);
+
+       idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
+
+       play_idle(idle_duration_ms);
+
+       /*
+        * The last CPU waking up is in charge of setting the timer. If
+        * the CPU is hotplugged, the timer will move to another CPU
+        * (which may not belong to the same cluster) but that is not a
+        * problem as the timer will be set again by another CPU
+        * belonging to the cluster. This mechanism is self adaptive.
+        */
+       if (!atomic_dec_and_test(&ii_dev->count))
+               return;
+
+       run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
+       if (!run_duration_ms)
+	       return;
+
+       hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
+		     HRTIMER_MODE_REL_PINNED);
+}
+
+/**
+ * idle_injection_set_duration - idle and run duration helper
+ * @run_duration_ms: an unsigned int giving the running time in milliseconds
+ * @idle_duration_ms: an unsigned int giving the idle time in milliseconds
+ */
+void idle_injection_set_duration(struct idle_injection_device *ii_dev,
+				 unsigned int run_duration_ms,
+				 unsigned int idle_duration_ms)
+{
+	atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
+	atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
+}
+
+
+/**
+ * idle_injection_get_duration - idle and run duration helper
+ * @run_duration_ms: a pointer to an unsigned int to store the running time
+ * @idle_duration_ms: a pointer to an unsigned int to store the idle time
+ */
+void idle_injection_get_duration(struct idle_injection_device *ii_dev,
+				 unsigned int *run_duration_ms,
+				 unsigned int *idle_duration_ms)
+{
+	*run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
+	*idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
+}
+
+/**
+ * idle_injection_start - starts the idle injections
+ * @ii_dev: a pointer to an idle_injection_device structure
+ *
+ * The function starts the idle injection cycles by first waking up
+ * all the tasks the ii_dev is attached to and let them handle the
+ * idle-run periods
+ *
+ * Returns -EINVAL if the idle or the running duration are not set
+ */
+int idle_injection_start(struct idle_injection_device *ii_dev)
+{
+	if (!atomic_read(&ii_dev->idle_duration_ms))
+		return -EINVAL;
+
+	if (!atomic_read(&ii_dev->run_duration_ms))
+		return -EINVAL;
+
+	pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n",
+		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
+
+	idle_injection_wakeup(ii_dev);
+
+	return 0;
+}
+
+/**
+ * idle_injection_stop - stops the idle injections
+ * @ii_dev: a pointer to an idle injection_device structure
+ *
+ * The function stops the idle injection by canceling the timer in
+ * charge of waking up the tasks to inject idle and unset the idle and
+ * running durations.
+ */
+void idle_injection_stop(struct idle_injection_device *ii_dev)
+{
+	pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
+		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
+
+	hrtimer_cancel(&ii_dev->timer);
+
+	idle_injection_set_duration(ii_dev, 0, 0);
+}
+
+/**
+ * idle_injection_setup - initialize the current task as a RT task
+ * @cpu: the CPU number where the kthread is running on (not used)
+ *
+ */
+static void idle_injection_setup(unsigned int cpu)
+{
+	struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 };
+
+	set_freezable();
+
+	sched_setscheduler(current, SCHED_FIFO, &param);
+}
+
+/**
+ * idle_injection_should_run - function helper for the smpboot API
+ * @cpu: the CPU number where the kthread is running on
+ *
+ * Returns the boolean telling if the thread can run
+ */
+static int idle_injection_should_run(unsigned int cpu)
+{
+	struct idle_injection_thread *iit =
+		per_cpu_ptr(&idle_injection_thread, cpu);
+
+	return iit->should_run;
+}
+
+/*
+ * idle_injection_threads - smp hotplug threads ops
+ */
+static struct smp_hotplug_thread idle_injection_threads = {
+	.store                  = &idle_injection_thread.tsk,
+	.thread_fn              = idle_injection_fn,
+	.thread_should_run	= idle_injection_should_run,
+	.setup                  = idle_injection_setup,
+};
+
+/**
+ * idle_injection_register - idle injection init routine
+ * @cpumask: the list of CPUs to run the kthreads
+ * @name: the threads command name
+ *
+ * This is the initialization function in charge of creating the
+ * kthreads, initializing the timer and allocate the structures.  It
+ * does not starts the idle injection cycles
+ *
+ * Returns -ENOMEM if an allocation fails, or < 0 if the smpboot
+ * kthread registering fails.
+ */
+struct idle_injection_device *
+idle_injection_register(struct cpumask *cpumask, const char *name)
+{
+	struct idle_injection_device *ii_dev;
+	struct smp_hotplug_thread *smp_hotplug_thread;
+	char *idle_injection_comm;
+	int cpu, ret;
+
+	ret = -ENOMEM;
+
+	idle_injection_comm = kasprintf(GFP_KERNEL, "%s/%%u", name);
+	if (!idle_injection_comm)
+		goto out;
+
+	smp_hotplug_thread = kmemdup(&idle_injection_threads,
+				     sizeof(*smp_hotplug_thread),
+				     GFP_KERNEL);
+	if (!smp_hotplug_thread)
+		goto out_free_thread_comm;
+
+	smp_hotplug_thread->thread_comm	= idle_injection_comm;
+
+	ii_dev = kzalloc(sizeof(*ii_dev),
+					GFP_KERNEL);
+	if (!ii_dev)
+		goto out_free_smp_hotplug;
+
+	ii_dev->smp_hotplug_thread = smp_hotplug_thread;
+
+	hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+
+	ii_dev->timer.function = idle_injection_wakeup_fn;
+
+	for_each_cpu(cpu, cpumask)
+		per_cpu(idle_injection_device, cpu) = ii_dev;
+
+	ret = smpboot_register_percpu_thread_cpumask(smp_hotplug_thread,
+						     cpumask);
+	if (ret)
+		goto out_free_idle_inject;
+
+	return ii_dev;
+
+out_free_idle_inject:
+	kfree(ii_dev);
+out_free_smp_hotplug:
+	kfree(smp_hotplug_thread);
+out_free_thread_comm:
+	kfree(idle_injection_comm);
+out:
+	return ERR_PTR(ret);
+}
+
+/**
+ * idle_injection_unregister - Unregister the idle injection device
+ * @ii_dev: a pointer to an idle injection device
+ *
+ * The function is in charge of stopping the idle injections,
+ * unregister the kthreads and free the allocated memory in the
+ * register function.
+ */
+void idle_injection_unregister(struct idle_injection_device *ii_dev)
+{
+	struct smp_hotplug_thread *smp_hotplug_thread = ii_dev->smp_hotplug_thread;
+
+	idle_injection_stop(ii_dev);
+	smpboot_unregister_percpu_thread(smp_hotplug_thread);
+	kfree(smp_hotplug_thread->thread_comm);
+	kfree(smp_hotplug_thread);
+	kfree(ii_dev);
+}
diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
new file mode 100644
index 0000000..084b999
--- /dev/null
+++ b/include/linux/idle_injection.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ */
+#ifndef __IDLE_INJECTION_H__
+#define __IDLE_INJECTION_H__
+
+/* private idle injection device structure */
+struct idle_injection_device;
+
+/**
+ * idle_injection_register - allocates and initializes an idle_injection_device
+ * @cpumask: all CPUs with a idle injection kthreads
+ * @name: a const string giving the kthread name
+ *
+ * Returns a pointer to a idle_injection_device, ERR_PTR otherwise.
+ */
+struct idle_injection_device *idle_injection_register(struct cpumask *cpumask,
+						      const char *name);
+
+/**
+ * idle_injection_unregister - stop and frees the resources
+ * @ii_dev: a pointer to an idle_injection_device structure
+ */
+void idle_injection_unregister(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_start - start injecting idle cycles
+ * @ii_dev: a pointer to an idle_injection_device structure
+ *
+ * Returns 0 on success, -EINVAL if the idle or the run durations are
+ * not set
+ */
+int idle_injection_start(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_stop - stop injecting idle cycles
+ * @ii_dev: a pointer to an idle_injection_device structure
+ */
+void idle_injection_stop(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_set_duration - set the idle/run durations
+ * @run_duration_ms: the running duration
+ * @idle_duration_ms : the idle duration
+ */
+void idle_injection_set_duration(struct idle_injection_device *ii_dev,
+				 unsigned int run_duration_ms,
+				 unsigned int idle_duration_ms);
+
+/**
+ * idle_injection_get_duration - get the idle/run durations
+ * @run_duration_ms: the running duration
+ * @idle_duration_ms : the idle duration
+ */
+void idle_injection_get_duration(struct idle_injection_device *ii_dev,
+				 unsigned int *run_duration_ms,
+				 unsigned int *idle_duration_ms);
+#endif /* __IDLE_INJECTION_H__ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-10 10:34 [PATCH] powercap/drivers/idle_injection: Add an idle injection framework Daniel Lezcano
@ 2018-05-10 12:26 ` Daniel Lezcano
  2018-05-11  9:32   ` Viresh Kumar
  2018-05-11  6:11 ` [PATCH] " kbuild test robot
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2018-05-10 12:26 UTC (permalink / raw)
  To: rjw
  Cc: viresh.kumar, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

Initially, the cpu_cooling device for ARM was changed by adding a new
policy inserting idle cycles. The intel_powerclamp driver does a
similar action.

Instead of implementing idle injections privately in the cpu_cooling
device, move the idle injection code in a dedicated framework and give
the opportunity to other frameworks to make use of it.

The framework relies on the smpboot kthreads which handles via its
mainloop the common code for hotplugging and [un]parking.

The idle injection is registered with a name and a cpumask. It will
result in the creation of the kthreads on all the cpus specified in
the cpumask with a name followed by the cpu number. No idle injection
is done at this time.

The idle + run duration must be specified via the helpers and then the
idle injection can be started at this point.

The kthread will call play_idle() with the specified idle duration
from above and then will schedule itself. The latest CPU is in charge
of setting the timer for the next idle injection deadline.

The task handling the timer interrupt will wakeup all the kthreads
belonging to the cpumask.

This code was previously tested with the cpu cooling device and went
through several iterations. It results now in split code and API
exported in the header file. It was tested with the cpu cooling device
with success.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 V2: Fixed checkpatch warnings

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/powercap/Kconfig          |  10 ++
 drivers/powercap/Makefile         |   1 +
 drivers/powercap/idle_injection.c | 331 ++++++++++++++++++++++++++++++++++++++
 include/linux/idle_injection.h    |  62 +++++++
 4 files changed, 404 insertions(+)
 create mode 100644 drivers/powercap/idle_injection.c
 create mode 100644 include/linux/idle_injection.h

diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
index 85727ef..a767ef2 100644
--- a/drivers/powercap/Kconfig
+++ b/drivers/powercap/Kconfig
@@ -29,4 +29,14 @@ config INTEL_RAPL
 	  controller, CPU core (Power Plance 0), graphics uncore (Power Plane
 	  1), etc.
 
+config IDLE_INJECTION
+	bool "Idle injection framework"
+	depends on CPU_IDLE
+	default n
+	help
+	  This enables support for the idle injection framework. It
+	  provides a way to force idle periods on a set of specified
+	  CPUs for power capping. Idle period can be injected
+	  synchronously on a set of specified CPUs or alternatively
+	  on a per CPU basis.
 endif
diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
index 0a21ef3..c3bbfee 100644
--- a/drivers/powercap/Makefile
+++ b/drivers/powercap/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
 obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o
+obj-$(CONFIG_IDLE_INJECTION) += idle_injection.o
diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
new file mode 100644
index 0000000..825ffac
--- /dev/null
+++ b/drivers/powercap/idle_injection.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drivers/powercap/idle_injection.c
+ *
+ * Copyright 2018 Linaro Limited
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ * The idle injection framework propose a way to force a cpu to enter
+ * an idle state during a specified amount of time for a specified
+ * period.
+ *
+ */
+#include <linux/cpu.h>
+#include <linux/freezer.h>
+#include <linux/hrtimer.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/smpboot.h>
+
+#include <uapi/linux/sched/types.h>
+
+/**
+ * struct idle_injection_thread - task on/off switch structure
+ * @tsk: a pointer to a task_struct injecting the idle cycles
+ * @should_run: a integer used as a boolean by the smpboot kthread API
+ */
+struct idle_injection_thread {
+	struct task_struct *tsk;
+	int should_run;
+};
+
+/**
+ * struct idle_injection_device - data for the idle injection
+ * @smp_hotplug_thread: a pointer to a struct smp_hotplug_thread
+ * @timer: a hrtimer giving the tempo for the idle injection
+ * @count: an atomic to keep track of the last task exiting the idle cycle
+ * @idle_duration_ms: an atomic specifying the idle duration
+ * @run_duration_ms: an atomic specifying the running duration
+ */
+struct idle_injection_device {
+	struct smp_hotplug_thread *smp_hotplug_thread;
+	struct hrtimer timer;
+	atomic_t idle_duration_ms;
+	atomic_t run_duration_ms;
+	atomic_t count;
+};
+
+static DEFINE_PER_CPU(struct idle_injection_thread, idle_injection_thread);
+static DEFINE_PER_CPU(struct idle_injection_device *, idle_injection_device);
+
+/**
+ * idle_injection_wakeup - Wake up all idle injection threads
+ * @ii_dev: the idle injection device
+ *
+ * Every idle injection task belonging to the idle injection device
+ * and running on an online CPU will be wake up by this call.
+ */
+static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
+{
+	struct idle_injection_thread *iit;
+	struct cpumask *cpumask = ii_dev->smp_hotplug_thread->cpumask;
+	int cpu;
+
+	for_each_cpu_and(cpu, cpumask, cpu_online_mask) {
+		iit = per_cpu_ptr(&idle_injection_thread, cpu);
+		iit->should_run = 1;
+		wake_up_process(iit->tsk);
+	}
+}
+
+/**
+ * idle_injection_wakeup_fn - idle injection timer callback
+ * @timer: a hrtimer structure
+ *
+ * This function is called when the idle injection timer expires which
+ * will wake up the idle injection tasks and these ones, in turn, play
+ * idle a specified amount of time.
+ *
+ * Always returns HRTIMER_NORESTART
+ */
+static enum hrtimer_restart idle_injection_wakeup_fn(struct hrtimer *timer)
+{
+	struct idle_injection_device *ii_dev =
+		container_of(timer, struct idle_injection_device, timer);
+
+	idle_injection_wakeup(ii_dev);
+
+	return HRTIMER_NORESTART;
+}
+
+/**
+ * idle_injection_fn - idle injection routine
+ * @cpu: the CPU number the tasks belongs to
+ *
+ * The idle injection routine will stay idle the specified amount of
+ * time
+ */
+static void idle_injection_fn(unsigned int cpu)
+{
+	struct idle_injection_device *ii_dev;
+	struct idle_injection_thread *iit;
+	int run_duration_ms, idle_duration_ms;
+
+	ii_dev = per_cpu(idle_injection_device, cpu);
+
+	iit = per_cpu_ptr(&idle_injection_thread, cpu);
+
+	/*
+	 * Boolean used by the smpboot mainloop and used as a flip-flop
+	 * in this function
+	 */
+	iit->should_run = 0;
+
+	atomic_inc(&ii_dev->count);
+
+	idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
+
+	play_idle(idle_duration_ms);
+
+	/*
+	 * The last CPU waking up is in charge of setting the timer. If
+	 * the CPU is hotplugged, the timer will move to another CPU
+	 * (which may not belong to the same cluster) but that is not a
+	 * problem as the timer will be set again by another CPU
+	 * belonging to the cluster. This mechanism is self adaptive.
+	 */
+	if (!atomic_dec_and_test(&ii_dev->count))
+		return;
+
+	run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
+	if (!run_duration_ms)
+		return;
+
+	hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
+		      HRTIMER_MODE_REL_PINNED);
+}
+
+/**
+ * idle_injection_set_duration - idle and run duration helper
+ * @run_duration_ms: an unsigned int giving the running time in milliseconds
+ * @idle_duration_ms: an unsigned int giving the idle time in milliseconds
+ */
+void idle_injection_set_duration(struct idle_injection_device *ii_dev,
+				 unsigned int run_duration_ms,
+				 unsigned int idle_duration_ms)
+{
+	atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
+	atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
+}
+
+
+/**
+ * idle_injection_get_duration - idle and run duration helper
+ * @run_duration_ms: a pointer to an unsigned int to store the running time
+ * @idle_duration_ms: a pointer to an unsigned int to store the idle time
+ */
+void idle_injection_get_duration(struct idle_injection_device *ii_dev,
+				 unsigned int *run_duration_ms,
+				 unsigned int *idle_duration_ms)
+{
+	*run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
+	*idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
+}
+
+/**
+ * idle_injection_start - starts the idle injections
+ * @ii_dev: a pointer to an idle_injection_device structure
+ *
+ * The function starts the idle injection cycles by first waking up
+ * all the tasks the ii_dev is attached to and let them handle the
+ * idle-run periods
+ *
+ * Returns -EINVAL if the idle or the running duration are not set
+ */
+int idle_injection_start(struct idle_injection_device *ii_dev)
+{
+	if (!atomic_read(&ii_dev->idle_duration_ms))
+		return -EINVAL;
+
+	if (!atomic_read(&ii_dev->run_duration_ms))
+		return -EINVAL;
+
+	pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n",
+		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
+
+	idle_injection_wakeup(ii_dev);
+
+	return 0;
+}
+
+/**
+ * idle_injection_stop - stops the idle injections
+ * @ii_dev: a pointer to an idle injection_device structure
+ *
+ * The function stops the idle injection by canceling the timer in
+ * charge of waking up the tasks to inject idle and unset the idle and
+ * running durations.
+ */
+void idle_injection_stop(struct idle_injection_device *ii_dev)
+{
+	pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
+		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
+
+	hrtimer_cancel(&ii_dev->timer);
+
+	idle_injection_set_duration(ii_dev, 0, 0);
+}
+
+/**
+ * idle_injection_setup - initialize the current task as a RT task
+ * @cpu: the CPU number where the kthread is running on (not used)
+ *
+ */
+static void idle_injection_setup(unsigned int cpu)
+{
+	struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
+
+	set_freezable();
+
+	sched_setscheduler(current, SCHED_FIFO, &param);
+}
+
+/**
+ * idle_injection_should_run - function helper for the smpboot API
+ * @cpu: the CPU number where the kthread is running on
+ *
+ * Returns the boolean telling if the thread can run
+ */
+static int idle_injection_should_run(unsigned int cpu)
+{
+	struct idle_injection_thread *iit =
+		per_cpu_ptr(&idle_injection_thread, cpu);
+
+	return iit->should_run;
+}
+
+/*
+ * idle_injection_threads - smp hotplug threads ops
+ */
+static struct smp_hotplug_thread idle_injection_threads = {
+	.store                  = &idle_injection_thread.tsk,
+	.thread_fn              = idle_injection_fn,
+	.thread_should_run	= idle_injection_should_run,
+	.setup                  = idle_injection_setup,
+};
+
+/**
+ * idle_injection_register - idle injection init routine
+ * @cpumask: the list of CPUs to run the kthreads
+ * @name: the threads command name
+ *
+ * This is the initialization function in charge of creating the
+ * kthreads, initializing the timer and allocate the structures.  It
+ * does not starts the idle injection cycles
+ *
+ * Returns -ENOMEM if an allocation fails, or < 0 if the smpboot
+ * kthread registering fails.
+ */
+struct idle_injection_device *
+idle_injection_register(struct cpumask *cpumask, const char *name)
+{
+	struct idle_injection_device *ii_dev;
+	struct smp_hotplug_thread *smp_hotplug_thread;
+	char *idle_injection_comm;
+	int cpu, ret;
+
+	ret = -ENOMEM;
+
+	idle_injection_comm = kasprintf(GFP_KERNEL, "%s/%%u", name);
+	if (!idle_injection_comm)
+		goto out;
+
+	smp_hotplug_thread = kmemdup(&idle_injection_threads,
+				     sizeof(*smp_hotplug_thread),
+				     GFP_KERNEL);
+	if (!smp_hotplug_thread)
+		goto out_free_thread_comm;
+
+	smp_hotplug_thread->thread_comm	= idle_injection_comm;
+
+	ii_dev = kzalloc(sizeof(*ii_dev),
+					GFP_KERNEL);
+	if (!ii_dev)
+		goto out_free_smp_hotplug;
+
+	ii_dev->smp_hotplug_thread = smp_hotplug_thread;
+
+	hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+
+	ii_dev->timer.function = idle_injection_wakeup_fn;
+
+	for_each_cpu(cpu, cpumask)
+		per_cpu(idle_injection_device, cpu) = ii_dev;
+
+	ret = smpboot_register_percpu_thread_cpumask(smp_hotplug_thread,
+						     cpumask);
+	if (ret)
+		goto out_free_idle_inject;
+
+	return ii_dev;
+
+out_free_idle_inject:
+	kfree(ii_dev);
+out_free_smp_hotplug:
+	kfree(smp_hotplug_thread);
+out_free_thread_comm:
+	kfree(idle_injection_comm);
+out:
+	return ERR_PTR(ret);
+}
+
+/**
+ * idle_injection_unregister - Unregister the idle injection device
+ * @ii_dev: a pointer to an idle injection device
+ *
+ * The function is in charge of stopping the idle injections,
+ * unregister the kthreads and free the allocated memory in the
+ * register function.
+ */
+void idle_injection_unregister(struct idle_injection_device *ii_dev)
+{
+	struct smp_hotplug_thread *smp_hotplug_thread;
+
+	idle_injection_stop(ii_dev);
+	smp_hotplug_thread = ii_dev->smp_hotplug_thread;
+	smpboot_unregister_percpu_thread(smp_hotplug_thread);
+	kfree(smp_hotplug_thread->thread_comm);
+	kfree(smp_hotplug_thread);
+	kfree(ii_dev);
+}
diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
new file mode 100644
index 0000000..084b999
--- /dev/null
+++ b/include/linux/idle_injection.h
@@ -0,0 +1,62 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Linaro Ltd
+ *
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *
+ */
+#ifndef __IDLE_INJECTION_H__
+#define __IDLE_INJECTION_H__
+
+/* private idle injection device structure */
+struct idle_injection_device;
+
+/**
+ * idle_injection_register - allocates and initializes an idle_injection_device
+ * @cpumask: all CPUs with a idle injection kthreads
+ * @name: a const string giving the kthread name
+ *
+ * Returns a pointer to a idle_injection_device, ERR_PTR otherwise.
+ */
+struct idle_injection_device *idle_injection_register(struct cpumask *cpumask,
+						      const char *name);
+
+/**
+ * idle_injection_unregister - stop and frees the resources
+ * @ii_dev: a pointer to an idle_injection_device structure
+ */
+void idle_injection_unregister(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_start - start injecting idle cycles
+ * @ii_dev: a pointer to an idle_injection_device structure
+ *
+ * Returns 0 on success, -EINVAL if the idle or the run durations are
+ * not set
+ */
+int idle_injection_start(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_stop - stop injecting idle cycles
+ * @ii_dev: a pointer to an idle_injection_device structure
+ */
+void idle_injection_stop(struct idle_injection_device *ii_dev);
+
+/**
+ * idle_injection_set_duration - set the idle/run durations
+ * @run_duration_ms: the running duration
+ * @idle_duration_ms : the idle duration
+ */
+void idle_injection_set_duration(struct idle_injection_device *ii_dev,
+				 unsigned int run_duration_ms,
+				 unsigned int idle_duration_ms);
+
+/**
+ * idle_injection_get_duration - get the idle/run durations
+ * @run_duration_ms: the running duration
+ * @idle_duration_ms : the idle duration
+ */
+void idle_injection_get_duration(struct idle_injection_device *ii_dev,
+				 unsigned int *run_duration_ms,
+				 unsigned int *idle_duration_ms);
+#endif /* __IDLE_INJECTION_H__ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-10 10:34 [PATCH] powercap/drivers/idle_injection: Add an idle injection framework Daniel Lezcano
  2018-05-10 12:26 ` [PATCH V2] " Daniel Lezcano
@ 2018-05-11  6:11 ` kbuild test robot
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2018-05-11  6:11 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: kbuild-all, rjw, viresh.kumar, edubezval, kevin.wangtao, leo.yan,
	vincent.guittot, linux-kernel, javi.merino, rui.zhang, linux-pm,
	daniel.thompson

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.17-rc4 next-20180510]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Lezcano/powercap-drivers-idle_injection-Add-an-idle-injection-framework/20180511-074751
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/powercap/idle_injection.c:242:36: sparse: incorrect type in initializer (different address spaces) @@    expected struct task_struct [noderef] <asn:3>**store @@    got struct task_struct [noderef] <asn:3>**store @@
   drivers/powercap/idle_injection.c:242:36:    expected struct task_struct [noderef] <asn:3>**store
   drivers/powercap/idle_injection.c:242:36:    got struct task_struct *[noderef] <asn:3>*<noident>

vim +242 drivers/powercap/idle_injection.c

   138	
   139	/**
   140	 * idle_injection_set_duration - idle and run duration helper
   141	 * @run_duration_ms: an unsigned int giving the running time in milliseconds
   142	 * @idle_duration_ms: an unsigned int giving the idle time in milliseconds
   143	 */
 > 144	void idle_injection_set_duration(struct idle_injection_device *ii_dev,
   145					 unsigned int run_duration_ms,
   146					 unsigned int idle_duration_ms)
   147	{
   148		atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
   149		atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
   150	}
   151	
   152	
   153	/**
   154	 * idle_injection_get_duration - idle and run duration helper
   155	 * @run_duration_ms: a pointer to an unsigned int to store the running time
   156	 * @idle_duration_ms: a pointer to an unsigned int to store the idle time
   157	 */
   158	void idle_injection_get_duration(struct idle_injection_device *ii_dev,
   159					 unsigned int *run_duration_ms,
   160					 unsigned int *idle_duration_ms)
   161	{
   162		*run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
   163		*idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
   164	}
   165	
   166	/**
   167	 * idle_injection_start - starts the idle injections
   168	 * @ii_dev: a pointer to an idle_injection_device structure
   169	 *
   170	 * The function starts the idle injection cycles by first waking up
   171	 * all the tasks the ii_dev is attached to and let them handle the
   172	 * idle-run periods
   173	 *
   174	 * Returns -EINVAL if the idle or the running duration are not set
   175	 */
   176	int idle_injection_start(struct idle_injection_device *ii_dev)
   177	{
   178		if (!atomic_read(&ii_dev->idle_duration_ms))
   179			return -EINVAL;
   180	
   181		if (!atomic_read(&ii_dev->run_duration_ms))
   182			return -EINVAL;
   183	
   184		pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n",
   185			 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
   186	
   187		idle_injection_wakeup(ii_dev);
   188	
   189		return 0;
   190	}
   191	
   192	/**
   193	 * idle_injection_stop - stops the idle injections
   194	 * @ii_dev: a pointer to an idle injection_device structure
   195	 *
   196	 * The function stops the idle injection by canceling the timer in
   197	 * charge of waking up the tasks to inject idle and unset the idle and
   198	 * running durations.
   199	 */
   200	void idle_injection_stop(struct idle_injection_device *ii_dev)
   201	{
   202		pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
   203			 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
   204	
   205		hrtimer_cancel(&ii_dev->timer);
   206	
   207		idle_injection_set_duration(ii_dev, 0, 0);
   208	}
   209	
   210	/**
   211	 * idle_injection_setup - initialize the current task as a RT task
   212	 * @cpu: the CPU number where the kthread is running on (not used)
   213	 *
   214	 */
   215	static void idle_injection_setup(unsigned int cpu)
   216	{
   217		struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO/2 };
   218	
   219		set_freezable();
   220	
   221		sched_setscheduler(current, SCHED_FIFO, &param);
   222	}
   223	
   224	/**
   225	 * idle_injection_should_run - function helper for the smpboot API
   226	 * @cpu: the CPU number where the kthread is running on
   227	 *
   228	 * Returns the boolean telling if the thread can run
   229	 */
   230	static int idle_injection_should_run(unsigned int cpu)
   231	{
   232		struct idle_injection_thread *iit =
   233			per_cpu_ptr(&idle_injection_thread, cpu);
   234	
   235		return iit->should_run;
   236	}
   237	
   238	/*
   239	 * idle_injection_threads - smp hotplug threads ops
   240	 */
   241	static struct smp_hotplug_thread idle_injection_threads = {
 > 242		.store                  = &idle_injection_thread.tsk,
   243		.thread_fn              = idle_injection_fn,
   244		.thread_should_run	= idle_injection_should_run,
   245		.setup                  = idle_injection_setup,
   246	};
   247	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-10 12:26 ` [PATCH V2] " Daniel Lezcano
@ 2018-05-11  9:32   ` Viresh Kumar
  2018-05-11 11:55     ` Daniel Lezcano
  0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2018-05-11  9:32 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

On 10-05-18, 14:26, Daniel Lezcano wrote:
> Initially, the cpu_cooling device for ARM was changed by adding a new
> policy inserting idle cycles. The intel_powerclamp driver does a
> similar action.
> 
> Instead of implementing idle injections privately in the cpu_cooling
> device, move the idle injection code in a dedicated framework and give
> the opportunity to other frameworks to make use of it.

Maybe move the above explanation in the comments section below, as it
doesn't belong to the commit log really.

> The framework relies on the smpboot kthreads which handles via its
> mainloop the common code for hotplugging and [un]parking.
> 
> The idle injection is registered with a name and a cpumask. It will
> result in the creation of the kthreads on all the cpus specified in
> the cpumask with a name followed by the cpu number. No idle injection
> is done at this time.
> 
> The idle + run duration must be specified via the helpers and then the
> idle injection can be started at this point.
> 
> The kthread will call play_idle() with the specified idle duration
> from above and then will schedule itself. The latest CPU is in charge
> of setting the timer for the next idle injection deadline.
> 
> The task handling the timer interrupt will wakeup all the kthreads
> belonging to the cpumask.
> 
> This code was previously tested with the cpu cooling device and went
> through several iterations. It results now in split code and API
> exported in the header file. It was tested with the cpu cooling device
> with success.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  V2: Fixed checkpatch warnings
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/powercap/Kconfig          |  10 ++
>  drivers/powercap/Makefile         |   1 +
>  drivers/powercap/idle_injection.c | 331 ++++++++++++++++++++++++++++++++++++++
>  include/linux/idle_injection.h    |  62 +++++++
>  4 files changed, 404 insertions(+)
>  create mode 100644 drivers/powercap/idle_injection.c
>  create mode 100644 include/linux/idle_injection.h
> 
> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
> index 85727ef..a767ef2 100644
> --- a/drivers/powercap/Kconfig
> +++ b/drivers/powercap/Kconfig
> @@ -29,4 +29,14 @@ config INTEL_RAPL
>  	  controller, CPU core (Power Plance 0), graphics uncore (Power Plane
>  	  1), etc.
>  
> +config IDLE_INJECTION
> +	bool "Idle injection framework"
> +	depends on CPU_IDLE
> +	default n
> +	help
> +	  This enables support for the idle injection framework. It
> +	  provides a way to force idle periods on a set of specified
> +	  CPUs for power capping. Idle period can be injected
> +	  synchronously on a set of specified CPUs or alternatively
> +	  on a per CPU basis.
>  endif
> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
> index 0a21ef3..c3bbfee 100644
> --- a/drivers/powercap/Makefile
> +++ b/drivers/powercap/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_POWERCAP)	+= powercap_sys.o
>  obj-$(CONFIG_INTEL_RAPL) += intel_rapl.o
> +obj-$(CONFIG_IDLE_INJECTION) += idle_injection.o
> diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
> new file mode 100644
> index 0000000..825ffac
> --- /dev/null
> +++ b/drivers/powercap/idle_injection.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * drivers/powercap/idle_injection.c
> + *

What's the purpose of this particular line ? Yeah, I have seen it
quite a number of times and might have added that myself as well
somewhere :)

> + * Copyright 2018 Linaro Limited
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + * The idle injection framework propose a way to force a cpu to enter
> + * an idle state during a specified amount of time for a specified
> + * period.
> + *
> + */
> +#include <linux/cpu.h>
> +#include <linux/freezer.h>
> +#include <linux/hrtimer.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/smpboot.h>
> +
> +#include <uapi/linux/sched/types.h>
> +
> +/**
> + * struct idle_injection_thread - task on/off switch structure
> + * @tsk: a pointer to a task_struct injecting the idle cycles
> + * @should_run: a integer used as a boolean by the smpboot kthread API
> + */
> +struct idle_injection_thread {
> +	struct task_struct *tsk;
> +	int should_run;
> +};
> +
> +/**
> + * struct idle_injection_device - data for the idle injection
> + * @smp_hotplug_thread: a pointer to a struct smp_hotplug_thread
> + * @timer: a hrtimer giving the tempo for the idle injection
> + * @count: an atomic to keep track of the last task exiting the idle cycle
> + * @idle_duration_ms: an atomic specifying the idle duration
> + * @run_duration_ms: an atomic specifying the running duration
> + */
> +struct idle_injection_device {
> +	struct smp_hotplug_thread *smp_hotplug_thread;
> +	struct hrtimer timer;
> +	atomic_t idle_duration_ms;
> +	atomic_t run_duration_ms;
> +	atomic_t count;
> +};
> +
> +static DEFINE_PER_CPU(struct idle_injection_thread, idle_injection_thread);
> +static DEFINE_PER_CPU(struct idle_injection_device *, idle_injection_device);
> +
> +/**
> + * idle_injection_wakeup - Wake up all idle injection threads
> + * @ii_dev: the idle injection device
> + *
> + * Every idle injection task belonging to the idle injection device
> + * and running on an online CPU will be wake up by this call.
> + */
> +static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
> +{
> +	struct idle_injection_thread *iit;
> +	struct cpumask *cpumask = ii_dev->smp_hotplug_thread->cpumask;
> +	int cpu;
> +
> +	for_each_cpu_and(cpu, cpumask, cpu_online_mask) {
> +		iit = per_cpu_ptr(&idle_injection_thread, cpu);
> +		iit->should_run = 1;
> +		wake_up_process(iit->tsk);
> +	}
> +}
> +
> +/**
> + * idle_injection_wakeup_fn - idle injection timer callback
> + * @timer: a hrtimer structure
> + *
> + * This function is called when the idle injection timer expires which
> + * will wake up the idle injection tasks and these ones, in turn, play
> + * idle a specified amount of time.
> + *
> + * Always returns HRTIMER_NORESTART
> + */
> +static enum hrtimer_restart idle_injection_wakeup_fn(struct hrtimer *timer)
> +{
> +	struct idle_injection_device *ii_dev =
> +		container_of(timer, struct idle_injection_device, timer);
> +
> +	idle_injection_wakeup(ii_dev);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * idle_injection_fn - idle injection routine
> + * @cpu: the CPU number the tasks belongs to
> + *
> + * The idle injection routine will stay idle the specified amount of
> + * time
> + */
> +static void idle_injection_fn(unsigned int cpu)
> +{
> +	struct idle_injection_device *ii_dev;
> +	struct idle_injection_thread *iit;
> +	int run_duration_ms, idle_duration_ms;
> +
> +	ii_dev = per_cpu(idle_injection_device, cpu);
> +
> +	iit = per_cpu_ptr(&idle_injection_thread, cpu);
> +
> +	/*
> +	 * Boolean used by the smpboot mainloop and used as a flip-flop

s/mainloop/main loop/ ?

> +	 * in this function
> +	 */
> +	iit->should_run = 0;

What is the purpose of this field ? Just wanted to check on how
smpboot stuff uses it.

> +
> +	atomic_inc(&ii_dev->count);
> +
> +	idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
> +
> +	play_idle(idle_duration_ms);
> +
> +	/*
> +	 * The last CPU waking up is in charge of setting the timer. If
> +	 * the CPU is hotplugged, the timer will move to another CPU
> +	 * (which may not belong to the same cluster) but that is not a
> +	 * problem as the timer will be set again by another CPU
> +	 * belonging to the cluster. This mechanism is self adaptive.
> +	 */
> +	if (!atomic_dec_and_test(&ii_dev->count))
> +		return;
> +
> +	run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> +	if (!run_duration_ms)
> +		return;
> +
> +	hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
> +		      HRTIMER_MODE_REL_PINNED);
> +}
> +
> +/**
> + * idle_injection_set_duration - idle and run duration helper
> + * @run_duration_ms: an unsigned int giving the running time in milliseconds
> + * @idle_duration_ms: an unsigned int giving the idle time in milliseconds
> + */
> +void idle_injection_set_duration(struct idle_injection_device *ii_dev,
> +				 unsigned int run_duration_ms,
> +				 unsigned int idle_duration_ms)
> +{
> +	atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
> +	atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
> +}
> +
> +

Two blank lines here ?

> +/**
> + * idle_injection_get_duration - idle and run duration helper
> + * @run_duration_ms: a pointer to an unsigned int to store the running time
> + * @idle_duration_ms: a pointer to an unsigned int to store the idle time
> + */
> +void idle_injection_get_duration(struct idle_injection_device *ii_dev,
> +				 unsigned int *run_duration_ms,
> +				 unsigned int *idle_duration_ms)
> +{
> +	*run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> +	*idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
> +}
> +
> +/**
> + * idle_injection_start - starts the idle injections
> + * @ii_dev: a pointer to an idle_injection_device structure
> + *
> + * The function starts the idle injection cycles by first waking up
> + * all the tasks the ii_dev is attached to and let them handle the
> + * idle-run periods
> + *
> + * Returns -EINVAL if the idle or the running duration are not set
> + */
> +int idle_injection_start(struct idle_injection_device *ii_dev)
> +{
> +	if (!atomic_read(&ii_dev->idle_duration_ms))
> +		return -EINVAL;
> +
> +	if (!atomic_read(&ii_dev->run_duration_ms))
> +		return -EINVAL;

You are required to have them as atomic variables to take care of the
races while they are set ? What about getting the durations as
arguments to this routine then ?

> +
> +	pr_debug("Starting injecting idle cycles on CPUs '%*pbl'\n",
> +		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
> +
> +	idle_injection_wakeup(ii_dev);
> +
> +	return 0;
> +}
> +
> +/**
> + * idle_injection_stop - stops the idle injections
> + * @ii_dev: a pointer to an idle injection_device structure
> + *
> + * The function stops the idle injection by canceling the timer in
> + * charge of waking up the tasks to inject idle and unset the idle and
> + * running durations.
> + */
> +void idle_injection_stop(struct idle_injection_device *ii_dev)
> +{
> +	pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
> +		 cpumask_pr_args(ii_dev->smp_hotplug_thread->cpumask));
> +
> +	hrtimer_cancel(&ii_dev->timer);
> +
> +	idle_injection_set_duration(ii_dev, 0, 0);
> +}
> +
> +/**
> + * idle_injection_setup - initialize the current task as a RT task
> + * @cpu: the CPU number where the kthread is running on (not used)
> + *
> + */
> +static void idle_injection_setup(unsigned int cpu)
> +{
> +	struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
> +
> +	set_freezable();
> +
> +	sched_setscheduler(current, SCHED_FIFO, &param);
> +}
> +
> +/**
> + * idle_injection_should_run - function helper for the smpboot API
> + * @cpu: the CPU number where the kthread is running on
> + *
> + * Returns the boolean telling if the thread can run
> + */
> +static int idle_injection_should_run(unsigned int cpu)
> +{
> +	struct idle_injection_thread *iit =
> +		per_cpu_ptr(&idle_injection_thread, cpu);
> +
> +	return iit->should_run;
> +}
> +
> +/*
> + * idle_injection_threads - smp hotplug threads ops
> + */
> +static struct smp_hotplug_thread idle_injection_threads = {
> +	.store                  = &idle_injection_thread.tsk,
> +	.thread_fn              = idle_injection_fn,
> +	.thread_should_run	= idle_injection_should_run,
> +	.setup                  = idle_injection_setup,
> +};

Why should we keep this structure at all and not have these four
assignments in the below routine itself ? It is unnecessarily copying
a bigger structure while it is required to copy only a part of it. And
then we keep wasting memory for this particular instance without any
use.

> +
> +/**
> + * idle_injection_register - idle injection init routine
> + * @cpumask: the list of CPUs to run the kthreads
> + * @name: the threads command name
> + *
> + * This is the initialization function in charge of creating the
> + * kthreads, initializing the timer and allocate the structures.  It
> + * does not starts the idle injection cycles

Forgot full stop (.). Please check that across file

> + *
> + * Returns -ENOMEM if an allocation fails, or < 0 if the smpboot

It should be Return:

> + * kthread registering fails.
> + */
> +struct idle_injection_device *
> +idle_injection_register(struct cpumask *cpumask, const char *name)
> +{
> +	struct idle_injection_device *ii_dev;
> +	struct smp_hotplug_thread *smp_hotplug_thread;
> +	char *idle_injection_comm;
> +	int cpu, ret;
> +
> +	ret = -ENOMEM;

Maybe merge it earlier only ?

> +
> +	idle_injection_comm = kasprintf(GFP_KERNEL, "%s/%%u", name);
> +	if (!idle_injection_comm)
> +		goto out;
> +
> +	smp_hotplug_thread = kmemdup(&idle_injection_threads,
> +				     sizeof(*smp_hotplug_thread),
> +				     GFP_KERNEL);
> +	if (!smp_hotplug_thread)
> +		goto out_free_thread_comm;
> +
> +	smp_hotplug_thread->thread_comm	= idle_injection_comm;
> +
> +	ii_dev = kzalloc(sizeof(*ii_dev),
> +					GFP_KERNEL);

Accidental line break ?

> +	if (!ii_dev)
> +		goto out_free_smp_hotplug;
> +
> +	ii_dev->smp_hotplug_thread = smp_hotplug_thread;
> +
> +	hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +
> +	ii_dev->timer.function = idle_injection_wakeup_fn;
> +
> +	for_each_cpu(cpu, cpumask)
> +		per_cpu(idle_injection_device, cpu) = ii_dev;
> +
> +	ret = smpboot_register_percpu_thread_cpumask(smp_hotplug_thread,
> +						     cpumask);

This creates the thread for all online CPUs and unparks only the one
in the cpumask. I am not sure how the smpboot stuff works, but this
looks somewhat wrong to me, we may end up creating multiple threads
per CPU even if this function is called twice for non-intersecting cpu
masks.

> +	if (ret)
> +		goto out_free_idle_inject;
> +
> +	return ii_dev;
> +
> +out_free_idle_inject:
> +	kfree(ii_dev);

What about resetting per-cpu idle_injection_device ? You missed the
same in unregister routine below as well ?

> +out_free_smp_hotplug:
> +	kfree(smp_hotplug_thread);
> +out_free_thread_comm:
> +	kfree(idle_injection_comm);
> +out:
> +	return ERR_PTR(ret);
> +}
> +
> +/**
> + * idle_injection_unregister - Unregister the idle injection device
> + * @ii_dev: a pointer to an idle injection device
> + *
> + * The function is in charge of stopping the idle injections,
> + * unregister the kthreads and free the allocated memory in the
> + * register function.
> + */
> +void idle_injection_unregister(struct idle_injection_device *ii_dev)
> +{
> +	struct smp_hotplug_thread *smp_hotplug_thread;
> +
> +	idle_injection_stop(ii_dev);
> +	smp_hotplug_thread = ii_dev->smp_hotplug_thread;
> +	smpboot_unregister_percpu_thread(smp_hotplug_thread);
> +	kfree(smp_hotplug_thread->thread_comm);
> +	kfree(smp_hotplug_thread);
> +	kfree(ii_dev);

Ideally it is much more readable if the ordering here is exactly
opposite of how things are done in registration time. You may need to
change the order in which things are allocated, and that would be
worth it :)

> +}
> diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
> new file mode 100644
> index 0000000..084b999
> --- /dev/null
> +++ b/include/linux/idle_injection.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Linaro Ltd
> + *
> + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> + *
> + */
> +#ifndef __IDLE_INJECTION_H__
> +#define __IDLE_INJECTION_H__
> +
> +/* private idle injection device structure */
> +struct idle_injection_device;
> +
> +/**
> + * idle_injection_register - allocates and initializes an idle_injection_device
> + * @cpumask: all CPUs with a idle injection kthreads
> + * @name: a const string giving the kthread name
> + *
> + * Returns a pointer to a idle_injection_device, ERR_PTR otherwise.

This needs to be written as "Return: XXXX.", else it wouldn't get
documented properly in kernel doc.

And I am wondering on why you have added all these kernel doc comments
in the .h file ? What will kernel doc look like as we will have to doc
comments for the same function ? Maybe try generating the doc and you
will see how it looks.

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-11  9:32   ` Viresh Kumar
@ 2018-05-11 11:55     ` Daniel Lezcano
  2018-05-15  5:12       ` Viresh Kumar
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2018-05-11 11:55 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

On Fri, May 11, 2018 at 03:02:21PM +0530, viresh kumar wrote:
> On 10-05-18, 14:26, Daniel Lezcano wrote:
> > Initially, the cpu_cooling device for ARM was changed by adding a new
> > policy inserting idle cycles. The intel_powerclamp driver does a
> > similar action.
> > 
> > Instead of implementing idle injections privately in the cpu_cooling
> > device, move the idle injection code in a dedicated framework and give
> > the opportunity to other frameworks to make use of it.
> 
> Maybe move the above explanation in the comments section below, as it
> doesn't belong to the commit log really.

Yes that makes sense.
 
[ ... ]

> > diff --git a/drivers/powercap/idle_injection.c b/drivers/powercap/idle_injection.c
> > new file mode 100644
> > index 0000000..825ffac
> > --- /dev/null
> > +++ b/drivers/powercap/idle_injection.c
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * drivers/powercap/idle_injection.c
> > + *
> 
> What's the purpose of this particular line ? Yeah, I have seen it
> quite a number of times and might have added that myself as well
> somewhere :)

I think it is a hundredth monkey effect :)
 
[ ... ]

> > +	/*
> > +	 * Boolean used by the smpboot mainloop and used as a flip-flop
> 
> s/mainloop/main loop/ ?
> 
> > +	 * in this function
> > +	 */
> > +	iit->should_run = 0;
> 
> What is the purpose of this field ? Just wanted to check on how
> smpboot stuff uses it.

This field is used as a boolean for the smpboot main loop.

You should look at the function smpboot_thread_fn().

The function idle_injection_thread_fn() idle is called every idle injection
period, sets a timer for the next idle injection deadline and tells the
smpboot main loop to schedule. The way to tell the smpboot main loop to
schedule and not call the idle_injection_thread_fn() again is by setting this
boolean to false.

static int smpboot_thread_fn(void *data)
{
	while (1) {

		[ ... ]

		if (!ht->thread_should_run(td->cpu)) {
			preempt_enable_no_resched();
			schedule();
		} else {
			__set_current_state(TASK_RUNNING);
			preempt_enable();
			ht->thread_fn(td->cpu);
		}

		[ ... ]
	}
}

[ ... ]

> > + */
> > +void idle_injection_set_duration(struct idle_injection_device *ii_dev,
> > +				 unsigned int run_duration_ms,
> > +				 unsigned int idle_duration_ms)
> > +{
> > +	atomic_set(&ii_dev->run_duration_ms, run_duration_ms);
> > +	atomic_set(&ii_dev->idle_duration_ms, idle_duration_ms);
> > +}
> > +
> > +
> 
> Two blank lines here ?

Ah, yes.

[ ... ]

> > +int idle_injection_start(struct idle_injection_device *ii_dev)
> > +{
> > +	if (!atomic_read(&ii_dev->idle_duration_ms))
> > +		return -EINVAL;
> > +
> > +	if (!atomic_read(&ii_dev->run_duration_ms))
> > +		return -EINVAL;
> 
> You are required to have them as atomic variables to take care of the
> races while they are set ? 

The race is when you change the values, while the idle injection is acting and
you read those values in the idle injection thread function.

> What about getting the durations as arguments to this routine then ?

May be I missed your point but I don't think that will change the above.
 
[ ... ]

> > +/*
> > + * idle_injection_threads - smp hotplug threads ops
> > + */
> > +static struct smp_hotplug_thread idle_injection_threads = {
> > +	.store                  = &idle_injection_thread.tsk,
> > +	.thread_fn              = idle_injection_fn,
> > +	.thread_should_run	= idle_injection_should_run,
> > +	.setup                  = idle_injection_setup,
> > +};
> 
> Why should we keep this structure at all and not have these four
> assignments in the below routine itself ? It is unnecessarily copying
> a bigger structure while it is required to copy only a part of it. And
> then we keep wasting memory for this particular instance without any
> use.

With your comment below about registering the smpboot threads with a cpumask, a
single structure should be used, I will fix this.

> > +
> > +/**
> > + * idle_injection_register - idle injection init routine
> > + * @cpumask: the list of CPUs to run the kthreads
> > + * @name: the threads command name
> > + *
> > + * This is the initialization function in charge of creating the
> > + * kthreads, initializing the timer and allocate the structures.  It
> > + * does not starts the idle injection cycles
> 
> Forgot full stop (.). Please check that across file

Oops, right. I rememeber you did a similar comment on the previous version.

> > + *
> > + * Returns -ENOMEM if an allocation fails, or < 0 if the smpboot
> 
> It should be Return:

Ok.
 
> > + * kthread registering fails.
> > + */
> > +struct idle_injection_device *
> > +idle_injection_register(struct cpumask *cpumask, const char *name)
> > +{
> > +	struct idle_injection_device *ii_dev;
> > +	struct smp_hotplug_thread *smp_hotplug_thread;
> > +	char *idle_injection_comm;
> > +	int cpu, ret;
> > +
> > +	ret = -ENOMEM;
> 
> Maybe merge it earlier only ?

What do you mean ? int ret = -ENOMEM ?

> > +
> > +	idle_injection_comm = kasprintf(GFP_KERNEL, "%s/%%u", name);
> > +	if (!idle_injection_comm)
> > +		goto out;
> > +
> > +	smp_hotplug_thread = kmemdup(&idle_injection_threads,
> > +				     sizeof(*smp_hotplug_thread),
> > +				     GFP_KERNEL);
> > +	if (!smp_hotplug_thread)
> > +		goto out_free_thread_comm;
> > +
> > +	smp_hotplug_thread->thread_comm	= idle_injection_comm;
> > +
> > +	ii_dev = kzalloc(sizeof(*ii_dev),
> > +					GFP_KERNEL);
> 
> Accidental line break ?

grumf ...

> > +	if (!ii_dev)
> > +		goto out_free_smp_hotplug;
> > +
> > +	ii_dev->smp_hotplug_thread = smp_hotplug_thread;
> > +
> > +	hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +
> > +	ii_dev->timer.function = idle_injection_wakeup_fn;
> > +
> > +	for_each_cpu(cpu, cpumask)
> > +		per_cpu(idle_injection_device, cpu) = ii_dev;
> > +
> > +	ret = smpboot_register_percpu_thread_cpumask(smp_hotplug_thread,
> > +						     cpumask);
> 
> This creates the thread for all online CPUs and unparks only the one
> in the cpumask. I am not sure how the smpboot stuff works, but this
> looks somewhat wrong to me, we may end up creating multiple threads
> per CPU even if this function is called twice for non-intersecting cpu
> masks.

Good point! That must be fixed, calling idle_injection_register()
one time and idle_injection_start() with a cpumask would be more convenient.

> > +	if (ret)
> > +		goto out_free_idle_inject;
> > +
> > +	return ii_dev;
> > +
> > +out_free_idle_inject:
> > +	kfree(ii_dev);
> 
> What about resetting per-cpu idle_injection_device ? You missed the
> same in unregister routine below as well ?

Ok.

> > +out_free_smp_hotplug:
> > +	kfree(smp_hotplug_thread);
> > +out_free_thread_comm:
> > +	kfree(idle_injection_comm);
> > +out:
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +/**
> > + * idle_injection_unregister - Unregister the idle injection device
> > + * @ii_dev: a pointer to an idle injection device
> > + *
> > + * The function is in charge of stopping the idle injections,
> > + * unregister the kthreads and free the allocated memory in the
> > + * register function.
> > + */
> > +void idle_injection_unregister(struct idle_injection_device *ii_dev)
> > +{
> > +	struct smp_hotplug_thread *smp_hotplug_thread;
> > +
> > +	idle_injection_stop(ii_dev);
> > +	smp_hotplug_thread = ii_dev->smp_hotplug_thread;
> > +	smpboot_unregister_percpu_thread(smp_hotplug_thread);
> > +	kfree(smp_hotplug_thread->thread_comm);
> > +	kfree(smp_hotplug_thread);
> > +	kfree(ii_dev);
> 
> Ideally it is much more readable if the ordering here is exactly
> opposite of how things are done in registration time. You may need to
> change the order in which things are allocated, and that would be
> worth it :)

Ok.

> > +}
> > diff --git a/include/linux/idle_injection.h b/include/linux/idle_injection.h
> > new file mode 100644
> > index 0000000..084b999
> > --- /dev/null
> > +++ b/include/linux/idle_injection.h
> > @@ -0,0 +1,62 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 Linaro Ltd
> > + *
> > + * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
> > + *
> > + */
> > +#ifndef __IDLE_INJECTION_H__
> > +#define __IDLE_INJECTION_H__
> > +
> > +/* private idle injection device structure */
> > +struct idle_injection_device;
> > +
> > +/**
> > + * idle_injection_register - allocates and initializes an idle_injection_device
> > + * @cpumask: all CPUs with a idle injection kthreads
> > + * @name: a const string giving the kthread name
> > + *
> > + * Returns a pointer to a idle_injection_device, ERR_PTR otherwise.
> 
> This needs to be written as "Return: XXXX.", else it wouldn't get
> documented properly in kernel doc.
> 
> And I am wondering on why you have added all these kernel doc comments
> in the .h file ? What will kernel doc look like as we will have to doc
> comments for the same function ? Maybe try generating the doc and you
> will see how it looks.

Hmm, right. I will remove the documentation in the header.

Thanks for the review.

  -- Daniel

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-11 11:55     ` Daniel Lezcano
@ 2018-05-15  5:12       ` Viresh Kumar
  2018-05-15  8:01         ` Daniel Lezcano
  0 siblings, 1 reply; 7+ messages in thread
From: Viresh Kumar @ 2018-05-15  5:12 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

On 11-05-18, 13:55, Daniel Lezcano wrote:
> On Fri, May 11, 2018 at 03:02:21PM +0530, viresh kumar wrote:
> > On 10-05-18, 14:26, Daniel Lezcano wrote:
> > > +int idle_injection_start(struct idle_injection_device *ii_dev)
> > > +{
> > > +	if (!atomic_read(&ii_dev->idle_duration_ms))
> > > +		return -EINVAL;
> > > +
> > > +	if (!atomic_read(&ii_dev->run_duration_ms))
> > > +		return -EINVAL;
> > 
> > You are required to have them as atomic variables to take care of the
> > races while they are set ? 
> 
> The race is when you change the values, while the idle injection is acting and
> you read those values in the idle injection thread function.
> 
> > What about getting the durations as arguments to this routine then ?
> 
> May be I missed your point but I don't think that will change the above.

Well, it can. Can you explain the kind of use-cases you have in mind ?

For example, what I assumed to be the usecase was:

idle_injection_start(iidev, idle_duration, run_duration);

and then at some point of time:

idle_injection_stop(iidev);


With this, you would be required to stop the idle injection stuff to
reconfigure the durations. And then you will never have a race which
you mentioned above.

What you are trying to do is something like this:

idle_injection_set_duration(idle_duration, run_duration);
idle_injection_start(iidev);

and then at some point of time:
idle_injection_set_duration(idle_duration2, run_duration2);

and then at some point of time:
idle_injection_stop(iidev);

I am not sure if we would ever want to do something like this. Or if
stopping the idle injection to start it again is that bad of an idea.

> > > +struct idle_injection_device *
> > > +idle_injection_register(struct cpumask *cpumask, const char *name)
> > > +{
> > > +	struct idle_injection_device *ii_dev;
> > > +	struct smp_hotplug_thread *smp_hotplug_thread;
> > > +	char *idle_injection_comm;
> > > +	int cpu, ret;
> > > +
> > > +	ret = -ENOMEM;
> > 
> > Maybe merge it earlier only ?
> 
> What do you mean ? int ret = -ENOMEM ?

Yes.

-- 
viresh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH V2] powercap/drivers/idle_injection: Add an idle injection framework
  2018-05-15  5:12       ` Viresh Kumar
@ 2018-05-15  8:01         ` Daniel Lezcano
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Lezcano @ 2018-05-15  8:01 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
	linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson

On Tue, May 15, 2018 at 10:42:41AM +0530, viresh kumar wrote:
> On 11-05-18, 13:55, Daniel Lezcano wrote:
> > On Fri, May 11, 2018 at 03:02:21PM +0530, viresh kumar wrote:
> > > On 10-05-18, 14:26, Daniel Lezcano wrote:
> > > > +int idle_injection_start(struct idle_injection_device *ii_dev)
> > > > +{
> > > > +	if (!atomic_read(&ii_dev->idle_duration_ms))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (!atomic_read(&ii_dev->run_duration_ms))
> > > > +		return -EINVAL;
> > > 
> > > You are required to have them as atomic variables to take care of the
> > > races while they are set ? 
> > 
> > The race is when you change the values, while the idle injection is acting and
> > you read those values in the idle injection thread function.
> > 
> > > What about getting the durations as arguments to this routine then ?
> > 
> > May be I missed your point but I don't think that will change the above.
> 
> Well, it can. Can you explain the kind of use-cases you have in mind ?
> 
> For example, what I assumed to be the usecase was:
> 
> idle_injection_start(iidev, idle_duration, run_duration);
> 
> and then at some point of time:
> 
> idle_injection_stop(iidev);
> 
> 
> With this, you would be required to stop the idle injection stuff to
> reconfigure the durations. And then you will never have a race which
> you mentioned above.
> 
> What you are trying to do is something like this:
> 
> idle_injection_set_duration(idle_duration, run_duration);
> idle_injection_start(iidev);
> 
> and then at some point of time:
> idle_injection_set_duration(idle_duration2, run_duration2);

Here potentially hundred of times between a start and a stop, and at a high
rate.
 
> and then at some point of time:
> idle_injection_stop(iidev);
> 
> I am not sure if we would ever want to do something like this. Or if
> stopping the idle injection to start it again is that bad of an idea.

It sounds strange, it is like you change the speed of the car by stopping and
restarting the motor.
 
> > > > +struct idle_injection_device *
> > > > +idle_injection_register(struct cpumask *cpumask, const char *name)
> > > > +{
> > > > +	struct idle_injection_device *ii_dev;
> > > > +	struct smp_hotplug_thread *smp_hotplug_thread;
> > > > +	char *idle_injection_comm;
> > > > +	int cpu, ret;
> > > > +
> > > > +	ret = -ENOMEM;
> > > 
> > > Maybe merge it earlier only ?
> > 
> > What do you mean ? int ret = -ENOMEM ?
> 
> Yes.

Ok.

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-05-15  8:01 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-10 10:34 [PATCH] powercap/drivers/idle_injection: Add an idle injection framework Daniel Lezcano
2018-05-10 12:26 ` [PATCH V2] " Daniel Lezcano
2018-05-11  9:32   ` Viresh Kumar
2018-05-11 11:55     ` Daniel Lezcano
2018-05-15  5:12       ` Viresh Kumar
2018-05-15  8:01         ` Daniel Lezcano
2018-05-11  6:11 ` [PATCH] " kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.