[PATCH] ksm: Provide support to use deferred timers for scanner thread

[PATCH] ksm: Provide support to use deferred timers for scanner thread

[Chintan Pandya]

KSM thread to scan pages is getting schedule on definite timeout.
That wakes up CPU from idle state and hence may affect the power
consumption. Provide an optional support to use deferred timer
which suites low-power use-cases.

To enable deferred timers,
$ echo 1 > /sys/kernel/mm/ksm/deferred_timer

Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
---
 Documentation/vm/ksm.txt |  7 ++++++
 mm/ksm.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/Documentation/vm/ksm.txt b/Documentation/vm/ksm.txt
index f34a8ee..f40b965 100644
--- a/Documentation/vm/ksm.txt
+++ b/Documentation/vm/ksm.txt
@@ -87,6 +87,13 @@ pages_sharing    - how many more sites are sharing them i.e. how much saved
 pages_unshared   - how many pages unique but repeatedly checked for merging
 pages_volatile   - how many pages changing too fast to be placed in a tree
 full_scans       - how many times all mergeable areas have been scanned
+deferred_timer   - whether to use deferred timers or not
+                 e.g. "echo 1 > /sys/kernel/mm/ksm/deferred_timer"
+                 Default: 0 (means, we are not using deferred timers. Users
+		 might want to set deferred_timer option if they donot want
+		 ksm thread to wakeup CPU to carryout ksm activities thus
+		 gaining on battery while compromising slightly on memory
+		 that could have been saved.)
 
 A high ratio of pages_sharing to pages_shared indicates good sharing, but
 a high ratio of pages_unshared to pages_sharing indicates wasted effort.
diff --git a/mm/ksm.c b/mm/ksm.c
index 346ddc9..e26ec3b 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -223,6 +223,9 @@ static unsigned int ksm_thread_pages_to_scan = 100;
 /* Milliseconds ksmd should sleep between batches */
 static unsigned int ksm_thread_sleep_millisecs = 20;
 
+/* Boolean to indicate whether to use deferred timer or not */
+static bool use_deferred_timer;
+
 #ifdef CONFIG_NUMA
 /* Zeroed when merging across nodes is not allowed */
 static unsigned int ksm_merge_across_nodes = 1;
@@ -1705,6 +1708,41 @@ static void ksm_do_scan(unsigned int scan_npages)
 	}
 }
 
