[PATCH] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[PATCH] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim]

From: "cleaeye.kim" <cleaneye.kim@lge.com>

The synchronous synchronize_rcu in wakeup_source_remove makes user process
which writes to /sys/kernel/wake_unlock blocked sometimes.

For example, when android eventhub tries to release wakelock,
this blocking process can occur, and eventhub can't get input event
for a while.

Using workqueue instead of direct function call at pm_wake_unlock
can prevent this unnecessary delay of an user space process.

Signed-off-by: cleaeye.kim <cleaneye.kim@lge.com>
---
 kernel/power/wakelock.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 019069c..ea10baa 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/rbtree.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 #include "power.h"
 
@@ -96,7 +97,7 @@ static inline void wakelocks_lru_most_recent(struct wakelock *wl)
 	list_move(&wl->lru, &wakelocks_lru_list);
 }
 
-static void wakelocks_gc(void)
+static void wakelocks_gc(struct work_struct *work)
 {
 	struct wakelock *wl, *aux;
 	ktime_t now;
@@ -105,6 +106,7 @@ static void wakelocks_gc(void)
 		return;
 
 	now = ktime_get();
+	mutex_lock(&wakelocks_lock);
 	list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
 		u64 idle_time_ns;
 		bool active;
@@ -126,12 +128,15 @@ static void wakelocks_gc(void)
 			decrement_wakelocks_number();
 		}
 	}
+	mutex_unlock(&wakelocks_lock);
 	wakelocks_gc_count = 0;
 }
+
+static DECLARE_WORK(wakelock_work, wakelocks_gc);
 #else /* !CONFIG_PM_WAKELOCKS_GC */
 static inline void wakelocks_lru_add(struct wakelock *wl) {}
 static inline void wakelocks_lru_most_recent(struct wakelock *wl) {}
-static inline void wakelocks_gc(void) {}
+static void wakelocks_gc(struct worksturct) {}
 #endif /* !CONFIG_PM_WAKELOCKS_GC */
 
 static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
@@ -260,7 +265,7 @@ int pm_wake_unlock(const char *buf)
 	__pm_relax(&wl->ws);
 
 	wakelocks_lru_most_recent(wl);
-	wakelocks_gc();
+	schedule_work(&wakelock_work);
 
  out:
 	mutex_unlock(&wakelocks_lock);
-- 
1.7.9.5

Re: [PATCH] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[Krzysztof Kozlowski]

2015-07-01 11:48 GMT+09:00 SungEun Kim <cleaneye.kim@lge.com>:
> From: "cleaeye.kim" <cleaneye.kim@lge.com>
>
> The synchronous synchronize_rcu in wakeup_source_remove makes user process
> which writes to /sys/kernel/wake_unlock blocked sometimes.
>
> For example, when android eventhub tries to release wakelock,
> this blocking process can occur, and eventhub can't get input event
> for a while.
>
> Using workqueue instead of direct function call at pm_wake_unlock
> can prevent this unnecessary delay of an user space process.
>
> Signed-off-by: cleaeye.kim <cleaneye.kim@lge.com>

Hi,

You send this patch for third time, without changelog and any
versioning. The signed-off and from fields look incorrect (no real
name?). What is more important I have doubts that it even compiles
(see below).

Could you follow the Documentation/SubmittingPatches?


