RE: [PATCH 2.5.69.bk9] Fix reference counts for pages (take 1) [w as RE: condvar wakeups]

> From: Saurabh Desai [mailto:[email protected]]
> 
> "Perez-Gonzalez, Inaky" wrote:
> > This is the first take at it; I still didn't have time to test it
> > (0.37 building, slow) ... but I don't think I screwed up anything
> > - Saurabh, would you mind torturing it a little bit in your x4 to
> > make sure it is not leaking memory anywhere? Performance wise it
> > should do nothing to the system ...
> >
> 
> This patch cause about 30% slowdown with or without requeue NPTL.
> And I found out why, you didn't return from __pin_page() as before
> (for the fastpath). It should be like (in __pin_page()),
> +       if (likely(page != NULL))
> +               return __pin_page_atomic(page);
> 
> With this, it works as before. I think its costing little bit for
> unpin/pin in the requeue case, which is required anyway.

Sometimes I really think I should be shepherding goats instead of
cranking out code ... Thanks so much, Saurabh. Ok, so here it goes,
take 3, with that fixed.

Index: linux3/linux/kernel/futex.c
diff -u linux3/linux/kernel/futex.c:1.1.1.1.2.4
linux3/linux/kernel/futex.c:1.1.1.1.2.2.2.5
--- linux3/linux/kernel/futex.c:1.1.1.1.2.4	Wed May 14 16:30:07 2003
+++ linux3/linux/kernel/futex.c	Thu May 15 14:32:56 2003
@@ -98,6 +98,13 @@
  *
  * Must be called with (and returns with) all futex-MM locks held.
  */
+static inline
+struct page *__pin_page_atomic (struct page *page)
+{
+	if (!PageReserved(page))
+		get_page(page);
+	return page;
+}
 static struct page *__pin_page(unsigned long addr)
 {
 	struct mm_struct *mm = current->mm;
@@ -108,11 +115,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))
+		return __pin_page_atomic(page);
 
 	/*
 	 * No luck - need to fault in the page:
@@ -202,7 +206,9 @@
 	spin_lock(&futex_lock);
 
 	if (!list_empty(&q->list)) {
+		unpin_page(q->page);
 		q->page = new_page;
+		__pin_page_atomic(new_page);
 		list_del(&q->list);
 		list_add_tail(&q->list, head);
 	}
@@ -247,6 +253,8 @@
 				if (this->filp)
 					send_sigio(&this->filp->f_owner,
this->fd, POLL_IN);
 			} else {
+				unpin_page(this->page);
+				__pin_page_atomic (page2);
 				list_add_tail(i, head2);
 				__attach_vcache(&this->vcache, uaddr2,
current->mm, futex_vcache_callback);
 				this->offset = offset2;
@@ -362,7 +370,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)