+static void process_timeout(unsigned long __data)
+{
+	wake_up_process((struct task_struct *)__data);
+}
+
+static signed long __sched deferred_schedule_timeout(signed long timeout)
+{
+	struct timer_list timer;
+	unsigned long expire;
+
+	__set_current_state(TASK_INTERRUPTIBLE);
+	if (timeout < 0) {
+		pr_err("schedule_timeout: wrong timeout value %lx\n",
+							timeout);
+		__set_current_state(TASK_RUNNING);
+		goto out;
+	}
+
+	expire = timeout + jiffies;
+
+	setup_deferrable_timer_on_stack(&timer, process_timeout,
+			(unsigned long)current);
+	mod_timer(&timer, expire);
+	schedule();
+	del_singleshot_timer_sync(&timer);
+
+	/* Remove the timer from the object tracker */
+	destroy_timer_on_stack(&timer);
+
+	timeout = expire - jiffies;
+
+out:
+	return timeout < 0 ? 0 : timeout;
+}
+
 static int ksmd_should_run(void)
 {
 	return (ksm_run & KSM_RUN_MERGE) && !list_empty(&ksm_mm_head.mm_list);
@@ -1725,7 +1763,11 @@ static int ksm_scan_thread(void *nothing)
 		try_to_freeze();
 
 		if (ksmd_should_run()) {
-			schedule_timeout_interruptible(
+			if (use_deferred_timer)
+				deferred_schedule_timeout(
+				msecs_to_jiffies(ksm_thread_sleep_millisecs));
+			else
+				schedule_timeout_interruptible(
 				msecs_to_jiffies(ksm_thread_sleep_millisecs));
 		} else {
 			wait_event_freezable(ksm_thread_wait,
@@ -2181,6 +2223,26 @@ static ssize_t run_store(struct kobject *kobj, struct kobj_attribute *attr,
 }
 KSM_ATTR(run);
 
+static ssize_t deferred_timer_show(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	return snprintf(buf, 8, "%d\n", use_deferred_timer);
+}
+
+static ssize_t deferred_timer_store(struct kobject *kobj,
+				     struct kobj_attribute *attr,
+				     const char *buf, size_t count)
+{
+	unsigned long enable;
+	int err;
+
+	err = kstrtoul(buf, 10, &enable);
+	use_deferred_timer = enable;
+
+	return count;
+}
+KSM_ATTR(deferred_timer);
+
 #ifdef CONFIG_NUMA
 static ssize_t merge_across_nodes_show(struct kobject *kobj,
 				struct kobj_attribute *attr, char *buf)
@@ -2293,6 +2355,7 @@ static struct attribute *ksm_attrs[] = {
 	&pages_unshared_attr.attr,
 	&pages_volatile_attr.attr,
 	&full_scans_attr.attr,
+	&deferred_timer_attr.attr,
 #ifdef CONFIG_NUMA
 	&merge_across_nodes_attr.attr,
 #endif
-- 
1.8.2.1

Re: [PATCH] ksm: Provide support to use deferred timers for scanner thread

[Andrew Morton]

On Wed, 23 Jul 2014 14:41:32 +0530 Chintan Pandya <cpandya@codeaurora.org> wrote:

> KSM thread to scan pages is getting schedule on definite timeout.
> That wakes up CPU from idle state and hence may affect the power
> consumption. Provide an optional support to use deferred timer
> which suites low-power use-cases.

Do you have any data on the effectiveness of this patch?  Because if it
makes no useful difference, we shouldn't merge it!

> To enable deferred timers,
> $ echo 1 > /sys/kernel/mm/ksm/deferred_timer

It would be preferable to do this unconditionally (or automatically
somehow) rather than adding yet another weird knob.

> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
> ---
>  Documentation/vm/ksm.txt |  7 ++++++
>  mm/ksm.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/vm/ksm.txt b/Documentation/vm/ksm.txt
> index f34a8ee..f40b965 100644
> --- a/Documentation/vm/ksm.txt
> +++ b/Documentation/vm/ksm.txt
> @@ -87,6 +87,13 @@ pages_sharing    - how many more sites are sharing them i.e. how much saved
>  pages_unshared   - how many pages unique but repeatedly checked for merging
>  pages_volatile   - how many pages changing too fast to be placed in a tree
>  full_scans       - how many times all mergeable areas have been scanned
> +deferred_timer   - whether to use deferred timers or not
> +                 e.g. "echo 1 > /sys/kernel/mm/ksm/deferred_timer"
> +                 Default: 0 (means, we are not using deferred timers. Users
> +		 might want to set deferred_timer option if they donot want
> +		 ksm thread to wakeup CPU to carryout ksm activities thus
> +		 gaining on battery while compromising slightly on memory
> +		 that could have been saved.)
>  
>  A high ratio of pages_sharing to pages_shared indicates good sharing, but
>  a high ratio of pages_unshared to pages_sharing indicates wasted effort.
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 346ddc9..e26ec3b 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -223,6 +223,9 @@ static unsigned int ksm_thread_pages_to_scan = 100;
>  /* Milliseconds ksmd should sleep between batches */
>  static unsigned int ksm_thread_sleep_millisecs = 20;
>  
> +/* Boolean to indicate whether to use deferred timer or not */
> +static bool use_deferred_timer;
> +
>  #ifdef CONFIG_NUMA
>  /* Zeroed when merging across nodes is not allowed */
>  static unsigned int ksm_merge_across_nodes = 1;
> @@ -1705,6 +1708,41 @@ static void ksm_do_scan(unsigned int scan_npages)
>  	}
>  }
>  
> +static void process_timeout(unsigned long __data)
> +{
> +	wake_up_process((struct task_struct *)__data);
> +}
> +
> +static signed long __sched deferred_schedule_timeout(signed long timeout)

Should be called schedule_timeout_deferrable_interruptible() for
consistency.

And let's not start mixing "deferred" and "deferrable".

> +{
> +	struct timer_list timer;
> +	unsigned long expire;
> +
> +	__set_current_state(TASK_INTERRUPTIBLE);
> +	if (timeout < 0) {
> +		pr_err("schedule_timeout: wrong timeout value %lx\n",

		        ^^^^^^^^^^^^^^^^ copy-n-paste?
> +							timeout);
> +		__set_current_state(TASK_RUNNING);
> +		goto out;
> +	}
> +
> +	expire = timeout + jiffies;
> +
> +	setup_deferrable_timer_on_stack(&timer, process_timeout,
> +			(unsigned long)current);
> +	mod_timer(&timer, expire);
> +	schedule();
> +	del_singleshot_timer_sync(&timer);
> +
> +	/* Remove the timer from the object tracker */
> +	destroy_timer_on_stack(&timer);
> +
> +	timeout = expire - jiffies;
> +
> +out:
> +	return timeout < 0 ? 0 : timeout;
> +}

Methinks all this should be in kernel/timer.c (kernel/time/timer.c in
linux-next).  And it should be documented.  That means a separate patch
and review by Thomas Gleixner and probably others.

I haven't looked, but I expect a lot of schedule_timeout() callsites
could be converted to use such a thing.

>  static int ksmd_should_run(void)
>  {
>  	return (ksm_run & KSM_RUN_MERGE) && !list_empty(&ksm_mm_head.mm_list);
> @@ -1725,7 +1763,11 @@ static int ksm_scan_thread(void *nothing)
>  		try_to_freeze();
>  
>  		if (ksmd_should_run()) {
> -			schedule_timeout_interruptible(
> +			if (use_deferred_timer)
> +				deferred_schedule_timeout(
> +				msecs_to_jiffies(ksm_thread_sleep_millisecs));
> +			else
> +				schedule_timeout_interruptible(
>  				msecs_to_jiffies(ksm_thread_sleep_millisecs));
>  		} else {
>  			wait_event_freezable(ksm_thread_wait,
> @@ -2181,6 +2223,26 @@ static ssize_t run_store(struct kobject *kobj, struct kobj_attribute *attr,
>  }
>  KSM_ATTR(run);
>  
> +static ssize_t deferred_timer_show(struct kobject *kobj,
> +				    struct kobj_attribute *attr, char *buf)
> +{
> +	return snprintf(buf, 8, "%d\n", use_deferred_timer);
> +}
> +
> +static ssize_t deferred_timer_store(struct kobject *kobj,
> +				     struct kobj_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	unsigned long enable;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &enable);
> +	use_deferred_timer = enable;

We should check for legitimate values here.  ie: 0 or 1 only.

> +	return count;
> +}
> +KSM_ATTR(deferred_timer);
> +
>  #ifdef CONFIG_NUMA
>  static ssize_t merge_across_nodes_show(struct kobject *kobj,
>  				struct kobj_attribute *attr, char *buf)
> @@ -2293,6 +2355,7 @@ static struct attribute *ksm_attrs[] = {
>  	&pages_unshared_attr.attr,
>  	&pages_volatile_attr.attr,
>  	&full_scans_attr.attr,
> +	&deferred_timer_attr.attr,
>  #ifdef CONFIG_NUMA
>  	&merge_across_nodes_attr.attr,
>  #endif

Re: [PATCH] ksm: Provide support to use deferred timers for scanner thread

[Chintan Pandya]

Thanks Andrew for reviewing. This is my first upstream patch :)

On 07/24/2014 01:51 AM, Andrew Morton wrote:
> On Wed, 23 Jul 2014 14:41:32 +0530 Chintan Pandya<cpandya@codeaurora.org>  wrote:
>
>> KSM thread to scan pages is getting schedule on definite timeout.
>> That wakes up CPU from idle state and hence may affect the power
>> consumption. Provide an optional support to use deferred timer
>> which suites low-power use-cases.
>
> Do you have any data on the effectiveness of this patch?  Because if it
> makes no useful difference, we shouldn't merge it!

Typically, we observed 10% less power consumption with some use-cases 
which in which CPU goes to power collapse frequently. For example, 
playing Audio on SoC where typically CPU is idle.

I will mention this in commit text also.
>
>> To enable deferred timers,
>> $ echo 1>  /sys/kernel/mm/ksm/deferred_timer
>
> It would be preferable to do this unconditionally (or automatically
> somehow) rather than adding yet another weird knob.

This has some trade-off. We have observed that KSM does maximum savings 
when system is idle. Where power is not concern but memory saving is, we 
may want KSM timer as non-deferrable. Considering that, I preferred to 
provide knob.
>
>> Signed-off-by: Chintan Pandya<cpandya@codeaurora.org>
>> ---
>>   Documentation/vm/ksm.txt |  7 ++++++
>>   mm/ksm.c                 | 65 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/vm/ksm.txt b/Documentation/vm/ksm.txt
>> index f34a8ee..f40b965 100644
>> --- a/Documentation/vm/ksm.txt
>> +++ b/Documentation/vm/ksm.txt
>> @@ -87,6 +87,13 @@ pages_sharing    - how many more sites are sharing them i.e. how much saved
>>   pages_unshared   - how many pages unique but repeatedly checked for merging
>>   pages_volatile   - how many pages changing too fast to be placed in a tree
>>   full_scans       - how many times all mergeable areas have been scanned
>> +deferred_timer   - whether to use deferred timers or not
>> +                 e.g. "echo 1>  /sys/kernel/mm/ksm/deferred_timer"
>> +                 Default: 0 (means, we are not using deferred timers. Users
>> +		 might want to set deferred_timer option if they donot want
>> +		 ksm thread to wakeup CPU to carryout ksm activities thus
>> +		 gaining on battery while compromising slightly on memory
>> +		 that could have been saved.)
>>
>>   A high ratio of pages_sharing to pages_shared indicates good sharing, but
>>   a high ratio of pages_unshared to pages_sharing indicates wasted effort.
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index 346ddc9..e26ec3b 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -223,6 +223,9 @@ static unsigned int ksm_thread_pages_to_scan = 100;
>>   /* Milliseconds ksmd should sleep between batches */
>>   static unsigned int ksm_thread_sleep_millisecs = 20;
>>
>> +/* Boolean to indicate whether to use deferred timer or not */
>> +static bool use_deferred_timer;
>> +
>>   #ifdef CONFIG_NUMA
>>   /* Zeroed when merging across nodes is not allowed */
>>   static unsigned int ksm_merge_across_nodes = 1;
>> @@ -1705,6 +1708,41 @@ static void ksm_do_scan(unsigned int scan_npages)
>>   	}
>>   }
>>
>> +static void process_timeout(unsigned long __data)
>> +{
>> +	wake_up_process((struct task_struct *)__data);
>> +}
>> +
>> +static signed long __sched deferred_schedule_timeout(signed long timeout)
>
> Should be called schedule_timeout_deferrable_interruptible() for
> consistency.
>
> And let's not start mixing "deferred" and "deferrable".

Sure. I will use schedule_timeout_deferrable_interruptible().

>
>> +{
>> +	struct timer_list timer;
>> +	unsigned long expire;
>> +
>> +	__set_current_state(TASK_INTERRUPTIBLE);
>> +	if (timeout<  0) {
>> +		pr_err("schedule_timeout: wrong timeout value %lx\n",
>
> 		^^^^^^^^^^^^^^^^ copy-n-paste?

My bad.

>> +							timeout);
>> +		__set_current_state(TASK_RUNNING);
>> +		goto out;
>> +	}
>> +
>> +	expire = timeout + jiffies;
>> +
>> +	setup_deferrable_timer_on_stack(&timer, process_timeout,
>> +			(unsigned long)current);
>> +	mod_timer(&timer, expire);
>> +	schedule();
>> +	del_singleshot_timer_sync(&timer);
>> +
>> +	/* Remove the timer from the object tracker */
>> +	destroy_timer_on_stack(&timer);
>> +
>> +	timeout = expire - jiffies;
>> +
>> +out:
>> +	return timeout<  0 ? 0 : timeout;
>> +}
>
> Methinks all this should be in kernel/timer.c (kernel/time/timer.c in
> linux-next).  And it should be documented.  That means a separate patch
> and review by Thomas Gleixner and probably others.

Ok. I will break this patch and upload again.

>
> I haven't looked, but I expect a lot of schedule_timeout() callsites
> could be converted to use such a thing.

 From our internal power experiments, we have seen only KSM waking up 
CPUs from sleep. Not sure about other use-cases. But an API would help all.

>
>>   static int ksmd_should_run(void)
>>   {
>>   	return (ksm_run&  KSM_RUN_MERGE)&&  !list_empty(&ksm_mm_head.mm_list);
>> @@ -1725,7 +1763,11 @@ static int ksm_scan_thread(void *nothing)
>>   		try_to_freeze();
>>
>>   		if (ksmd_should_run()) {
>> -			schedule_timeout_interruptible(
>> +			if (use_deferred_timer)
>> +				deferred_schedule_timeout(
>> +				msecs_to_jiffies(ksm_thread_sleep_millisecs));
>> +			else
>> +				schedule_timeout_interruptible(
>>   				msecs_to_jiffies(ksm_thread_sleep_millisecs));
>>   		} else {
>>   			wait_event_freezable(ksm_thread_wait,
>> @@ -2181,6 +2223,26 @@ static ssize_t run_store(struct kobject *kobj, struct kobj_attribute *attr,
>>   }
>>   KSM_ATTR(run);
>>
>> +static ssize_t deferred_timer_show(struct kobject *kobj,
>> +				    struct kobj_attribute *attr, char *buf)
>> +{
>> +	return snprintf(buf, 8, "%d\n", use_deferred_timer);
>> +}
>> +
>> +static ssize_t deferred_timer_store(struct kobject *kobj,
>> +				     struct kobj_attribute *attr,
>> +				     const char *buf, size_t count)
>> +{
>> +	unsigned long enable;
>> +	int err;
>> +
>> +	err = kstrtoul(buf, 10,&enable);
>> +	use_deferred_timer = enable;
>
> We should check for legitimate values here.  ie: 0 or 1 only.

Ok.

>
>> +	return count;
>> +}
>> +KSM_ATTR(deferred_timer);
>> +
>>   #ifdef CONFIG_NUMA
>>   static ssize_t merge_across_nodes_show(struct kobject *kobj,
>>   				struct kobj_attribute *attr, char *buf)
>> @@ -2293,6 +2355,7 @@ static struct attribute *ksm_attrs[] = {
>>   	&pages_unshared_attr.attr,
>>   	&pages_volatile_attr.attr,
>>   	&full_scans_attr.attr,
>> +	&deferred_timer_attr.attr,
>>   #ifdef CONFIG_NUMA
>>   	&merge_across_nodes_attr.attr,
>>   #endif
>

I will re-share new patch set incorporating above comments, in some days.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation