linux-kernel - [PATCH 3/3] WorkStruct: Use direct assignment rather than cmpxchg()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20061207153143.28408.7274.stgit@warthog.cambridge.redhat.com>
Date:	Thu, 07 Dec 2006 15:31:43 +0000
From:	David Howells <dhowells@...hat.com>
To:	torvalds@...l.org, akpm@...l.org, davem@...emloft.com,
	wli@...omorphy.com, matthew@....cx
Cc:	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	dhowells@...hat.com
Subject: [PATCH 3/3] WorkStruct: Use direct assignment rather than cmpxchg()

Use direct assignment rather than cmpxchg() as the latter is unavailable and
unimplementable on some platforms and is actually unnecessary.

The use of cmpxchg() was to guard against two possibilities, neither of which
can actually occur:

 (1) The pending flag may have been unset or may be cleared.  However, given
     where it's called, the pending flag is _always_ set.  I don't think it
     can be unset whilst we're in set_wq_data().

     Once the work is enqueued to be actually run, the only way off the queue
     is for it to be actually run.

     If it's a delayed work item, then the bit can't be cleared by the timer
     because we haven't started the timer yet.  Also, the pending bit can't be
     cleared by cancelling the delayed work _until_ the work item has had its
     timer started.

 (2) The workqueue pointer might change.  This can only happen in two cases:

     (a) The work item has just been queued to actually run, and so we're
         protected by the appropriate workqueue spinlock.

     (b) A delayed work item is being queued, and so the timer hasn't been
     	 started yet, and so no one else knows about the work item or can
     	 access it (the pending bit protects us).

     Besides, set_wq_data() _sets_ the workqueue pointer unconditionally, so
     it can be assigned instead.

So, replacing the set_wq_data() with a straight assignment would be okay in
most cases.  The problem is where we end up tangling with test_and_set_bit()
emulated using spinlocks, and even then it's not a problem _provided_
test_and_set_bit() doesn't attempt to modify the word if the bit was set.

If that's a problem, then a bitops-proofed assignment will be required -
equivalent to atomic_set() vs other atomic_xxx() ops.

Signed-Off-By: David Howells <dhowells@...hat.com>
---

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

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8d1e7cb..f5f3819 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -80,22 +80,19 @@ static inline int is_single_threaded(str
 	return list_empty(&wq->list);
 }
 
+/*
+ * Set the workqueue on which a work item is to be run
+ * - Must *only* be called if the pending flag is set
+ */
 static inline void set_wq_data(struct work_struct *work, void *wq)
 {
-	unsigned long new, old, res;
+	unsigned long new;
+
+	BUG_ON(!work_pending(work));
 
-	/* assume the pending flag is already set and that the task has already
-	 * been queued on this workqueue */
 	new = (unsigned long) wq | (1UL << WORK_STRUCT_PENDING);
-	res = work->management;
-	if (res != new) {
-		do {
-			old = res;
-			new = (unsigned long) wq;
-			new |= (old & WORK_STRUCT_FLAG_MASK);
-			res = cmpxchg(&work->management, old, new);
-		} while (res != old);
-	}
+	new |= work->management & WORK_STRUCT_FLAG_MASK;
+	assign_bits(new, &work->management);
 }
 
 static inline void *get_wq_data(struct work_struct *work)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/