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(-)
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(-)
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
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(-)
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
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(-)
>
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 <[email protected]>
> Signed-off-by: Michael Wang <[email protected]>
> ---
> 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
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(-)
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
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 <[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.
--
tejun
On Wed, Jun 05, 2013 at 03:14:04PM +0800, Michael Wang wrote:
> 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(-)
>
> 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
__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(-)
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
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
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(-)
>