RE: condvar wakeups

> From: Ingo Molnar [mailto:[email protected]]
> until someone can show some real degradation due to wakeup behavior i'll
> close this issue and wont pursue FUTEX_REQUEUE upstream, it seems to bring
> no benefits.

Hi Ingo - not related, except for a similar problem I found
when playing with it: Isn't vcache_callback screwing the page 
reference counts? 

My point is we update the page (as in the vcache callback code) but 
we ain't correctly accounting for it.

The futex_wait() call sets q->page and has a refcount on that q->page.
Now if we hit the callback, q->page is set to 'newpage', but we don't
hold a reference to it, so it might be paged out and then we are kind
of screwed. However, futex_wait() exit path will correctly remove the
reference to 'oldpage'.

Shouldn't this change to have vcache callback unpin() q->page, set the
new page, pin it (well, page_get()) and then, futex_wait() and futex_close()
both unpin (on exit) q->page? -third and fourth patch chunks-- 

--- futex.c	1 May 2003 20:11:20 -0000	1.1.1.1
+++ futex.c	5 May 2003 20:48:37 -0000
@@ -106,6 +106,13 @@
  *
  * Must be called with (and returns with) all futex-MM locks held.
  */
+static inline
+struct page *__pin_page_atomic (unsigned long addr)
+{
+	if (!PageReserved(page))
+		get_page(page);
+	return page;
+}
 static struct page *__pin_page(unsigned long addr)
 {
 	struct mm_struct *mm = current->mm;
@@ -116,11 +123,8 @@
 	 * Do a quick atomic lookup first - this is the fastpath.
 	 */
 	page = follow_page(mm, addr, 0);
-	if (likely(page != NULL)) {	
-		if (!PageReserved(page))
-			get_page(page);
-		return page;
-	}
+	if (likely(page != NULL))
+		__pin_page_atomic (page);
 
 	/*
 	 * No luck - need to fault in the page:
@@ -208,7 +212,9 @@
 	spin_lock(&futex_lock);
 
 	if (!list_empty(&q->list)) {
+		unpin_page (q->page);
 		q->page = new_page;
+		__pin_page_atomic (q->page);
 		list_del(&q->list);
 		list_add_tail(&q->list, head);
 	}
@@ -310,7 +316,7 @@
 	/* Were we woken up anyway? */
 	if (!unqueue_me(&q))
 		ret = 0;
-	unpin_page(page);
+	unpin_page(q->page);
 
 	return ret;
 }

Iñaky Pérez-González -- Not speaking for Intel -- all opinions are my own
(and my fault)