workqueue: code refine/clean for workqueue

PATCH In review Michael Wang 2 participants 11 messages Jun 5, 2013 Workqueue

lore

Share & tools

lore

Michael Wang

author

2013-06-05 07:10 UTC #0 lore raw

cover workqueue: code refine/clean for workqueue

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(-)

Michael Wang

author

2013-06-05 07:10 UTC #1 lore raw

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

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 <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 include/linux/workqueue.h |    5 -----
 kernel/workqueue.c        |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

include/linux/workqueue.h +0 -5

index 623488f..de40c8f 100644
@@ -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,

kernel/workqueue.c +5 -0

index ee8e29a..7c2fa6b 100644
@@ -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)
 {

Signature · 1 line

1.7.4.1

Tejun Heo

maintainer

2013-06-05 07:19 UTC #2 lore raw

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

Quoted 5 lines · 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 <[email protected]>
Signed-off-by Michael Wang <[email protected]>

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

Thanks.

Signature · 1 line

tejun

Michael Wang

author

2013-06-05 07:23 UTC #3 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 include/linux/workqueue.h |   27 ---------------------------
 1 files changed, 0 insertions(+), 27 deletions(-)

include/linux/workqueue.h +0 ~~-27~~

index 623488f..93a6f5e 100644
@@ -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)
 {

Signature · 1 line

1.7.4.1

Tejun Heo

maintainer

2013-06-05 07:24 UTC #4 lore raw

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

Quoted 2 lines · Michael Wang

__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.

Signature · 1 line

tejun

Michael Wang

author

2013-06-05 07:16 UTC #5 lore raw

In reply to Michael Wang

Quoted 6 lines · 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()

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

Lost one...

Regards,
Michael Wang

Quoted 8 lines

---
 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(-)

Michael Wang

author

2013-06-05 07:18 UTC #6 lore raw

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

In reply to Michael Wang

code refine to save some line.

CC Tejun Heo <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 kernel/workqueue.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

kernel/workqueue.c +2 -3

index ee8e29a..5fd4791 100644
@@ -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);
 }
 
 /**

Signature · 1 line

1.7.4.1

Tejun Heo

maintainer

2013-06-05 07:21 UTC #7 lore raw

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

Quoted 13 lines · Michael Wang

code refine to save some line.
CC Tejun Heo <[email protected]>
Signed-off-by Michael Wang <[email protected]>
---
 kernel/workqueue.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)
kernel/workqueue.c +0 -0
index ee8e29a..5fd4791 100644

kernel/workqueue.c static bool wqattrs_equal(const struct workqueue_attrs *a, · line 3421

Quoted 8 lines

kernel/workqueue.c +2 -3

@@ -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.

Signature · 1 line

tejun

Michael Wang

author

2013-06-05 07:11 UTC #8 lore raw

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

In reply to 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 <[email protected]>

Signed-off-by Michael Wang <[email protected]>

---
 kernel/workqueue.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

kernel/workqueue.c +1 -0

index ee8e29a..ea2ec38 100644
@@ -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);

Signature · 1 line

1.7.4.1

Tejun Heo

maintainer

2013-06-05 07:17 UTC #9 lore raw

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

Quoted 14 lines · 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 <[email protected]>
Signed-off-by Michael Wang <[email protected]>
---
 kernel/workqueue.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
kernel/workqueue.c +0 -0
index ee8e29a..ea2ec38 100644

kernel/workqueue.c static void put_pwq_unlocked(struct pool_workqueue *pwq) · line 1075

Quoted 4 lines

kernel/workqueue.c +1 -0

@@ -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.

Signature · 1 line

tejun

Michael Wang

author

2013-06-05 07:39 UTC #10 lore raw

In reply to Michael Wang

Quoted 6 lines · 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()

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

Quoted 8 lines

---
 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(-)