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

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

[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?

Thanks,
Robin Holt

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Chris Wright]

* Robin Holt (holt@sgi.com) wrote:
> 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?

It does look extraneous to me as well.

thanks,
-chris

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Izik Eidus]

On 03/10/2010 09:18 PM, Robin Holt wrote:
> 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?
>    

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

Thanks.

> Thanks,
> Robin Holt
>    

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Andrea Arcangeli]

On Wed, Mar 10, 2010 at 10:19:33PM +0200, Izik Eidus wrote:
> On 03/10/2010 09:18 PM, Robin Holt wrote:
> > 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?
> >    
> 
> 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hugh Dickins]

On Wed, 10 Mar 2010, Andrea Arcangeli wrote:
> On Wed, Mar 10, 2010 at 10:19:33PM +0200, Izik Eidus wrote:
> > On 03/10/2010 09:18 PM, Robin Holt wrote:
> > > 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?
> > >    
> > 
> > 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.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Izik Eidus]

On 03/11/2010 08:23 AM, Hugh Dickins wrote:
> On Wed, 10 Mar 2010, Andrea Arcangeli wrote:
>    
>> On Wed, Mar 10, 2010 at 10:19:33PM +0200, Izik Eidus wrote:
>>      
>>> On 03/10/2010 09:18 PM, Robin Holt wrote:
>>>        
>>>> 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?
>>>>
>>>>          
>>> 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.
>>      
> 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.
>    

Yes exactly, and at that 'counts don`t match' path -
there is no need to call to _notify.

> (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.)
>    


Yea, it won`t run unless at very rare cases

> Hugh
>    

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[Patch] mm/ksm.c is doing an unneeded _notify in write_protect_page.

[Robin Holt]

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 <holt@sgi.com>
To: Izik Eidus <ieidus@redhat.com>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Chris Wright <chrisw@redhat.com>
Cc: linux-mm@kvack.org

---

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

Index: ksm_remove_notify/mm/ksm.c
===================================================================
--- ksm_remove_notify.orig/mm/ksm.c	2010-03-11 09:24:30.000000000 -0600
+++ ksm_remove_notify/mm/ksm.c	2010-03-11 09:35:18.000000000 -0600
@@ -751,7 +751,7 @@ static int write_protect_page(struct vm_
 		 * page
 		 */
 		if (page_mapcount(page) + 1 + swapped != page_count(page)) {
-			set_pte_at_notify(mm, addr, ptep, entry);
+			set_pte_at(mm, addr, ptep, entry);
 			goto out_unlock;
 		}
 		entry = pte_wrprotect(entry);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Patch] mm/ksm.c is doing an unneeded _notify in write_protect_page.

[Izik Eidus]

On Thu, 11 Mar 2010 09:54:22 -0600
Robin Holt <holt@sgi.com> 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.
> 
> Signed-off-by: Robin Holt <holt@sgi.com>
> To: Izik Eidus <ieidus@redhat.com>
> Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Cc: Chris Wright <chrisw@redhat.com>
> Cc: linux-mm@kvack.org

Acked-by: Izik Eidus <ieidus@redhat.com>

> 
> ---
> 
>  mm/ksm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: ksm_remove_notify/mm/ksm.c
> ===================================================================
> --- ksm_remove_notify.orig/mm/ksm.c	2010-03-11 09:24:30.000000000 -0600
> +++ ksm_remove_notify/mm/ksm.c	2010-03-11 09:35:18.000000000 -0600
> @@ -751,7 +751,7 @@ static int write_protect_page(struct vm_
>  		 * page
>  		 */
>  		if (page_mapcount(page) + 1 + swapped != page_count(page)) {
> -			set_pte_at_notify(mm, addr, ptep, entry);
> +			set_pte_at(mm, addr, ptep, entry);
>  			goto out_unlock;
>  		}
>  		entry = pte_wrprotect(entry);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [Patch] mm/ksm.c is doing an unneeded _notify in write_protect_page.

[Andrea Arcangeli]

On Thu, Mar 11, 2010 at 06:01:59PM +0200, Izik Eidus wrote:
> On Thu, 11 Mar 2010 09:54:22 -0600
> Robin Holt <holt@sgi.com> 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.
> > 
> > Signed-off-by: Robin Holt <holt@sgi.com>
> > To: Izik Eidus <ieidus@redhat.com>
> > Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> > Cc: Chris Wright <chrisw@redhat.com>
> > Cc: linux-mm@kvack.org
> 
> Acked-by: Izik Eidus <ieidus@redhat.com>

Ack too. I misunderstood what you were talking about before, patch
makes it clear now ;).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>