[PATCH] x86: correct vCPU dirty CPU handling

[PATCH] x86: correct vCPU dirty CPU handling

[Jan Beulich]

Commit df8234fd2c ("replace vCPU's dirty CPU mask by numeric ID") was
too lax in two respects: First of all it didn't consider the case of a
vCPU not having a valid dirty CPU in the descriptor table TLB flush
case. This is the issue Manual has run into with NetBSD.

Additionally reads of ->dirty_cpu for other than the current vCPU are at
risk of racing with scheduler actions, i.e. single atomic reads need to
be used there. Obviously the non-init write sites then better also use
atomic writes.

Having to touch the descriptor table TLB flush code here anyway, take
the opportunity and switch it to be at most one flush_tlb_mask()
invocation.

Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1631,7 +1631,7 @@ static void __context_switch(void)
      */
     if ( pd != nd )
         cpumask_set_cpu(cpu, nd->dirty_cpumask);
-    n->dirty_cpu = cpu;
+    write_atomic(&n->dirty_cpu, cpu);
 
     if ( !is_idle_domain(nd) )
     {
@@ -1687,7 +1687,7 @@ static void __context_switch(void)
 
     if ( pd != nd )
         cpumask_clear_cpu(cpu, pd->dirty_cpumask);
-    p->dirty_cpu = VCPU_CPU_CLEAN;
+    write_atomic(&p->dirty_cpu, VCPU_CPU_CLEAN);
 
     per_cpu(curr_vcpu, cpu) = n;
 }
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
              unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
              (l1e_owner == pg_owner) )
         {
+            cpumask_t *mask = this_cpu(scratch_cpumask);
+
+            cpumask_clear(mask);
+
             for_each_vcpu ( pg_owner, v )
             {
-                if ( pv_destroy_ldt(v) )
-                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
+                unsigned int cpu;
+
+                if ( !pv_destroy_ldt(v) )
+                    continue;
+                cpu = read_atomic(&v->dirty_cpu);
+                if ( is_vcpu_dirty_cpu(cpu) )
+                    __cpumask_set_cpu(cpu, mask);
             }
+
+            if ( !cpumask_empty(mask) )
+                flush_tlb_mask(mask);
         }
         put_page(page);
     }
@@ -2979,13 +2991,18 @@ static inline int vcpumask_to_pcpumask(
 
         while ( vmask )
         {
+            unsigned int cpu;
+
             vcpu_id = find_first_set_bit(vmask);
             vmask &= ~(1UL << vcpu_id);
             vcpu_id += vcpu_bias;
             if ( (vcpu_id >= d->max_vcpus) )
                 return 0;
-            if ( ((v = d->vcpu[vcpu_id]) != NULL) && vcpu_cpu_dirty(v) )
-                __cpumask_set_cpu(v->dirty_cpu, pmask);
+            if ( (v = d->vcpu[vcpu_id]) == NULL )
+                continue;
+            cpu = read_atomic(&v->dirty_cpu);
+            if ( is_vcpu_dirty_cpu(cpu) )
+                __cpumask_set_cpu(cpu, pmask);
         }
     }
 }
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -795,10 +795,15 @@ static inline int vcpu_runnable(struct v
              atomic_read(&v->domain->pause_count));
 }
 
-static inline bool vcpu_cpu_dirty(const struct vcpu *v)
+static inline bool is_vcpu_dirty_cpu(unsigned int cpu)
 {
     BUILD_BUG_ON(NR_CPUS >= VCPU_CPU_CLEAN);
-    return v->dirty_cpu != VCPU_CPU_CLEAN;
+    return cpu != VCPU_CPU_CLEAN;
+}
+
+static inline bool vcpu_cpu_dirty(const struct vcpu *v)
+{
+    return is_vcpu_dirty_cpu(v->dirty_cpu);
 }
 
 void vcpu_block(void);




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

Re: [PATCH] x86: correct vCPU dirty CPU handling

[Andrew Cooper]

