[PATCH 0/4] Scrubbing updates

[PATCH 0/4] Scrubbing updates

[Boris Ostrovsky]

First patch fixes a long-standing bug where a low memory monitor is
not initializes if boottime scrubbing is turned off.

The other threee patches are performace optimizations.

Boris Ostrovsky (4):
  mm: Initialize lowmem virq when boot-time scrubbing is disabled
  mm: Don't poison a page if boot-time scrubbing is off
  mm: Don't request scrubbing until dom0 is running
  mm: Don't hold heap lock in alloc_heap_pages() longer than necessary

 xen/common/page_alloc.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/4] mm: Initialize lowmem virq when boot-time scrubbing is disabled

[Boris Ostrovsky]

scrub_heap_pages() does early return if boot-time scrubbing is
disabled, neglecting to initialize lowmem VIRQ.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9fa62d2..7d56e92 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1849,6 +1849,14 @@ void __init scrub_heap_pages(void)
     int last_distance, best_node;
     int cpus;
 
+    /*
+     * Set bounds for the low mem virq algorithm.
+     * Not the most logical place for this but it needs to be done
+     * after dom0 has been constructed and this seems to be a
+     * convenient location.
+     */
+    setup_low_mem_virq();
+
     if ( !opt_bootscrub )
         return;
 
@@ -1970,10 +1978,6 @@ void __init scrub_heap_pages(void)
 #ifdef CONFIG_SCRUB_DEBUG
     boot_scrub_done = true;
 #endif
-
-    /* Now that the heap is initialized, run checks and set bounds
-     * for the low mem virq algorithm. */
-    setup_low_mem_virq();
 }
 
 
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] mm: Initialize lowmem virq when boot-time scrubbing is disabled

[Wei Liu]

On Mon, Aug 28, 2017 at 04:40:26PM -0400, Boris Ostrovsky wrote:
> scrub_heap_pages() does early return if boot-time scrubbing is
> disabled, neglecting to initialize lowmem VIRQ.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Seems more appropriate to lift the call to setup_low_mem_virq to the
caller of scrub_heap_pages.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] mm: Initialize lowmem virq when boot-time scrubbing is disabled

[Boris Ostrovsky]

On 08/29/2017 07:51 AM, Wei Liu wrote:
> On Mon, Aug 28, 2017 at 04:40:26PM -0400, Boris Ostrovsky wrote:
>> scrub_heap_pages() does early return if boot-time scrubbing is
>> disabled, neglecting to initialize lowmem VIRQ.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Seems more appropriate to lift the call to setup_low_mem_virq to the
> caller of scrub_heap_pages.

setup_low_mem_virq() is only useful in page_alloc.c so I'd rather keep
it static there.

What I could do is create something like

void heap_init_late(void)
{
    setup_low_mem_virq();

    if ( opt_bootscrub )
        scrub_heap_pages();
}


-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] mm: Initialize lowmem virq when boot-time scrubbing is disabled

[Jan Beulich]

>>> On 29.08.17 at 15:12, <boris.ostrovsky@oracle.com> wrote:
> On 08/29/2017 07:51 AM, Wei Liu wrote:
>> On Mon, Aug 28, 2017 at 04:40:26PM -0400, Boris Ostrovsky wrote:
>>> scrub_heap_pages() does early return if boot-time scrubbing is
>>> disabled, neglecting to initialize lowmem VIRQ.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Seems more appropriate to lift the call to setup_low_mem_virq to the
>> caller of scrub_heap_pages.
> 
> setup_low_mem_virq() is only useful in page_alloc.c so I'd rather keep
> it static there.
> 
> What I could do is create something like
> 
> void heap_init_late(void)
> {
>     setup_low_mem_virq();
> 
>     if ( opt_bootscrub )
>         scrub_heap_pages();
> }

Ah, yes, that's what I've outlined in the other reply just sent.
Don't forget the __init, though.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] mm: Initialize lowmem virq when boot-time scrubbing is disabled

[Jan Beulich]

>>> On 29.08.17 at 13:51, <wei.liu2@citrix.com> wrote:
> On Mon, Aug 28, 2017 at 04:40:26PM -0400, Boris Ostrovsky wrote:
>> scrub_heap_pages() does early return if boot-time scrubbing is
>> disabled, neglecting to initialize lowmem VIRQ.
>> 
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Seems more appropriate to lift the call to setup_low_mem_virq to the
> caller of scrub_heap_pages.

That's not a good idea imo, as that would mean replicating the
code in ARM and x86. I'd rather see the early return to become
an ordinary if (skipping the rest of the function, preferably
without goto). Or, mainly to limit code churn, introduce a new
wrapper function (of perhaps a different name, as
setup_low_mem_virq() has nothing to do with scrubbing
anyway) and move the setup_low_mem_virq() call
there, making scrub_heap_pages() static.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/4] mm: Don't poison a page if boot-time scrubbing is off

