[PATCH 0/3] workqueue: code refine/clean for workqueue

[PATCH 0/3] workqueue: code refine/clean for workqueue

[Michael Wang]

Code refine/clean patch set for workqueue.

Michael Wang (3):
	[PATCH 1/3] workqueue: move the internal helper from .h to .c
	[PATCH 2/3] workqueue: remove the unused helper in .h
	[PATCH 3/3] workqueue: add a check point in pwq_activate_delayed_work()

---
 b/include/linux/workqueue.h |    5 -----
 b/kernel/workqueue.c        |    5 +++++
 include/linux/workqueue.h   |   27 ---------------------------
 kernel/workqueue.c          |    6 +++---
 4 files changed, 8 insertions(+), 35 deletions(-)

[PATCH 1/3] workqueue: move the internal helper from .h to .c

[Michael Wang]

alloc_workqueue_attrs(), free_workqueue_attrs() and apply_workqueue_attrs()
are only used internally, move them to .c and make them static.

CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
---
 include/linux/workqueue.h |    5 -----
 kernel/workqueue.c        |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 623488f..de40c8f 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -418,11 +418,6 @@ __alloc_workqueue_key(const char *fmt, unsigned int flags, int max_active,
 
 extern void destroy_workqueue(struct workqueue_struct *wq);
 
-struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask);
-void free_workqueue_attrs(struct workqueue_attrs *attrs);
-int apply_workqueue_attrs(struct workqueue_struct *wq,
-			  const struct workqueue_attrs *attrs);
-
 extern bool queue_work_on(int cpu, struct workqueue_struct *wq,
 			struct work_struct *work);
 extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ee8e29a..7c2fa6b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3137,6 +3137,11 @@ static ssize_t wq_nice_show(struct device *dev, struct device_attribute *attr,
 	return written;
 }
 
+static struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask);
+static void free_workqueue_attrs(struct workqueue_attrs *attrs);
+static int apply_workqueue_attrs(struct workqueue_struct *wq,
+			  const struct workqueue_attrs *attrs);
+
 /* prepare workqueue_attrs for sysfs store operations */
 static struct workqueue_attrs *wq_sysfs_prep_attrs(struct workqueue_struct *wq)
 {
-- 
1.7.4.1

Re: [PATCH 1/3] workqueue: move the internal helper from .h to .c

[Tejun Heo]

On Wed, Jun 05, 2013 at 03:10:43PM +0800, Michael Wang wrote:
> alloc_workqueue_attrs(), free_workqueue_attrs() and apply_workqueue_attrs()
> are only used internally, move them to .c and make them static.
> 
> CC: Tejun Heo <tj@kernel.org>
> Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>

Nack.  That's the published and only interface to use custom attrs
from inside kernel.

Thanks.

-- 
tejun

[PATCH 2/3] workqueue: remove the unused helper in .h

[Michael Wang]

__cancel_delayed_work(), flush_work_sync() and flush_delayed_work_sync()
are no longer used by anyone, just remove them.

CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
---
 include/linux/workqueue.h |   27 ---------------------------
 1 files changed, 0 insertions(+), 27 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 623488f..93a6f5e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -564,33 +564,6 @@ static inline bool keventd_up(void)
 	return system_wq != NULL;
 }
 