On 26/04/18 10:41, Jan Beulich wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>               (l1e_owner == pg_owner) )
>          {
> +            cpumask_t *mask = this_cpu(scratch_cpumask);
> +
> +            cpumask_clear(mask);
> +
>              for_each_vcpu ( pg_owner, v )
>              {
> -                if ( pv_destroy_ldt(v) )
> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
> +                unsigned int cpu;
> +
> +                if ( !pv_destroy_ldt(v) )
> +                    continue;
> +                cpu = read_atomic(&v->dirty_cpu);
> +                if ( is_vcpu_dirty_cpu(cpu) )
> +                    __cpumask_set_cpu(cpu, mask);
>              }
> +
> +            if ( !cpumask_empty(mask) )
> +                flush_tlb_mask(mask);

Thinking about this, what is wrong with:

bool flush;

for_each_vcpu ( pg_owner, v )
    if ( pv_destroy_ldt(v) )
        flush = true;

if ( flush )
   flush_tlb_mask(pg_owner->dirty_cpumask);

This is far less complicated cpumask handling.  As the loop may be long,
it avoids flushing pcpus which have subsequently switched away from
pg_owner context.  It also avoids all playing with v->dirty_cpu.

~Andrew

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

Re: [PATCH] x86: correct vCPU dirty CPU handling

[Jan Beulich]

>>> On 26.04.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
> On 26/04/18 10:41, Jan Beulich wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>>               (l1e_owner == pg_owner) )
>>          {
>> +            cpumask_t *mask = this_cpu(scratch_cpumask);
>> +
>> +            cpumask_clear(mask);
>> +
>>              for_each_vcpu ( pg_owner, v )
>>              {
>> -                if ( pv_destroy_ldt(v) )
>> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
>> +                unsigned int cpu;
>> +
>> +                if ( !pv_destroy_ldt(v) )
>> +                    continue;
>> +                cpu = read_atomic(&v->dirty_cpu);
>> +                if ( is_vcpu_dirty_cpu(cpu) )
>> +                    __cpumask_set_cpu(cpu, mask);
>>              }
>> +
>> +            if ( !cpumask_empty(mask) )
>> +                flush_tlb_mask(mask);
> 
> Thinking about this, what is wrong with:
> 
> bool flush;
> 
> for_each_vcpu ( pg_owner, v )
>     if ( pv_destroy_ldt(v) )
>         flush = true;
> 
> if ( flush )
>    flush_tlb_mask(pg_owner->dirty_cpumask);
> 
> This is far less complicated cpumask handling.  As the loop may be long,
> it avoids flushing pcpus which have subsequently switched away from
> pg_owner context.  It also avoids all playing with v->dirty_cpu.

That would look to be correct, but I'm not sure it would be an improvement:
While it may avoid flushing some CPUs, it may then do extra flushes on
others (which another vCPU of the domain has been switched to). Plus it
would flush even those CPUs where pv_destroy_ldt() has returned false,
as long as the function returned true at least once.

If I was to go that route, I'd at least extend to latching
pg_owner->dirty_cpumask before the loop into scratch_cpumask, ANDing
in pg_owner->dirty_cpumask after the loop to restrict to those CPUs which
may have remained active over the entire time the loop takes. But even
then I would still be afraid of flushing far more CPUs than actually needed.

Jan



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

Ping: Re: [PATCH] x86: correct vCPU dirty CPU handling

[Jan Beulich]

>>> On 26.04.18 at 12:52, <JBeulich@suse.com> wrote:
>>>> On 26.04.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> On 26/04/18 10:41, Jan Beulich wrote:
>>> --- a/xen/arch/x86/mm.c
>>> +++ b/xen/arch/x86/mm.c
>>> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>>>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>>>               (l1e_owner == pg_owner) )
>>>          {
>>> +            cpumask_t *mask = this_cpu(scratch_cpumask);
>>> +
>>> +            cpumask_clear(mask);
>>> +
>>>              for_each_vcpu ( pg_owner, v )
>>>              {
>>> -                if ( pv_destroy_ldt(v) )
>>> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
>>> +                unsigned int cpu;
>>> +
>>> +                if ( !pv_destroy_ldt(v) )
>>> +                    continue;
>>> +                cpu = read_atomic(&v->dirty_cpu);
>>> +                if ( is_vcpu_dirty_cpu(cpu) )
>>> +                    __cpumask_set_cpu(cpu, mask);
>>>              }
>>> +
>>> +            if ( !cpumask_empty(mask) )
>>> +                flush_tlb_mask(mask);
>> 
>> Thinking about this, what is wrong with:
>> 
>> bool flush;
>> 
>> for_each_vcpu ( pg_owner, v )
>>     if ( pv_destroy_ldt(v) )
>>         flush = true;
>> 
>> if ( flush )
>>    flush_tlb_mask(pg_owner->dirty_cpumask);
>> 
>> This is far less complicated cpumask handling.  As the loop may be long,
>> it avoids flushing pcpus which have subsequently switched away from
>> pg_owner context.  It also avoids all playing with v->dirty_cpu.
> 
> That would look to be correct, but I'm not sure it would be an improvement:
> While it may avoid flushing some CPUs, it may then do extra flushes on
> others (which another vCPU of the domain has been switched to). Plus it
> would flush even those CPUs where pv_destroy_ldt() has returned false,
> as long as the function returned true at least once.

Ping?

> If I was to go that route, I'd at least extend to latching
> pg_owner->dirty_cpumask before the loop into scratch_cpumask, ANDing
> in pg_owner->dirty_cpumask after the loop to restrict to those CPUs which
> may have remained active over the entire time the loop takes. But even
> then I would still be afraid of flushing far more CPUs than actually needed.

I don't think anymore that this would be correct, so please ignore this
part.

Thanks, Jan



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

Re: Ping: Re: [PATCH] x86: correct vCPU dirty CPU handling

[Andrew Cooper]

On 15/05/18 09:25, Jan Beulich wrote:
>>>> On 26.04.18 at 12:52, <JBeulich@suse.com> wrote:
>>>>> On 26.04.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> On 26/04/18 10:41, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/mm.c
>>>> +++ b/xen/arch/x86/mm.c
>>>> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>>>>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>>>>               (l1e_owner == pg_owner) )
>>>>          {
>>>> +            cpumask_t *mask = this_cpu(scratch_cpumask);
>>>> +
>>>> +            cpumask_clear(mask);
>>>> +
>>>>              for_each_vcpu ( pg_owner, v )
>>>>              {
>>>> -                if ( pv_destroy_ldt(v) )
>>>> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
>>>> +                unsigned int cpu;
>>>> +
>>>> +                if ( !pv_destroy_ldt(v) )
>>>> +                    continue;
>>>> +                cpu = read_atomic(&v->dirty_cpu);
>>>> +                if ( is_vcpu_dirty_cpu(cpu) )
>>>> +                    __cpumask_set_cpu(cpu, mask);
>>>>              }
>>>> +
>>>> +            if ( !cpumask_empty(mask) )
>>>> +                flush_tlb_mask(mask);
>>> Thinking about this, what is wrong with:
>>>
>>> bool flush;
>>>
>>> for_each_vcpu ( pg_owner, v )
>>>     if ( pv_destroy_ldt(v) )
>>>         flush = true;
>>>
>>> if ( flush )
>>>    flush_tlb_mask(pg_owner->dirty_cpumask);
>>>
>>> This is far less complicated cpumask handling.  As the loop may be long,
>>> it avoids flushing pcpus which have subsequently switched away from
>>> pg_owner context.  It also avoids all playing with v->dirty_cpu.
>> That would look to be correct, but I'm not sure it would be an improvement:
>> While it may avoid flushing some CPUs, it may then do extra flushes on
>> others (which another vCPU of the domain has been switched to). Plus it
>> would flush even those CPUs where pv_destroy_ldt() has returned false,
>> as long as the function returned true at least once.
> Ping?

I'm not sure it is worth trying to optimise this code.  I've got a patch
for 4.12 to leave it compiled out by default.

Therefore, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> on the
original patch.

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

Re: Ping: Re: [PATCH] x86: correct vCPU dirty CPU handling

[Juergen Gross]

On 22/05/18 14:29, Andrew Cooper wrote:
> On 15/05/18 09:25, Jan Beulich wrote:
>>>>> On 26.04.18 at 12:52, <JBeulich@suse.com> wrote:
>>>>>> On 26.04.18 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> On 26/04/18 10:41, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/mm.c
>>>>> +++ b/xen/arch/x86/mm.c
>>>>> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>>>>>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>>>>>               (l1e_owner == pg_owner) )
>>>>>          {
>>>>> +            cpumask_t *mask = this_cpu(scratch_cpumask);
>>>>> +
>>>>> +            cpumask_clear(mask);
>>>>> +
>>>>>              for_each_vcpu ( pg_owner, v )
>>>>>              {
>>>>> -                if ( pv_destroy_ldt(v) )
>>>>> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
>>>>> +                unsigned int cpu;
>>>>> +
>>>>> +                if ( !pv_destroy_ldt(v) )
>>>>> +                    continue;
>>>>> +                cpu = read_atomic(&v->dirty_cpu);
>>>>> +                if ( is_vcpu_dirty_cpu(cpu) )
>>>>> +                    __cpumask_set_cpu(cpu, mask);
>>>>>              }
>>>>> +
>>>>> +            if ( !cpumask_empty(mask) )
>>>>> +                flush_tlb_mask(mask);
>>>> Thinking about this, what is wrong with:
>>>>
>>>> bool flush;
>>>>
>>>> for_each_vcpu ( pg_owner, v )
>>>>     if ( pv_destroy_ldt(v) )
>>>>         flush = true;
>>>>
>>>> if ( flush )
>>>>    flush_tlb_mask(pg_owner->dirty_cpumask);
>>>>
>>>> This is far less complicated cpumask handling.  As the loop may be long,
>>>> it avoids flushing pcpus which have subsequently switched away from
>>>> pg_owner context.  It also avoids all playing with v->dirty_cpu.
>>> That would look to be correct, but I'm not sure it would be an improvement:
>>> While it may avoid flushing some CPUs, it may then do extra flushes on
>>> others (which another vCPU of the domain has been switched to). Plus it
>>> would flush even those CPUs where pv_destroy_ldt() has returned false,
>>> as long as the function returned true at least once.
>> Ping?
> 
> I'm not sure it is worth trying to optimise this code.  I've got a patch
> for 4.12 to leave it compiled out by default.
> 
> Therefore, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> on the
> original patch.
> 

You can add my

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH] x86: correct vCPU dirty CPU handling

