On Thu, 2011-01-20 at 11:57 -0800, Hugh Dickins wrote:
> > > 21/21 mm-optimize_page_lock_anon_vma_fast-path.patch
> > > I certainly see the call for this patch, I want to eliminate those
> > > doubled atomics too. This appears correct to me, and I've not dreamt
> > > up an alternative; but I do dislike it, and I suspect you don't like
> > > it much either. I'm ambivalent about it, would love a better patch.
> >
> > Like said, I fully agree with that sentiment, just haven't been able to
> > come up with anything saner :/ Although I can optimize the
> > __put_anon_vma() path a bit by doing something like:
> >
> > if (mutex_is_locked()) { anon_vma_lock(); anon_vma_unlock(); }
> >
> > But I bet that wants a barrier someplace and my head hurts..
>
> Without daring to hurt my head very much, yes, I'd say those kind
> of "optimizations" have a habit of turning out to be racily wrong.
>
> But you put your finger on it: if you hadn't had to add that lock-
> unlock pair into __put_anon_vma(), I wouldn't have minded the
> contortions added to page_lock_anon_vma().
I think there's just about enough implied barriers there that the
'simple' code just works ;-)
But given that I'm trying to think with snot for brains thanks to some
cold, I don't trust myself at all to have gotten this right.
[ for Oleg and Paul: https://lkml.org/lkml/2010/11/26/213 contains the
full patch this is against ]
---
Index: linux-2.6/mm/rmap.c
===================================================================
--- linux-2.6.orig/mm/rmap.c
+++ linux-2.6/mm/rmap.c
@@ -1559,9 +1559,20 @@ void __put_anon_vma(struct anon_vma *ano
* Synchronize against page_lock_anon_vma() such that
* we can safely hold the lock without the anon_vma getting
* freed.
+ *
+ * Relies on the full mb implied by the atomic_dec_and_test() from
+ * put_anon_vma() against the full mb implied by mutex_trylock() from
+ * page_lock_anon_vma(). This orders:
+ *
+ * page_lock_anon_vma() VS put_anon_vma()
+ * mutex_trylock() atomic_dec_and_test()
+ * smp_mb() smp_mb()
+ * atomic_read() mutex_is_locked()
*/
- anon_vma_lock(anon_vma);
- anon_vma_unlock(anon_vma);
+ if (mutex_is_locked(&anon_vma->root->mutex)) {
+ anon_vma_lock(anon_vma);
+ anon_vma_unlock(anon_vma);
+ }
if (anon_vma->root != anon_vma)
put_anon_vma(anon_vma->root);
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
