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

[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

[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

[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

[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