-/*
- * Like above, but uses del_timer() instead of del_timer_sync(). This means,
- * if it returns 0 the timer function may be running and the queueing is in
- * progress.
- */
-static inline bool __deprecated __cancel_delayed_work(struct delayed_work *work)
-{
-	bool ret;
-
-	ret = del_timer(&work->timer);
-	if (ret)
-		work_clear_pending(&work->work);
-	return ret;
-}
-
-/* used to be different but now identical to flush_work(), deprecated */
-static inline bool __deprecated flush_work_sync(struct work_struct *work)
-{
-	return flush_work(work);
-}
-
-/* used to be different but now identical to flush_delayed_work(), deprecated */
-static inline bool __deprecated flush_delayed_work_sync(struct delayed_work *dwork)
-{
-	return flush_delayed_work(dwork);
-}
-
 #ifndef CONFIG_SMP
 static inline long work_on_cpu(int cpu, long (*fn)(void *), void *arg)
 {
-- 
1.7.4.1

Re: [PATCH 2/3] workqueue: remove the unused helper in .h

[Tejun Heo]

On Wed, Jun 05, 2013 at 03:11:08PM +0800, Michael Wang wrote:
> __cancel_delayed_work(), flush_work_sync() and flush_delayed_work_sync()
> are no longer used by anyone, just remove them.

These are deprecated interfaces which used to be necessary because
they functioned differently.  Keeping them around is mostly a courtesy
to out-of-tree developers as workqueue tends to be used by a lot of
them too.  The patch description is rather misleading, system_nrt_*wq
should be removed together, and I kinda wanna keep them for one more
cycle, so no cookie for this one either.

Thanks.

-- 
tejun

[PATCH 3/3] workqueue: add a check point in pwq_activate_delayed_work()

[Michael Wang]

get_work_pwq() is possible to return NULL, add a check point for that in
the context inside pwq_activate_delayed_work().

CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
---
 kernel/workqueue.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ee8e29a..ea2ec38 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1072,6 +1072,7 @@ static void put_pwq_unlocked(struct pool_workqueue *pwq)
 static void pwq_activate_delayed_work(struct work_struct *work)
 {
 	struct pool_workqueue *pwq = get_work_pwq(work);
+	BUG_ON(!pwq);
 
 	trace_workqueue_activate_work(work);
 	move_linked_works(work, &pwq->pool->worklist, NULL);
-- 
1.7.4.1

Re: [PATCH 3/3] workqueue: add a check point in pwq_activate_delayed_work()

[Tejun Heo]

On Wed, Jun 05, 2013 at 03:11:35PM +0800, Michael Wang wrote:
> get_work_pwq() is possible to return NULL, add a check point for that in
> the context inside pwq_activate_delayed_work().
> 
> CC: Tejun Heo <tj@kernel.org>
> Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
> ---
>  kernel/workqueue.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index ee8e29a..ea2ec38 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1072,6 +1072,7 @@ static void put_pwq_unlocked(struct pool_workqueue *pwq)
>  static void pwq_activate_delayed_work(struct work_struct *work)
>  {
>  	struct pool_workqueue *pwq = get_work_pwq(work);
> +	BUG_ON(!pwq);

pwq deref right below is gonna crash anyway and it's not like that
crash is gonna difficult to identify.  How is this an improvement?

Thanks.

-- 
tejun

[PATCH 4/3] workqueue: code refine in wqattrs_equal()

[Michael Wang]

code refine to save some line.

CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
---
 kernel/workqueue.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index ee8e29a..5fd4791 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3417,9 +3417,8 @@ static bool wqattrs_equal(const struct workqueue_attrs *a,
 {
 	if (a->nice != b->nice)
 		return false;
-	if (!cpumask_equal(a->cpumask, b->cpumask))
-		return false;
-	return true;
+
+	return cpumask_equal(a->cpumask, b->cpumask);
 }
 
 /**
-- 
1.7.4.1

Re: [PATCH 4/3] workqueue: code refine in wqattrs_equal()

[Tejun Heo]

On Wed, Jun 05, 2013 at 03:14:04PM +0800, Michael Wang wrote:
> code refine to save some line.
> 
> CC: Tejun Heo <tj@kernel.org>
> Signed-off-by: Michael Wang <wangyun@linux.vnet.ibm.com>
> ---
>  kernel/workqueue.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index ee8e29a..5fd4791 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3417,9 +3417,8 @@ static bool wqattrs_equal(const struct workqueue_attrs *a,
>  {
>  	if (a->nice != b->nice)
>  		return false;
> -	if (!cpumask_equal(a->cpumask, b->cpumask))
> -		return false;
> -	return true;
> +
> +	return cpumask_equal(a->cpumask, b->cpumask);

I don't know.  I kinda like the current form because we can add new
attributes easily without modifying existing lines.  The suggested
patch is frivolous.  It doesn't really improve anything.

Thanks.

-- 
tejun

Re: [PATCH 0/3] workqueue: code refine/clean for workqueue

[Michael Wang]

On 06/05/2013 03:10 PM, Michael Wang wrote:
> Code refine/clean patch set for workqueue.
> 
> Michael Wang (3):
> 	[PATCH 1/3] workqueue: move the internal helper from .h to .c
> 	[PATCH 2/3] workqueue: remove the unused helper in .h
> 	[PATCH 3/3] workqueue: add a check point in pwq_activate_delayed_work()
	[PATCH 4/3] workqueue: code refine in wqattrs_equal()

Lost one...

Regards,
Michael Wang

> 
> ---
>  b/include/linux/workqueue.h |    5 -----
>  b/kernel/workqueue.c        |    5 +++++
>  include/linux/workqueue.h   |   27 ---------------------------
>  kernel/workqueue.c          |    6 +++---
>  4 files changed, 8 insertions(+), 35 deletions(-)
> 

Re: [PATCH 0/3] workqueue: code refine/clean for workqueue

[Michael Wang]

On 06/05/2013 03:10 PM, Michael Wang wrote:
> Code refine/clean patch set for workqueue.
> 
> Michael Wang (3):
> 	[PATCH 1/3] workqueue: move the internal helper from .h to .c
> 	[PATCH 2/3] workqueue: remove the unused helper in .h
> 	[PATCH 3/3] workqueue: add a check point in pwq_activate_delayed_work()

Hi, Tejun

Sounds like I failed to grasp the essentials of the way we maintain
workqueue :(

These are just self opinion I got during the review, please ignore them
if they will cause trouble to your maintain work :)

Regards,
Michael Wang

> 
> ---
>  b/include/linux/workqueue.h |    5 -----
>  b/kernel/workqueue.c        |    5 +++++
>  include/linux/workqueue.h   |   27 ---------------------------
>  kernel/workqueue.c          |    6 +++---
>  4 files changed, 8 insertions(+), 35 deletions(-)
>