RE: [Xen-ia64-devel][PATCH] Enable SMP on VTI domain.

["Xu, Anthony" <anthony.xu@xxxxxxxxx>]

See comments

>From: Isaku Yamahata [mailto:yamahata@xxxxxxxxxxxxx]
>Sent: 2006?6?29? 16:03
>To: Xu, Anthony
>Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
>Subject: Re: [Xen-ia64-devel][PATCH] Enable SMP on VTI domain.
>
>Hi Anthony.
>
>On Thu, Jun 29, 2006 at 02:21:48PM +0800, Xu, Anthony wrote:
>> >From: Isaku Yamahata [mailto:yamahata@xxxxxxxxxxxxx]
>> >Sent: 2006?6?21? 17:25
>> >To: Xu, Anthony
>> >Cc: xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
>> >Subject: Re: [Xen-ia64-devel][PATCH] Enable SMP on VTI domain.
>> >
>> >
>> >On Thu, Jun 01, 2006 at 12:55:08PM +0800, Xu, Anthony wrote:
>> >
>> >> >> Or you mean the protection of global purge.
>> >> >> When a vcpu get IPI to purge TLB,
>> >> >> What it does is to invalid the TLB entry in VHPT,
>> >> >> but not remove the TLB entry.
>> >> >> There is no race condition.
>> >> >
>> >> >Is there any gurantee that the vcpu which recives IPI isn't touching 
>> >> >VHPT?
>> >>
>> >> The vcpu which receives IPI can touch VHPT in the same time.
>> >> Because purge operation only sets the TLB entry invalid, like entry->ti=1.
>> >> That has the same philosophy with Tristan's direct purge
>> >
>> >Could you review the two attached patches?
>> >Purge function traverses the collision chain when IPI is sent.
>> >But there is a window when the assumption of the collision chain
>> >is broken.
>> >vmx_hpw_miss() has a race. ia64_do_page_fault() had a similar race before.
>> >
>> >--
>>
>> Sorry for late response.
>>
>> The second patch is good cleanup and improvement.
>
>The second patch is also a bug fix patch.
>
>
>> I don't understand the race condition the first patch fixes.
>>
>> Could you please elaborate this?
>
>The patch fixes two races.
>- a race in vmx_process() of vmx_process.c
>  The same race was there in ia64_do_page_fault() before.
>  The check, (fault == IA64_USE_TLB && !current->arch.dtlb.pte.p), in
>  ia64_do_page_fault() avoids this race.
VTI-domain doesn't have this issue, which is introduced by 1-entry TLB

>
>- a race in vtlb.c
>  vmx_vcpu_ptc_l() needs a certain condition on the collision chain
>  to traverse collision chains and purge entries correctly.
>  But there are windows when the condision is broken.
>  The critical areas are surrounded by local_irq_save() and local_irq_restore()
>  with the patch.
>  If a vcpu send a IPI to another vcpu for ptc.ga when another vcpu is
>  in the critical area, things goes bad.
>
There seems be some race conditions. But I had used correct operation sequence 
on VTLB to avoid these race conditions. local_irq_save() and 
local_irq_restore() 
are not needed, some mb may be needed to guarantee the memory access order.

>
>  Actually the patch assumes that the targeted vcpu is still
>  on the physical cpu which received IPI.
>  It might be reasonable to assume so before credit scheduler was
>  introduced...
I don't assume the targeted vcpu is running on the physical cpu.

>
>  If the targeted vcpu moved to another physical cpu,
>  the collision chain is traversed on the IPI'ed physical cpu and
>  the collision chain is modified on the physical cpu on which the
>  targeted vcpu runs at the same time.
>  The collision chain modification/traverse code doesn't seem to
>  be lock-free. Something bad would happen.
>
>
>In fact I suspect that there still remains a problem.
>I haven't checked the generated assembler code and I'm not sure though.
>- vmx_vcpu_ptc_ga()
>  while (proc != v->processor);
>
>  v->processor's type is int.
>  However v->processor might be changed asynchronously by vcpu scheduler
>  on another physical cpu.
>  Compiler barrier and memory barrier might be needed somewhere.
>
I didn't think of vcpu being scheduled to other LP by far.

>--
>yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel