Re: [PATCH v2 2/2] ksm: Provide support to use deferrable timers for scanner thread

[Andrew Morton]

On Fri, 25 Jul 2014 20:18:18 +0530 Chintan Pandya <cpandya@xxxxxxxxxxxxxx> wrote:

> KSM thread to scan pages is scheduled on definite timeout. That wakes
> up CPU from idle state and hence may affect the power consumption.
> Provide an optional support to use deferrable timer which suites
> low-power use-cases.
> 
> Typically, on our setup we observed, 10% less power consumption with
> some use-cases in which CPU goes to power collapse frequently. For
> example, playing audio while typically CPU remains idle.
> 
> To enable deferrable timers,
> $ echo 1 > /sys/kernel/mm/ksm/deferrable_timer
> 
> ...
>
> --- 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
> +deferrable_timer - whether to use deferrable timers or not
> +                 e.g. "echo 1 > /sys/kernel/mm/ksm/deferrable_timer"
> +                 Default: 0 (means, we are not using deferrable timers. Users
> +		 might want to set deferrable_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.)

Text indenting is odd.

> 
> ...
>  
> +static ssize_t deferrable_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);

Unhandled error.

> +	if (enable == 0 || enable == 1)
> +		use_deferrable_timer = enable;
> +
> +	return count;

Should return -EINVAL if `enable' is invalid.


--- a/Documentation/vm/ksm.txt~ksm-provide-support-to-use-deferrable-timers-for-scanner-thread-fix
+++ a/Documentation/vm/ksm.txt
@@ -88,12 +88,12 @@ pages_unshared   - how many pages unique
 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
 deferrable_timer - whether to use deferrable timers or not
-                 e.g. "echo 1 > /sys/kernel/mm/ksm/deferrable_timer"
-                 Default: 0 (means, we are not using deferrable timers. Users
-		 might want to set deferrable_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.)
+                   e.g. "echo 1 > /sys/kernel/mm/ksm/deferrable_timer"
+                   Default: 0 (means, we are not using deferrable timers. Users
+		   might want to set deferrable_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.
--- a/mm/ksm.c~ksm-provide-support-to-use-deferrable-timers-for-scanner-thread-fix
+++ a/mm/ksm.c
@@ -2202,10 +2202,11 @@ static ssize_t deferrable_timer_store(st
 	int err;
 
 	err = kstrtoul(buf, 10, &enable);
-
-	if (enable == 0 || enable == 1)
-		use_deferrable_timer = enable;
-
+	if (err < 0)
+		return err;
+	if (enable >= 1)
+		return -EINVAL;
+	use_deferrable_timer = enable;
 	return count;
 }
 KSM_ATTR(deferrable_timer);
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/