> ---
>  kernel/power/wakelock.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 019069c..ea10baa 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -17,6 +17,7 @@
>  #include <linux/list.h>
>  #include <linux/rbtree.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>
>  #include "power.h"
>
> @@ -96,7 +97,7 @@ static inline void wakelocks_lru_most_recent(struct wakelock *wl)
>         list_move(&wl->lru, &wakelocks_lru_list);
>  }
>
> -static void wakelocks_gc(void)
> +static void wakelocks_gc(struct work_struct *work)
>  {
>         struct wakelock *wl, *aux;
>         ktime_t now;
> @@ -105,6 +106,7 @@ static void wakelocks_gc(void)
>                 return;
>
>         now = ktime_get();
> +       mutex_lock(&wakelocks_lock);
>         list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
>                 u64 idle_time_ns;
>                 bool active;
> @@ -126,12 +128,15 @@ static void wakelocks_gc(void)
>                         decrement_wakelocks_number();
>                 }
>         }
> +       mutex_unlock(&wakelocks_lock);
>         wakelocks_gc_count = 0;
>  }
> +
> +static DECLARE_WORK(wakelock_work, wakelocks_gc);
>  #else /* !CONFIG_PM_WAKELOCKS_GC */
>  static inline void wakelocks_lru_add(struct wakelock *wl) {}
>  static inline void wakelocks_lru_most_recent(struct wakelock *wl) {}
> -static inline void wakelocks_gc(void) {}
> +static void wakelocks_gc(struct worksturct) {}

worksturct? Does it compile?

Best regards,
Krzysztof

Re: [PATCH] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[Krzysztof Kozłowski]

2015-07-01 11:48 GMT+09:00 SungEun Kim <cleaneye.kim@lge.com>:
> From: "cleaeye.kim" <cleaneye.kim@lge.com>
>
> The synchronous synchronize_rcu in wakeup_source_remove makes user process
> which writes to /sys/kernel/wake_unlock blocked sometimes.
>
> For example, when android eventhub tries to release wakelock,
> this blocking process can occur, and eventhub can't get input event
> for a while.
>
> Using workqueue instead of direct function call at pm_wake_unlock
> can prevent this unnecessary delay of an user space process.
>
> Signed-off-by: cleaeye.kim <cleaneye.kim@lge.com>

Hi,

You send this patch for third time, without changelog and any
versioning. Please put proper vesion indicating what have changed. Do
not resend continuously (unless you encountered some SMTP problems?).

The signed-off and from fields look incorrect (no real name?). What is
more important I have doubts that it even compiles (see below).

Could you follow the Documentation/SubmittingPatches?


> ---
>  kernel/power/wakelock.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 019069c..ea10baa 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -17,6 +17,7 @@
>  #include <linux/list.h>
>  #include <linux/rbtree.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>
>  #include "power.h"
>
> @@ -96,7 +97,7 @@ static inline void wakelocks_lru_most_recent(struct wakelock *wl)
>         list_move(&wl->lru, &wakelocks_lru_list);
>  }
>
> -static void wakelocks_gc(void)
> +static void wakelocks_gc(struct work_struct *work)
>  {
>         struct wakelock *wl, *aux;
>         ktime_t now;
> @@ -105,6 +106,7 @@ static void wakelocks_gc(void)
>                 return;
>
>         now = ktime_get();
> +       mutex_lock(&wakelocks_lock);
>         list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
>                 u64 idle_time_ns;
>                 bool active;
> @@ -126,12 +128,15 @@ static void wakelocks_gc(void)
>                         decrement_wakelocks_number();
>                 }
>         }
> +       mutex_unlock(&wakelocks_lock);
>         wakelocks_gc_count = 0;
>  }
> +
> +static DECLARE_WORK(wakelock_work, wakelocks_gc);
>  #else /* !CONFIG_PM_WAKELOCKS_GC */
>  static inline void wakelocks_lru_add(struct wakelock *wl) {}
>  static inline void wakelocks_lru_most_recent(struct wakelock *wl) {}
> -static inline void wakelocks_gc(void) {}
> +static void wakelocks_gc(struct worksturct) {}

worksturct? Does it compile?

Best regards,
Krzysztof

Re: Re: [PATCH] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim(cleaneye.kim@lge.com)]

