gcc-3.2 breaks rmap on s390x

Hi, Rik:

the following patch seems to fix my rmap problems on s390x.

@@ -376,8 +376,10 @@
 	 */
 #ifdef CONFIG_SMP
 	while (test_and_set_bit(PG_chainlock, &page->flags)) {
-		while (test_bit(PG_chainlock, &page->flags))
+		while (test_bit(PG_chainlock, &page->flags)) {
 			cpu_relax();
+			barrier();
+		}
 	}
 #endif
 }

The failure happens in page_remove_rmap. It runs like so:

        if (!page_mapped(page))    // (page->pte.direct!=0)
                return;         /* remap_page_range() from a driver? */
        pte_chain_lock(page);
        if (PageDirect(page)) {
                if (page->pte.direct == pte_paddr) {

The pte_chain_lock loop is in the patch above. The compiler loads page->pte.direct into a register when page_mapped() is tested, and uses it in if statement across pte_chain_lock().

Andrew Morton observed that bitops on s390(x) do not clobber memory. I saw it too, but I thought it was legal. A bitop changes a word in a specific location. I think it just does not sound right to trash an optimization because of that.

As an example, sparc(32) has a mix of bitops implemented in C and bitops implemented in assembly. C bitops do not clobber memory wholesale. Of asm bitops, only test_and_set_bit clobbers memory (I have to ask Dave why).

Cheers,
-- Pete

cpu_relax() is a barrier on Intel ( pause ). It's just a coincidence IMHO.

- Davide

The actual code is not drastically rearranged and fragments are not replicated. Thus, the code below the while() loop cannot possibly know if the lock was contested or not (if the loop was executed at least once or not), and it has to load the value from memory.

But suppose your worst fears came true and complier decided to push the beyond the limits of reasonable into the shady realm of legal. The worst the compiler can do is:

   register word r10;
   r10 = page->pte.direct;
   if (r10!=0) something();
   if (test_and_set_bit(PG_chainlock, &page->flags) == 0) {
      /* No barrier here, mwuahahaha!!! */
   } else {
      do {
        while (test_bit(PG_chainlock, &page->flags)) {
          // This loop can be inverted too. No change to the reasoning.
          cpu_relax();
          barrier();
        }
      } while (test_and_set_bit(PG_chainlock, &page->flags));
      r10 = page->pte.direct;
   }
   foo_using(r10);

Which continues to work, because the value loaded before uncontested was taken is still valid later. The problem only happens if the lock was contested.

BTW, Bill Irwin noted that, strictly speaking, the first if statement with page_mapped should be done under the lock for safety. I think he got a point. A 2.5 preempt in the wrong moment (between the first load and test_and_set_bit in the above example) makes the code invalid after all. Not that it mattered as long as compiler does not do such exercises as I outlined above.

-- Pete