[Boris Ostrovsky]

If boot-time scrubbing is turned off we don't check pages in
check_one_page(). Thus there is no reason to ever poison them.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7d56e92..34a7992 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -712,6 +712,9 @@ static void poison_one_page(struct page_info *pg)
     mfn_t mfn = _mfn(page_to_mfn(pg));
     uint64_t *ptr;
 
+    if ( !boot_scrub_done )
+        return;
+
     ptr = map_domain_page(mfn);
     *ptr = ~SCRUB_PATTERN;
     unmap_domain_page(ptr);
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] mm: Don't poison a page if boot-time scrubbing is off

[Wei Liu]

On Mon, Aug 28, 2017 at 04:40:27PM -0400, Boris Ostrovsky wrote:
> If boot-time scrubbing is turned off we don't check pages in
> check_one_page(). Thus there is no reason to ever poison them.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Boris Ostrovsky]

There is no need to scrub pages freed during dom0 construction
since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()).

Since boot_scrub_done will not be set if boot-time scrubbing is off we
also check for domain state.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 34a7992..b93dae9 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
              */
             scrub = !!d->is_dying;
 #else
-            scrub = true;
+            scrub = boot_scrub_done || !!d->is_dying;
 #endif
         }
         else
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Wei Liu]

On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote:
> There is no need to scrub pages freed during dom0 construction
> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()).
> 
> Since boot_scrub_done will not be set if boot-time scrubbing is off we
> also check for domain state.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/common/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 34a7992..b93dae9 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>               */
>              scrub = !!d->is_dying;
>  #else
> -            scrub = true;
> +            scrub = boot_scrub_done || !!d->is_dying;

It seems that the debug and non-debug case should use the same
predicate.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Jan Beulich]

>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote:
> On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote:
>> There is no need to scrub pages freed during dom0 construction
>> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()).
>> 
>> Since boot_scrub_done will not be set if boot-time scrubbing is off we
>> also check for domain state.
>> 
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  xen/common/page_alloc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>> index 34a7992..b93dae9 100644
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>               */
>>              scrub = !!d->is_dying;
>>  #else
>> -            scrub = true;
>> +            scrub = boot_scrub_done || !!d->is_dying;
> 
> It seems that the debug and non-debug case should use the same
> predicate.

No - boot_scrub_done doesn't even exist in the other case.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Boris Ostrovsky]

On 08/29/2017 09:22 AM, Jan Beulich wrote:
>>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote:
>> On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote:
>>> There is no need to scrub pages freed during dom0 construction
>>> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()).
>>>
>>> Since boot_scrub_done will not be set if boot-time scrubbing is off we
>>> also check for domain state.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> ---
>>>  xen/common/page_alloc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>> index 34a7992..b93dae9 100644
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>>>               */
>>>              scrub = !!d->is_dying;
>>>  #else
>>> -            scrub = true;
>>> +            scrub = boot_scrub_done || !!d->is_dying;
>> It seems that the debug and non-debug case should use the same
>> predicate.
> No - boot_scrub_done doesn't even exist in the other case.


I read Wei's message as suggesting

        scrub = !!d->is_dying;
    #ifdef CONFIG_SCRUB_DEBUG
        scrub |= boot_scrub_done;
    #endif


which I could do.


-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Jan Beulich]

>>> On 29.08.17 at 15:27, <boris.ostrovsky@oracle.com> wrote:
> On 08/29/2017 09:22 AM, Jan Beulich wrote:
>>>>> On 29.08.17 at 13:56, <wei.liu2@citrix.com> wrote:
>>> On Mon, Aug 28, 2017 at 04:40:28PM -0400, Boris Ostrovsky wrote:
>>>> There is no need to scrub pages freed during dom0 construction
>>>> since heap will be scrubbed once dom0 is ready (by scrub_heap_pages()).
>>>>
>>>> Since boot_scrub_done will not be set if boot-time scrubbing is off we
>>>> also check for domain state.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> ---
>>>>  xen/common/page_alloc.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>>> index 34a7992..b93dae9 100644
>>>> --- a/xen/common/page_alloc.c
>>>> +++ b/xen/common/page_alloc.c
>>>> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned 
> int order)
>>>>               */
>>>>              scrub = !!d->is_dying;
>>>>  #else
>>>> -            scrub = true;
>>>> +            scrub = boot_scrub_done || !!d->is_dying;
>>> It seems that the debug and non-debug case should use the same
>>> predicate.
>> No - boot_scrub_done doesn't even exist in the other case.
> 
> 
> I read Wei's message as suggesting
> 
>         scrub = !!d->is_dying;
>     #ifdef CONFIG_SCRUB_DEBUG
>         scrub |= boot_scrub_done;
>     #endif
> 
> 
> which I could do.

Yes, but please consider the "#define boot_scrub_done 0" approach
too.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Boris Ostrovsky]

