LKML Archive on
help / color / mirror / Atom feed
From: Martin Schwidefsky <>
To: Hugh Dickins <>
Cc: Peter Zijlstra <>,,
Subject: Re: [patch 5/5] Optimize page_mkclean_one
Date: Sun, 01 Jul 2007 21:50:30 +0200	[thread overview]
Message-ID: <1183319430.15238.18.camel@localhost> (raw)
In-Reply-To: <>

On Sun, 2007-07-01 at 09:54 +0100, Hugh Dickins wrote:
> On Sun, 1 Jul 2007, Martin Schwidefsky wrote:
> > > 
> > > Expect you're right, but I _really_ don't want to comment, when I don't
> > > understand that "|| pte_write" in the first place, and don't know the
> > > consequence of pte_dirty && !pte_write or !pte_dirty && pte_write there.
> > 
> > The pte_write() part is for the shared dirty page tracking. If you want
> > to make sure that a max of x% of your pages are dirty then you cannot
> > allow to have more than x% to be writable. Thats why page_mkclean_one
> > clears the dirty bit and makes the page read-only.
> The whole of page_mkclean_one is for the dirty page tracking: so it's
> obvious why it tests pte_dirty, but not obvious why it tests pte_write.

Yes, the pte_write call is needed for shared dirty page tracking. In
particular its needed for s390 but for corner cases where a page is
writable but not dirty it might be needed for other architectures as

> > > My suspicion is that the "|| pte_write" is precisely to cover your
> > > s390 case where pte is never dirty (it may even have been me who got
> > > Peter to put it in for that reason).  In which case your patch would
> > > be fine - though I think it'd be improved a lot by a comment or
> > > rearrangement or new macro in place of the pte_dirty || pte_write
> > > line (perhaps adjust my pte_maybe_dirty in asm-generic/pgtable.h,
> > > and use that - its former use in msync has gone away now).
> > 
> > No, s390 is covered by the page_test_dirty / page_clear_dirty pair in
> > page_mkclean. 
> That's where its dirty page count comes from, yes: but since the s390
> pte_dirty just says no, if page_mkclean_one tested only pte_dirty,
> then it wouldn't do anything on s390, and in particular wouldn't
> write protect the ptes to re-enforce dirty counting from then on.

Yes, I definitly agree that the pte_write is required for s390.

> So in answering your denials, I grow more confident that the pte_write
> test is precisely for the s390 case.  Though it might also be to cover
> some defect in the write-protection scheme on other arches.

Well, here I'm not so sure. You need the implication
  pte_write() == true -> pte_dirty() == true
to be able to skip the pte_write check for architectures that keep their
dirty bits in the pte. Is this really true for all corner-cases?

> Come to think of it, would your patch really make any difference?
> Although page_mkclean's "count" of dirty ptes on s390 will be nonsense,
> that count would anyway be unknown, and it's only used as a boolean;
> and now I don't think your patch changes the boolean value - if any
> pte is found writable (and if the scheme is working) that implies
> that the page was written to, and so should give the same answer
> as the page_test_dirty.

It depends on code outside of pte_mkclean_one if the patch makes a
difference or not. The additional check for pte_dirty will make the
function less suble as it will not depends on code outside of it.
With the additional check for pte_dirty the function does the following:
1) make the pte clean, 2) make the pte read-only, 3) return true if the
pte has been marked dirty.
Without the check the function does 1), 2) as above and 3) return true
if the pte has been marked dirty or has been writable.
I find it easier to understand the semantics of the function with the
additional check. But that is only me ..

blue skies,

"Reality continues to ruin my life." - Calvin.

  parent reply	other threads:[~2007-07-01 19:48 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-29 13:55 [patch 0/5] Various mm improvements Martin Schwidefsky
2007-06-29 13:55 ` [patch 1/5] avoid tlb gather restarts Martin Schwidefsky
2007-06-29 18:56   ` Hugh Dickins
2007-06-29 21:19     ` Martin Schwidefsky
2007-06-30 13:16       ` Hugh Dickins
2007-06-29 13:55 ` [patch 2/5] remove ptep_establish Martin Schwidefsky
2007-06-29 13:55 ` [patch 3/5] remove ptep_test_and_clear_dirty and ptep_clear_flush_dirty Martin Schwidefsky
2007-07-03  1:29   ` Zachary Amsden
2007-07-03  7:26     ` Martin Schwidefsky
2007-06-29 13:55 ` [patch 4/5] move mm_struct and vm_area_struct Martin Schwidefsky
2007-06-29 13:55 ` [patch 5/5] Optimize page_mkclean_one Martin Schwidefsky
2007-06-30 14:04   ` Hugh Dickins
2007-07-01  7:15     ` Martin Schwidefsky
2007-07-01  8:54       ` Hugh Dickins
2007-07-01 13:27         ` Peter Zijlstra
2007-07-02  7:07           ` Martin Schwidefsky
2007-07-01 19:50         ` Martin Schwidefsky [this message]
2007-07-01 10:29   ` Miklos Szeredi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1183319430.15238.18.camel@localhost \ \ \ \ \ \
    --subject='Re: [patch 5/5] Optimize page_mkclean_one' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).