Skip to contentSkip to conversation

workqueue.c: should schedule work serialize work->func

RFC PATCH In review John Fastabend 2 participants 2 messages Jan 6, 2010 – Jan 7, 2010

Subsystems (3)
Networking Network drivers Workqueue
lore
Share & tools
lore

John Fastabend

author
#0 lore raw

RFC PATCH 1/1 workqueue.c: should schedule work serialize work->func

codepath
schedule_work(); ... ; run_workqueue() -> work_clear_pending() -> f(work)

Although schedule_work() will only schedule the task if it is not already on the work queue the WORK_STRUCT_PENDING bits are cleared just before calling the work function. If work is scheduled after this occurs and before f(work) completes it is possible to have multiple calls into f().

Should the kernel protect us from this? With something like the patch below.

Thanks
---

 include/linux/workqueue.h |    3 ++-
 kernel/workqueue.c        |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)
include/linux/workqueue.h +2 -1
index 9466e86..fa6ffea 100644
@@ -26,7 +26,8 @@ struct work_struct {
 	atomic_long_t data;
 #define WORK_STRUCT_PENDING 0		/* T if work item pending execution */
 #define WORK_STRUCT_STATIC  1		/* static initializer (debugobjects) */
-#define WORK_STRUCT_FLAG_MASK (3UL)
+#define WORK_STRUCT_RUNNING 2
+#define WORK_STRUCT_FLAG_MASK (7UL)	/* 0111 */
 #define WORK_STRUCT_WQ_DATA_MASK (~WORK_STRUCT_FLAG_MASK)
 	struct list_head entry;
 	work_func_t func;
kernel/workqueue.c +3 -0
index dee4865..b867125 100644
@@ -397,10 +397,13 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
 		spin_unlock_irq(&cwq->lock);
 
 		BUG_ON(get_wq_data(work) != cwq);
+		while (test_and_set_bit(WORK_STRUCT_RUNNING, work_data_bits(work)))
+			cpu_relax();
 		work_clear_pending(work);
 		lock_map_acquire(&cwq->wq->lockdep_map);
 		lock_map_acquire(&lockdep_map);
 		f(work);
+		clear_bit(WORK_STRUCT_RUNNING, work_data_bits(work));
 		lock_map_release(&lockdep_map);
 		lock_map_release(&cwq->wq->lockdep_map);
maintainer
#1 lore raw

Hello,

Quoted 10 lines · John Fastabend

codepath
schedule_work(); ... ; run_workqueue() -> work_clear_pending() -> f(work)

Although schedule_work() will only schedule the task if it is not already on the work queue the WORK_STRUCT_PENDING bits are cleared just before calling the work function. If work is scheduled after this occurs and before f(work) completes it is possible to have multiple calls into f().

Should the kernel protect us from this? With something like the patch below.

This is by design. Multithread workqueue doesn't protect against works running simultaneously on different cpus. The callback is responsible for synchronization.

Quoted 19 lines
include/linux/workqueue.h +2 -1
index 9466e86..fa6ffea 100644
@@ -26,7 +26,8 @@ struct work_struct {
 	atomic_long_t data;
 #define WORK_STRUCT_PENDING 0		/* T if work item pending execution */
 #define WORK_STRUCT_STATIC  1		/* static initializer (debugobjects) */
-#define WORK_STRUCT_FLAG_MASK (3UL)
+#define WORK_STRUCT_RUNNING 2
+#define WORK_STRUCT_FLAG_MASK (7UL)	/* 0111 */
 #define WORK_STRUCT_WQ_DATA_MASK (~WORK_STRUCT_FLAG_MASK)
 	struct list_head entry;
 	work_func_t func;
kernel/workqueue.c +0 -0
index dee4865..b867125 100644

kernel/workqueue.c static void run_workqueue(struct cpu_workqueue_struct *cwq) · line 406

Quoted 10 lines
kernel/workqueue.c +3 -0
@@ -397,10 +397,13 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
 		spin_unlock_irq(&cwq->lock);
 
 		BUG_ON(get_wq_data(work) != cwq);
+		while (test_and_set_bit(WORK_STRUCT_RUNNING, work_data_bits(work)))
+			cpu_relax();
 		work_clear_pending(work);
 		lock_map_acquire(&cwq->wq->lockdep_map);
 		lock_map_acquire(&lockdep_map);
 		f(work);
+		clear_bit(WORK_STRUCT_RUNNING, work_data_bits(work));

work is not accessible at this point. f() may have freed it already.

Thanks.

Signature · 1 line
tejun