On 2015-07-01 오후 12:51, k.kozlowski.k@gmail.com wrote:
> 2015-07-01 11:48 GMT+09:00 SungEun Kim <cleaneye.kim@lge.com>:
>> From: "cleaeye.kim" <cleaneye.kim@lge.com>
>>
>> The synchronous synchronize_rcu in wakeup_source_remove makes user
> process
>> which writes to /sys/kernel/wake_unlock blocked sometimes.
>>
>> For example, when android eventhub tries to release wakelock,
>> this blocking process can occur, and eventhub can't get input event
>> for a while.
>>
>> Using workqueue instead of direct function call at pm_wake_unlock
>> can prevent this unnecessary delay of an user space process.
>>
>> Signed-off-by: cleaeye.kim <cleaneye.kim@lge.com>
> 
> Hi,
> 
> You send this patch for third time, without changelog and any
> versioning. The signed-off and from fields look incorrect (no real
> name?). What is more important I have doubts that it even compiles
> (see below).
> 
> Could you follow the Documentation/SubmittingPatches?
> 
> 

Hi,

I'm sorry for my lack knowledge of submitting patch.
I have read that document but not carefully.
I will correct signed-off and please forgive my mistake of three times
sending generously.

And, I have not done compiling with no CONFIG_PM_WAKELOCKS_GC .
It's my fault. I will send v2.

Thank you.
SungEun Kim

>> ---
>> kernel/power/wakelock.c | 11 ++++++++---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
>> index 019069c..ea10baa 100644
>> --- a/kernel/power/wakelock.c
>> +++ b/kernel/power/wakelock.c
>> @@ -17,6 +17,7 @@
>> #include <linux/list.h>
>> #include <linux/rbtree.h>
>> #include <linux/slab.h>
>> +#include <linux/workqueue.h>
>>
>> #include "power.h"
>>
>> @@ -96,7 +97,7 @@ static inline void wakelocks_lru_most_recent(struct
> wakelock *wl)
>> list_move(&wl->lru, &wakelocks_lru_list);
>> }
>>
>> -static void wakelocks_gc(void)
>> +static void wakelocks_gc(struct work_struct *work)
>> {
>> struct wakelock *wl, *aux;
>> ktime_t now;
>> @@ -105,6 +106,7 @@ static void wakelocks_gc(void)
>> return;
>>
>> now = ktime_get();
>> + mutex_lock(&wakelocks_lock);
>> list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
>> u64 idle_time_ns;
>> bool active;
>> @@ -126,12 +128,15 @@ static void wakelocks_gc(void)
>> decrement_wakelocks_number();
>> }
>> }
>> + mutex_unlock(&wakelocks_lock);
>> wakelocks_gc_count = 0;
>> }
>> +
>> +static DECLARE_WORK(wakelock_work, wakelocks_gc);
>> #else /* !CONFIG_PM_WAKELOCKS_GC */
>> static inline void wakelocks_lru_add(struct wakelock *wl) {}
>> static inline void wakelocks_lru_most_recent(struct wakelock *wl) {}
>> -static inline void wakelocks_gc(void) {}
>> +static void wakelocks_gc(struct worksturct) {}
> 
> worksturct? Does it compile?
> 
> Best regards,
> Krzysztof

[PATCH v2] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim]

From: "SungEun Kim" <cleaneye.kim@lge.com>

The synchronous synchronize_rcu in wakeup_source_remove makes user process
which writes to /sys/kernel/wake_unlock blocked sometimes.

For example, when android eventhub tries to release wakelock,
this blocking process can occur, and eventhub can't get input event
for a while.

Using workqueue instead of direct function call at pm_wake_unlock
can prevent this unnecessary delay of an user space process.

Signed-off-by: SungEun Kim <cleaneye.kim@lge.com>
---
 kernel/power/wakelock.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 019069c..5df6aa2 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/rbtree.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 #include "power.h"
 
@@ -83,7 +84,9 @@ static inline void decrement_wakelocks_number(void) {}
 #define WL_GC_COUNT_MAX	100
 #define WL_GC_TIME_SEC	300
 
+static void __wakelocks_gc(struct work_struct *work);
 static LIST_HEAD(wakelocks_lru_list);
+static DECLARE_WORK(wakelock_work, __wakelocks_gc);
 static unsigned int wakelocks_gc_count;
 
 static inline void wakelocks_lru_add(struct wakelock *wl)