> Yes, but please consider the "#define boot_scrub_done 0" approach
> too.

Yes, I'll do that but I will rename boot_scrub_done to scrub_debug since
having boot_scrub_done=0 in non-debug case will not convey what it
actually is supposed to mean.

-boris


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Jan Beulich]

>>> On 29.08.17 at 16:20, <boris.ostrovsky@oracle.com> wrote:

>> Yes, but please consider the "#define boot_scrub_done 0" approach
>> too.
> 
> Yes, I'll do that but I will rename boot_scrub_done to scrub_debug since
> having boot_scrub_done=0 in non-debug case will not convey what it
> actually is supposed to mean.

Yeah, I too did realize that, but didn't have a good idea for a
replacement name. Yours sounds good.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Jan Beulich]

>>> On 28.08.17 at 22:40, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>               */
>              scrub = !!d->is_dying;
>  #else
> -            scrub = true;
> +            scrub = boot_scrub_done || !!d->is_dying;
>  #endif

Actually Wei has a point here - already when reviewing the original
series I had wondered whether a "#define boot_scrub_done false"
in the non-debug case wouldn't help eliminate some #ifdef-ary.
Here it would mean fulfilling Wei's request indirectly, by simply
removing the preprocessor directives altogether.

In any event the !! is pointless on an operand to || or && .

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] mm: Don't request scrubbing until dom0 is running

[Wei Liu]

On Tue, Aug 29, 2017 at 07:26:28AM -0600, Jan Beulich wrote:
> >>> On 28.08.17 at 22:40, <boris.ostrovsky@oracle.com> wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2259,7 +2259,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
> >               */
> >              scrub = !!d->is_dying;
> >  #else
> > -            scrub = true;
> > +            scrub = boot_scrub_done || !!d->is_dying;
> >  #endif
> 
> Actually Wei has a point here - already when reviewing the original
> series I had wondered whether a "#define boot_scrub_done false"
> in the non-debug case wouldn't help eliminate some #ifdef-ary.
> Here it would mean fulfilling Wei's request indirectly, by simply
> removing the preprocessor directives altogether.

This SGTM

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/4] mm: Don't hold heap lock in alloc_heap_pages() longer than necessary

[Boris Ostrovsky]

Once pages are removed from the heap we don't need to hold the heap
lock. It is especially useful to drop it for an unscrubbed buddy since
we will be scrubbing it.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/common/page_alloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index b93dae9..1ec788e 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -858,6 +858,7 @@ static struct page_info *alloc_heap_pages(
     struct page_info *pg;
     bool need_tlbflush = false;
     uint32_t tlbflush_timestamp = 0;
+    unsigned int dirty_cnt = 0;
 
     /* Make sure there are enough bits in memflags for nodeID. */
     BUILD_BUG_ON((_MEMF_bits - _MEMF_node) < (8 * sizeof(nodeid_t)));
@@ -943,6 +944,8 @@ static struct page_info *alloc_heap_pages(
 
     check_low_mem_virq();
 
+    spin_unlock(&heap_lock);
+
     if ( d != NULL )
         d->last_alloc_node = node;
 
@@ -955,7 +958,7 @@ static struct page_info *alloc_heap_pages(
         {
             if ( !(memflags & MEMF_no_scrub) )
                 scrub_one_page(&pg[i]);
-            node_need_scrub[node]--;
+            dirty_cnt++;
         }
 
         pg[i].count_info = PGC_state_inuse;
@@ -977,6 +980,8 @@ static struct page_info *alloc_heap_pages(
             check_one_page(&pg[i]);
     }
 
+    spin_lock(&heap_lock);
+    node_need_scrub[node] -= dirty_cnt;
     spin_unlock(&heap_lock);
 
     if ( need_tlbflush )
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] mm: Don't hold heap lock in alloc_heap_pages() longer than necessary

[Wei Liu]

On Mon, Aug 28, 2017 at 04:40:29PM -0400, Boris Ostrovsky wrote:
> Once pages are removed from the heap we don't need to hold the heap
> lock. It is especially useful to drop it for an unscrubbed buddy since
> we will be scrubbing it.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] mm: Don't hold heap lock in alloc_heap_pages() longer than necessary

[Jan Beulich]

>>> On 28.08.17 at 22:40, <boris.ostrovsky@oracle.com> wrote:
> @@ -943,6 +944,8 @@ static struct page_info *alloc_heap_pages(
>  
>      check_low_mem_virq();
>  
> +    spin_unlock(&heap_lock);
> +
>      if ( d != NULL )
>          d->last_alloc_node = node;

I'm not sure about the placement - as long as there's only a single
heap lock it certainly also protects the last_alloc_node updates
visible in context here. The consumer of this field also holds the
heap lock afaict, so at least for the moment it would feel more
safe if you moved the unlock past that update. With that feel
free to add
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel