Re: [Xen-devel] [PATCH v5 3/8] mm: Scrub pages in alloc_heap_pages() if needed

2017-07-31 Thread Boris Ostrovsky 
On 07/31/2017 11:16 AM, Jan Beulich wrote:
 Boris Ostrovsky  07/23/17 4:07 AM >>>
>> On 06/27/2017 02:00 PM, Jan Beulich wrote:
>> Boris Ostrovsky  06/22/17 8:55 PM >>>
 @@ -862,10 +879,19 @@ static struct page_info *alloc_heap_pages(
   if ( d != NULL )
   d->last_alloc_node = node;
   
 +need_scrub = !!first_dirty_pg && !(memflags & MEMF_no_scrub);
>>> No need for !! here. But I wonder whether that part of the check is really
>>> useful anyway, considering the sole use ...
>>>
   for ( i = 0; i < (1 << order); i++ )
   {
   /* Reference count must continuously be zero for free pages. */
 -BUG_ON(pg[i].count_info != PGC_state_free);
 +BUG_ON((pg[i].count_info & ~PGC_need_scrub) != PGC_state_free);
 +
 +if ( test_bit(_PGC_need_scrub, [i].count_info) )
 +{
 +if ( need_scrub )
 +scrub_one_page([i]);
>>> ... here. If it isn't, I think the local variable isn't warranted either.
>>> If you agree, the thus adjusted patch can have
>>> Reviewed-by: Jan Beulich 
>>> (otherwise I'll wait with it to understand the reason first).
>> first_dirty_pg is indeed unnecessary but I think local variable is 
>> useful to avoid ANDing memflags inside the loop on each iteration 
>> (unless you think compiler is smart enough to realize that memflags is 
>> not changing).
> I don't understand: At least on x86 I'd expect the compiler to use a
> single TEST if you used memflags inside the loop, whereas the local
> variable would likely be a single CMP inside the loop plus setup code
> outside of it.

OK, I haven't considered that you don't actually need to AND and then
CMP. Then yes, local variable is unnecessary.

-boris

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

Re: [Xen-devel] [PATCH v5 3/8] mm: Scrub pages in alloc_heap_pages() if needed

2017-07-31 Thread Jan Beulich 
>>> Boris Ostrovsky  07/23/17 4:07 AM >>>
>On 06/27/2017 02:00 PM, Jan Beulich wrote:
> Boris Ostrovsky  06/22/17 8:55 PM >>>
>>> @@ -862,10 +879,19 @@ static struct page_info *alloc_heap_pages(
>>>   if ( d != NULL )
>>>   d->last_alloc_node = node;
>>>   
>>> +need_scrub = !!first_dirty_pg && !(memflags & MEMF_no_scrub);
>> 
>> No need for !! here. But I wonder whether that part of the check is really
>> useful anyway, considering the sole use ...
>> 
>>>   for ( i = 0; i < (1 << order); i++ )
>>>   {
>>>   /* Reference count must continuously be zero for free pages. */
>>> -BUG_ON(pg[i].count_info != PGC_state_free);
>>> +BUG_ON((pg[i].count_info & ~PGC_need_scrub) != PGC_state_free);
>>> +
>>> +if ( test_bit(_PGC_need_scrub, [i].count_info) )
>>> +{
>>> +if ( need_scrub )
>>> +scrub_one_page([i]);
>> 
>> ... here. If it isn't, I think the local variable isn't warranted either.
>> If you agree, the thus adjusted patch can have
>> Reviewed-by: Jan Beulich 
>> (otherwise I'll wait with it to understand the reason first).
>
>first_dirty_pg is indeed unnecessary but I think local variable is 
>useful to avoid ANDing memflags inside the loop on each iteration 
>(unless you think compiler is smart enough to realize that memflags is 
>not changing).

I don't understand: At least on x86 I'd expect the compiler to use a
single TEST if you used memflags inside the loop, whereas the local
variable would likely be a single CMP inside the loop plus setup code
outside of it.

Jan


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

Re: [Xen-devel] [PATCH v5 3/8] mm: Scrub pages in alloc_heap_pages() if needed

2017-07-22 Thread Boris Ostrovsky 



On 06/27/2017 02:00 PM, Jan Beulich wrote:

Boris Ostrovsky  06/22/17 8:55 PM >>>

@@ -862,10 +879,19 @@ static struct page_info *alloc_heap_pages(
  if ( d != NULL )
  d->last_alloc_node = node;
  
+need_scrub = !!first_dirty_pg && !(memflags & MEMF_no_scrub);


No need for !! here. But I wonder whether that part of the check is really
useful anyway, considering the sole use ...


  for ( i = 0; i < (1 << order); i++ )
  {
  /* Reference count must continuously be zero for free pages. */
-BUG_ON(pg[i].count_info != PGC_state_free);
+BUG_ON((pg[i].count_info & ~PGC_need_scrub) != PGC_state_free);
+
+if ( test_bit(_PGC_need_scrub, [i].count_info) )
+{
+if ( need_scrub )
+scrub_one_page([i]);


... here. If it isn't, I think the local variable isn't warranted either.
If you agree, the thus adjusted patch can have
Reviewed-by: Jan Beulich 
(otherwise I'll wait with it to understand the reason first).




first_dirty_pg is indeed unnecessary but I think local variable is 
useful to avoid ANDing memflags inside the loop on each iteration 
(unless you think compiler is smart enough to realize that memflags is 
not changing).



-boris

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

Re: [Xen-devel] [PATCH v5 3/8] mm: Scrub pages in alloc_heap_pages() if needed

2017-06-27 Thread Jan Beulich 
>>> Boris Ostrovsky  06/22/17 8:55 PM >>>
> @@ -862,10 +879,19 @@ static struct page_info *alloc_heap_pages(
>  if ( d != NULL )
>  d->last_alloc_node = node;
>  
> +need_scrub = !!first_dirty_pg && !(memflags & MEMF_no_scrub);

No need for !! here. But I wonder whether that part of the check is really
useful anyway, considering the sole use ...

>  for ( i = 0; i < (1 << order); i++ )
>  {
>  /* Reference count must continuously be zero for free pages. */
> -BUG_ON(pg[i].count_info != PGC_state_free);
> +BUG_ON((pg[i].count_info & ~PGC_need_scrub) != PGC_state_free);
> +
> +if ( test_bit(_PGC_need_scrub, [i].count_info) )
> +{
> +if ( need_scrub )
> +scrub_one_page([i]);

... here. If it isn't, I think the local variable isn't warranted either.
If you agree, the thus adjusted patch can have
Reviewed-by: Jan Beulich 
(otherwise I'll wait with it to understand the reason first).

Jan

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

[Xen-devel] [PATCH v5 3/8] mm: Scrub pages in alloc_heap_pages() if needed

2017-06-22 Thread Boris Ostrovsky 
When allocating pages in alloc_heap_pages() first look for clean pages. If none
is found then retry, take pages marked as unscrubbed and scrub them.

Note that we shouldn't find unscrubbed pages in alloc_heap_pages() yet. However,
this will become possible when we stop scrubbing from free_heap_pages() and
instead do it from idle loop.

Since not all allocations require clean pages (such as xenheap allocations)
introduce MEMF_no_scrub flag that callers can set if they are willing to
consume unscrubbed pages.

Signed-off-by: Boris Ostrovsky 
---
Changes in v5:
* Added comment explaining why we always grab order 0 pages in alloc_heap_pages)
* Dropped the somewhat confusing comment about not needing to set first_dirty
  in alloc_heap_pages().
* Moved first bit of _MEMF_node by 8 to accommodate MEMF_no_scrub (bit 7 is
  no longer available)


 xen/common/page_alloc.c | 36 +++-
 xen/include/xen/mm.h|  4 +++-
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 89fe3ce..9aac196 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -703,6 +703,7 @@ static struct page_info *get_free_buddy(unsigned int 
zone_lo,
 nodemask_t nodemask = d ? d->node_affinity : node_online_map;
 unsigned int j, zone, nodemask_retry = 0;
 struct page_info *pg;
+bool use_unscrubbed = (memflags & MEMF_no_scrub);
 
 if ( node == NUMA_NO_NODE )
 {
@@ -734,8 +735,20 @@ static struct page_info *get_free_buddy(unsigned int 
zone_lo,
 
 /* Find smallest order which can satisfy the request. */
 for ( j = order; j <= MAX_ORDER; j++ )
+{
 if ( (pg = page_list_remove_head((node, zone, j))) )
-return pg;
+{
+/*
+ * We grab single pages (order=0) even if they are
+ * unscrubbed. Given that scrubbing one page is fairly 
quick
+ * it is not worth breaking higher orders.
+ */
+if ( (order == 0) || use_unscrubbed ||
+ pg->u.free.first_dirty == INVALID_DIRTY_IDX)
+return pg;
+page_list_add_tail(pg, (node, zone, j));
+}
+}
 } while ( zone-- > zone_lo ); /* careful: unsigned zone may wrap */
 
 if ( (memflags & MEMF_exact_node) && req_node != NUMA_NO_NODE )
@@ -775,7 +788,7 @@ static struct page_info *alloc_heap_pages(
 unsigned int i, buddy_order, zone;
 unsigned long request = 1UL << order;
 struct page_info *pg, *first_dirty_pg = NULL;
-bool_t need_tlbflush = 0;
+bool need_scrub, need_tlbflush = false;
 uint32_t tlbflush_timestamp = 0;
 
 /* Make sure there are enough bits in memflags for nodeID. */
@@ -819,6 +832,10 @@ static struct page_info *alloc_heap_pages(
  }
  
 pg = get_free_buddy(zone_lo, zone_hi, order, memflags, d);
+/* Try getting a dirty buddy if we couldn't get a clean one. */
+if ( !pg && !(memflags & MEMF_no_scrub) )
+pg = get_free_buddy(zone_lo, zone_hi, order,
+memflags | MEMF_no_scrub, d);
 if ( !pg )
 {
 /* No suitable memory blocks. Fail the request. */
@@ -862,10 +879,19 @@ static struct page_info *alloc_heap_pages(
 if ( d != NULL )
 d->last_alloc_node = node;
 
+need_scrub = !!first_dirty_pg && !(memflags & MEMF_no_scrub);
 for ( i = 0; i < (1 << order); i++ )
 {
 /* Reference count must continuously be zero for free pages. */
-BUG_ON(pg[i].count_info != PGC_state_free);
+BUG_ON((pg[i].count_info & ~PGC_need_scrub) != PGC_state_free);
+
+if ( test_bit(_PGC_need_scrub, [i].count_info) )
+{
+if ( need_scrub )
+scrub_one_page([i]);
+node_need_scrub[node]--;
+}
+
 pg[i].count_info = PGC_state_inuse;
 
 if ( !(memflags & MEMF_no_tlbflush) )
@@ -1749,7 +1775,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned 
int memflags)
 ASSERT(!in_irq());
 
 pg = alloc_heap_pages(MEMZONE_XEN, MEMZONE_XEN,
-  order, memflags, NULL);
+  order, memflags | MEMF_no_scrub, NULL);
 if ( unlikely(pg == NULL) )
 return NULL;
 
@@ -1799,7 +1825,7 @@ void *alloc_xenheap_pages(unsigned int order, unsigned 
int memflags)
 if ( !(memflags >> _MEMF_bits) )
 memflags |= MEMF_bits(xenheap_bits);
 
-pg = alloc_domheap_pages(NULL, order, memflags);
+pg = alloc_domheap_pages(NULL, order, memflags | MEMF_no_scrub);
 if ( unlikely(pg == NULL) )
 return NULL;
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3d3f31b..5f3d84a 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -238,7 +238,9 @@ struct npfec {
 #define  MEMF_no_tlbflush