[Manuel Bouyer]

On Thu, Apr 26, 2018 at 03:41:20AM -0600, Jan Beulich wrote:
> Commit df8234fd2c ("replace vCPU's dirty CPU mask by numeric ID") was
> too lax in two respects: First of all it didn't consider the case of a
> vCPU not having a valid dirty CPU in the descriptor table TLB flush
> case. This is the issue Manual has run into with NetBSD.

Hello,
I tested this patch with NetBSD, it looks good.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

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

Re: [PATCH] x86: correct vCPU dirty CPU handling

[Manuel Bouyer]

Hello,
I still had to apply the patch below with 4.11rc4 for NetBSD.
Any chance to get it in the 4.11 branch before release ?

On Thu, Apr 26, 2018 at 03:41:20AM -0600, Jan Beulich wrote:
> Commit df8234fd2c ("replace vCPU's dirty CPU mask by numeric ID") was
> too lax in two respects: First of all it didn't consider the case of a
> vCPU not having a valid dirty CPU in the descriptor table TLB flush
> case. This is the issue Manual has run into with NetBSD.
> 
> Additionally reads of ->dirty_cpu for other than the current vCPU are at
> risk of racing with scheduler actions, i.e. single atomic reads need to
> be used there. Obviously the non-init write sites then better also use
> atomic writes.
> 
> Having to touch the descriptor table TLB flush code here anyway, take
> the opportunity and switch it to be at most one flush_tlb_mask()
> invocation.
> 
> Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1631,7 +1631,7 @@ static void __context_switch(void)
>       */
>      if ( pd != nd )
>          cpumask_set_cpu(cpu, nd->dirty_cpumask);
> -    n->dirty_cpu = cpu;
> +    write_atomic(&n->dirty_cpu, cpu);
>  
>      if ( !is_idle_domain(nd) )
>      {
> @@ -1687,7 +1687,7 @@ static void __context_switch(void)
>  
>      if ( pd != nd )
>          cpumask_clear_cpu(cpu, pd->dirty_cpumask);
> -    p->dirty_cpu = VCPU_CPU_CLEAN;
> +    write_atomic(&p->dirty_cpu, VCPU_CPU_CLEAN);
>  
>      per_cpu(curr_vcpu, cpu) = n;
>  }
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1202,11 +1202,23 @@ void put_page_from_l1e(l1_pgentry_t l1e,
>               unlikely(((page->u.inuse.type_info & PGT_count_mask) != 0)) &&
>               (l1e_owner == pg_owner) )
>          {
> +            cpumask_t *mask = this_cpu(scratch_cpumask);
> +
> +            cpumask_clear(mask);
> +
>              for_each_vcpu ( pg_owner, v )
>              {
> -                if ( pv_destroy_ldt(v) )
> -                    flush_tlb_mask(cpumask_of(v->dirty_cpu));
> +                unsigned int cpu;
> +
> +                if ( !pv_destroy_ldt(v) )
> +                    continue;
> +                cpu = read_atomic(&v->dirty_cpu);
> +                if ( is_vcpu_dirty_cpu(cpu) )
> +                    __cpumask_set_cpu(cpu, mask);
>              }
> +
> +            if ( !cpumask_empty(mask) )
> +                flush_tlb_mask(mask);
>          }
>          put_page(page);
>      }
> @@ -2979,13 +2991,18 @@ static inline int vcpumask_to_pcpumask(
>  
>          while ( vmask )
>          {
> +            unsigned int cpu;
> +
>              vcpu_id = find_first_set_bit(vmask);
>              vmask &= ~(1UL << vcpu_id);
>              vcpu_id += vcpu_bias;
>              if ( (vcpu_id >= d->max_vcpus) )
>                  return 0;
> -            if ( ((v = d->vcpu[vcpu_id]) != NULL) && vcpu_cpu_dirty(v) )
> -                __cpumask_set_cpu(v->dirty_cpu, pmask);
> +            if ( (v = d->vcpu[vcpu_id]) == NULL )
> +                continue;
> +            cpu = read_atomic(&v->dirty_cpu);
> +            if ( is_vcpu_dirty_cpu(cpu) )
> +                __cpumask_set_cpu(cpu, pmask);
>          }
>      }
>  }
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -795,10 +795,15 @@ static inline int vcpu_runnable(struct v
>               atomic_read(&v->domain->pause_count));
>  }
>  
> -static inline bool vcpu_cpu_dirty(const struct vcpu *v)
> +static inline bool is_vcpu_dirty_cpu(unsigned int cpu)
>  {
>      BUILD_BUG_ON(NR_CPUS >= VCPU_CPU_CLEAN);
> -    return v->dirty_cpu != VCPU_CPU_CLEAN;
> +    return cpu != VCPU_CPU_CLEAN;
> +}
> +
> +static inline bool vcpu_cpu_dirty(const struct vcpu *v)
> +{
> +    return is_vcpu_dirty_cpu(v->dirty_cpu);
>  }
>  
>  void vcpu_block(void);
> 
> 
> 
-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

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

Re: [PATCH] x86: correct vCPU dirty CPU handling

[Jan Beulich]

>>> On 22.05.18 at 13:02, <bouyer@antioche.eu.org> wrote:
> I still had to apply the patch below with 4.11rc4 for NetBSD.
> Any chance to get it in the 4.11 branch before release ?

See the thread "Pending patches for 4.11?" IOW - I very much hope so.

Jan



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