Messages in this thread | | | Date | Thu, 24 Jul 2014 18:42:57 +0530 | From | Chintan Pandya <> | Subject | Re: [PATCH] ksm: Provide support to use deferred timers for scanner thread |
| |
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
| |