@@ -96,7 +99,7 @@ static inline void wakelocks_lru_most_recent(struct wakelock *wl)
 	list_move(&wl->lru, &wakelocks_lru_list);
 }
 
-static void wakelocks_gc(void)
+static void __wakelocks_gc(struct work_struct *work)
 {
 	struct wakelock *wl, *aux;
 	ktime_t now;
@@ -105,6 +108,7 @@ static void wakelocks_gc(void)
 		return;
 
 	now = ktime_get();
+	mutex_lock(&wakelocks_lock);
 	list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
 		u64 idle_time_ns;
 		bool active;
@@ -126,8 +130,14 @@ static void wakelocks_gc(void)
 			decrement_wakelocks_number();
 		}
 	}
+	mutex_unlock(&wakelocks_lock);
 	wakelocks_gc_count = 0;
 }
+
+static void wakelocks_gc(void)
+{
+	schedule_work(&wakelock_work);
+}
 #else /* !CONFIG_PM_WAKELOCKS_GC */
 static inline void wakelocks_lru_add(struct wakelock *wl) {}
 static inline void wakelocks_lru_most_recent(struct wakelock *wl) {}
-- 
1.7.9.5

Re: [PATCH v2] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[Rafael J. Wysocki]

On Wednesday, July 01, 2015 05:28:48 PM SungEun Kim wrote:
> From: "SungEun Kim" <cleaneye.kim@lge.com>
> 
> The synchronous synchronize_rcu in wakeup_source_remove makes user process
> which writes to /sys/kernel/wake_unlock blocked sometimes.
> 
> For example, when android eventhub tries to release wakelock,
> this blocking process can occur, and eventhub can't get input event
> for a while.
> 
> Using workqueue instead of direct function call at pm_wake_unlock
> can prevent this unnecessary delay of an user space process.

The idea is defendable, but the patch is too simple.

For example, if the garbage collection is in progress, it is not useful
to start a new one.

Also the incrementation and clearing of wakelocks_gc_count should be under
the lock.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Re: [PATCH v2] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim(cleaneye.kim@lge.com)]

On 2015-07-03 오전 9:15, Rafael J. Wysocki wrote:
> On Wednesday, July 01, 2015 05:28:48 PM SungEun Kim wrote:
>> From: "SungEun Kim" <cleaneye.kim@lge.com>
>>
>> The synchronous synchronize_rcu in wakeup_source_remove makes user
> process
>> which writes to /sys/kernel/wake_unlock blocked sometimes.
>>
>> For example, when android eventhub tries to release wakelock,
>> this blocking process can occur, and eventhub can't get input event
>> for a while.
>>
>> Using workqueue instead of direct function call at pm_wake_unlock
>> can prevent this unnecessary delay of an user space process.
> 
> The idea is defendable, but the patch is too simple.
> 
> For example, if the garbage collection is in progress, it is not useful
> to start a new one.
> 
> Also the incrementation and clearing of wakelocks_gc_count should be under
> the lock.

Thank you for your advices.
I will correct and amend my patch and then submit v3 patch.

Thank you.
SungEun Kim

> 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v3] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim]

From: "cleaeye.kim" <cleaneye.kim@lge.com>

The synchronous synchronize_rcu in wakeup_source_remove makes user process
which writes to /sys/kernel/wake_unlock blocked sometimes.

For example, when android eventhub tries to release wakelock,
this blocking process can occur, and eventhub can't get input event
for a while.

Using workqueue instead of direct function call at pm_wake_unlock
can prevent this unnecessary delay of an user space process.

Signed-off-by: SungEun Kim <cleaneye.kim@lge.com>
---
 kernel/power/wakelock.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
index 019069c..1896386 100644
--- a/kernel/power/wakelock.c
+++ b/kernel/power/wakelock.c
@@ -17,6 +17,7 @@
 #include <linux/list.h>
 #include <linux/rbtree.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 #include "power.h"
 
