mm/ksm.c seems to be doing an unneeded _notify.

While reviewing ksm.c, I noticed that ksm.c does:

         if (pte_write(*ptep)) {
                 pte_t entry;

                 swapped = PageSwapCache(page);
                 flush_cache_page(vma, addr, page_to_pfn(page));
                 /*
                  * Ok this is tricky, when get_user_pages_fast() run it doesnt
                  * take any lock, therefore the check that we are going to make
                  * with the pagecount against the mapcount is racey and
                  * O_DIRECT can happen right after the check.
                  * So we clear the pte and flush the tlb before the check
                  * this assure us that no O_DIRECT can happen after the check
                  * or in the middle of the check.
                  */
                 entry = ptep_clear_flush(vma, addr, ptep);
                 /*
                  * Check that no O_DIRECT or similar I/O is in progress on the
                  * page
                  */
                 if (page_mapcount(page) + 1 + swapped != page_count(page)) {
                         set_pte_at_notify(mm, addr, ptep, entry);
                         goto out_unlock;
                 }
                 entry = pte_wrprotect(entry);
                 set_pte_at_notify(mm, addr, ptep, entry);

I would think the error case (where the page has an elevated page_count) should not be using set_pte_at_notify. In that event, you are simply restoring the previous value. Have I missed something or is this an extraneous _notify?

Thanks,
Robin Holt

Quoted 29 lines · Robin Holt

While reviewing ksm.c, I noticed that ksm.c does:

         if (pte_write(*ptep)) {
                 pte_t entry;

                 swapped = PageSwapCache(page);
                 flush_cache_page(vma, addr, page_to_pfn(page));
                 /*
                  * Ok this is tricky, when get_user_pages_fast() run it doesnt
                  * take any lock, therefore the check that we are going to make
                  * with the pagecount against the mapcount is racey and
                  * O_DIRECT can happen right after the check.
                  * So we clear the pte and flush the tlb before the check
                  * this assure us that no O_DIRECT can happen after the check
                  * or in the middle of the check.
                  */
                 entry = ptep_clear_flush(vma, addr, ptep);
                 /*
                  * Check that no O_DIRECT or similar I/O is in progress on the
                  * page
                  */
                 if (page_mapcount(page) + 1 + swapped != page_count(page)) {
                         set_pte_at_notify(mm, addr, ptep, entry);
                         goto out_unlock;
                 }
                 entry = pte_wrprotect(entry);
                 set_pte_at_notify(mm, addr, ptep, entry);

Quoted 5 lines

I would think the error case (where the page has an elevated page_count) should not be using set_pte_at_notify. In that event, you are simply restoring the previous value. Have I missed something or is this an extraneous _notify?

Yes, I think you are right set_pte_at(mm, addr, ptep, entry); would be enough here.

I can`t remember or think any reason why I have used the _notify...

Lets just get ACK from Andrea and Hugh that they agree it isn't needed

Quoted 36 lines · Izik Eidus

Quoted 34 lines · Robin Holt

While reviewing ksm.c, I noticed that ksm.c does:

         if (pte_write(*ptep)) {
                 pte_t entry;

                 swapped = PageSwapCache(page);
                 flush_cache_page(vma, addr, page_to_pfn(page));
                 /*
                  * Ok this is tricky, when get_user_pages_fast() run it doesnt
                  * take any lock, therefore the check that we are going to make
                  * with the pagecount against the mapcount is racey and
                  * O_DIRECT can happen right after the check.
                  * So we clear the pte and flush the tlb before the check
                  * this assure us that no O_DIRECT can happen after the check
                  * or in the middle of the check.
                  */
                 entry = ptep_clear_flush(vma, addr, ptep);
                 /*
                  * Check that no O_DIRECT or similar I/O is in progress on the
                  * page
                  */
                 if (page_mapcount(page) + 1 + swapped != page_count(page)) {
                         set_pte_at_notify(mm, addr, ptep, entry);
                         goto out_unlock;
                 }
                 entry = pte_wrprotect(entry);
                 set_pte_at_notify(mm, addr, ptep, entry);

I would think the error case (where the page has an elevated page_count) should not be using set_pte_at_notify. In that event, you are simply restoring the previous value. Have I missed something or is this an extraneous _notify?

Quoted 6 lines

Yes, I think you are right set_pte_at(mm, addr, ptep, entry); would be enough here.

I can`t remember or think any reason why I have used the _notify...

Lets just get ACK from Andrea and Hugh that they agree it isn't needed

_notify it's needed, we're downgrading permissions here.

Quoted 44 lines · Andrea Arcangeli

Quoted 42 lines · Izik Eidus

Quoted 29 lines · Robin Holt

While reviewing ksm.c, I noticed that ksm.c does:

          if (pte_write(*ptep)) {
                  pte_t entry;

                  swapped = PageSwapCache(page);
                  flush_cache_page(vma, addr, page_to_pfn(page));
                  /*
                   * Ok this is tricky, when get_user_pages_fast() run it doesnt
                   * take any lock, therefore the check that we are going to make
                   * with the pagecount against the mapcount is racey and
                   * O_DIRECT can happen right after the check.
                   * So we clear the pte and flush the tlb before the check
                   * this assure us that no O_DIRECT can happen after the check
                   * or in the middle of the check.
                   */
                  entry = ptep_clear_flush(vma, addr, ptep);
                  /*
                   * Check that no O_DIRECT or similar I/O is in progress on the
                   * page
                   */
                  if (page_mapcount(page) + 1 + swapped != page_count(page)) {
                          set_pte_at_notify(mm, addr, ptep, entry);
                          goto out_unlock;
                  }
                  entry = pte_wrprotect(entry);
                  set_pte_at_notify(mm, addr, ptep, entry);

Quoted 6 lines

I would think the error case (where the page has an elevated page_count) should not be using set_pte_at_notify. In that event, you are simply restoring the previous value. Have I missed something or is this an extraneous _notify?

Yes, I think you are right set_pte_at(mm, addr, ptep, entry); would be enough here.

I can`t remember or think any reason why I have used the _notify...

Quoted 4 lines

Quoted 2 lines

Lets just get ACK from Andrea and Hugh that they agree it isn't needed

_notify it's needed, we're downgrading permissions here.

Robin is not questioning that it's needed in the success case; but in the case where we back out because the counts don't match, and just put back the original entry, he's suggesting that then the _notify isn't needed.

(I'm guessing that Robin is not making a significant improvement to KSM, but rather trying to clarify his understanding of set_pte_at_notify.)

Hugh

ksm.c's write_protect_page implements a lockless means of verifying a page does not have any users of the page which are not accounted for via other kernel tracking means. It does this by removing the writable pte with TLB flushes, checking the page_count against the total known users, and then using set_pte_at_notify to make it a read-only entry.

An unneeded mmu_notifier callout is made in the case where the known users does not match the page_count. In that event, we are inserting the identical pte and there is no need for the set_pte_at_notify, but rather the simpler set_pte_at suffices.

Signed-off-by Robin Holt <[email protected]>

To: Izik Eidus <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Chris Wright <[email protected]>
Cc: [email protected]

---

 mm/ksm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: ksm_remove_notify/mm/ksm.c
===================================================================

Quoted 12 lines · On Thu, 11 Mar 2010 09:54:22 -0600 Robin Holt <[email protected]> wrote:

ksm.c's write_protect_page implements a lockless means of verifying a page does not have any users of the page which are not accounted for via other kernel tracking means. It does this by removing the writable pte with TLB flushes, checking the page_count against the total known users, and then using set_pte_at_notify to make it a read-only entry.

An unneeded mmu_notifier callout is made in the case where the known users does not match the page_count. In that event, we are inserting the identical pte and there is no need for the set_pte_at_notify, but rather the simpler set_pte_at suffices.

Quoted 5 lines

Signed-off-by Robin Holt <[email protected]>

To: Izik Eidus <[email protected]>
Cc: Hugh Dickins <[email protected]>
Cc: Chris Wright <[email protected]>
Cc: [email protected]

Acked-by Izik Eidus <[email protected]>

While reviewing ksm.c, I noticed that ksm.c does:

        if (pte_write(*ptep)) {
                pte_t entry;

                swapped = PageSwapCache(page);
                flush_cache_page(vma, addr, page_to_pfn(page));
                /*
                 * Ok this is tricky, when get_user_pages_fast() run it doesnt
                 * take any lock, therefore the check that we are going to make
                 * with the pagecount against the mapcount is racey and
                 * O_DIRECT can happen right after the check.
                 * So we clear the pte and flush the tlb before the check
                 * this assure us that no O_DIRECT can happen after the check
                 * or in the middle of the check.
                 */
                entry = ptep_clear_flush(vma, addr, ptep);
                /*
                 * Check that no O_DIRECT or similar I/O is in progress on the
                 * page 
                 */
                if (page_mapcount(page) + 1 + swapped != page_count(page)) {
                        set_pte_at_notify(mm, addr, ptep, entry);
                        goto out_unlock;
                }
                entry = pte_wrprotect(entry);
                set_pte_at_notify(mm, addr, ptep, entry);

I would think the error case (where the page has an elevated page_count) should not be using set_pte_at_notify. In that event, you are simply restoring the previous value. Have I missed something or is this an extraneous _notify?