@@ -83,7 +84,9 @@ static inline void decrement_wakelocks_number(void) {}
 #define WL_GC_COUNT_MAX	100
 #define WL_GC_TIME_SEC	300
 
+static void __wakelocks_gc(struct work_struct *work);
 static LIST_HEAD(wakelocks_lru_list);
+static DECLARE_WORK(wakelock_work, __wakelocks_gc);
 static unsigned int wakelocks_gc_count;
 
 static inline void wakelocks_lru_add(struct wakelock *wl)
@@ -96,13 +99,12 @@ static inline void wakelocks_lru_most_recent(struct wakelock *wl)
 	list_move(&wl->lru, &wakelocks_lru_list);
 }
 
-static void wakelocks_gc(void)
+static void __wakelocks_gc(struct work_struct *work)
 {
 	struct wakelock *wl, *aux;
 	ktime_t now;
 
-	if (++wakelocks_gc_count <= WL_GC_COUNT_MAX)
-		return;
+	mutex_lock(&wakelocks_lock);
 
 	now = ktime_get();
 	list_for_each_entry_safe_reverse(wl, aux, &wakelocks_lru_list, lru) {
@@ -127,6 +129,16 @@ static void wakelocks_gc(void)
 		}
 	}
 	wakelocks_gc_count = 0;
+
+	mutex_unlock(&wakelocks_lock);
+}
+
+static void wakelocks_gc(void)
+{
+	if (++wakelocks_gc_count <= WL_GC_COUNT_MAX)
+		return;
+
+	schedule_work(&wakelock_work);
 }
 #else /* !CONFIG_PM_WAKELOCKS_GC */
 static inline void wakelocks_lru_add(struct wakelock *wl) {}
-- 
1.7.9.5

Re: [PATCH v2] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[SungEun Kim(cleaneye.kim@lge.com)]

On 2015-07-03 오후 2:03, SungEun Kim(cleaneye.kim@lge.com) wrote:
> On 2015-07-03 오전 9:15, Rafael J. Wysocki wrote:
>> On Wednesday, July 01, 2015 05:28:48 PM SungEun Kim wrote:
>>> From: "SungEun Kim" <cleaneye.kim@lge.com>
>>>
>>> The synchronous synchronize_rcu in wakeup_source_remove makes user
>> process
>>> which writes to /sys/kernel/wake_unlock blocked sometimes.
>>>
>>> For example, when android eventhub tries to release wakelock,
>>> this blocking process can occur, and eventhub can't get input event
>>> for a while.
>>>
>>> Using workqueue instead of direct function call at pm_wake_unlock
>>> can prevent this unnecessary delay of an user space process.
>>
>> The idea is defendable, but the patch is too simple.
>>
>> For example, if the garbage collection is in progress, it is not useful
>> to start a new one.
>>
>> Also the incrementation and clearing of wakelocks_gc_count should be under
>> the lock.
> 
> Thank you for your advices.
> I will correct and amend my patch and then submit v3 patch.
> 
> Thank you.
> SungEun Kim
> 

Dear Wysocki,

I've submit patch v3.
Could you review patch v3?

Thank you.
SungEun Kim

>>
>>
>> -- 
>> I speak only for myself.
>> Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3] PM / Sleep: Use workqueue for user space wakeup sources garbage collector

[Rafael J. Wysocki]

On Friday, July 03, 2015 03:57:20 PM SungEun Kim wrote:
> From: "cleaeye.kim" <cleaneye.kim@lge.com>
> 
> The synchronous synchronize_rcu in wakeup_source_remove makes user process
> which writes to /sys/kernel/wake_unlock blocked sometimes.
> 
> For example, when android eventhub tries to release wakelock,
> this blocking process can occur, and eventhub can't get input event
> for a while.
> 
> Using workqueue instead of direct function call at pm_wake_unlock
> can prevent this unnecessary delay of an user space process.
> 
> Signed-off-by: SungEun Kim <cleaneye.kim@lge.com>

Queued up for 4.3, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.