RE: [Xen-ia64-devel] PATCH: cleanup of tlbflush

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

>From: Tristan Gingold [mailto:Tristan.Gingold@xxxxxxxx]
>Sent: 2006年5月10日 15:29
>> Hi, Tristan,
>>      Some comments here:
>>
>> - How about renaming include/asm-ia64/flushtlb.h to tlbflush.h, and
>thus
>> avoid several #ifndef XEN in C linux files?
>flushtlb.h is a Xen standard header.

It's only x86 specific header. No common file directly includes it.

>
>> - I didn't find definition for flush_local_tlb_vhpt. Is it a typo?
>Where is flush_local_tlb_vhpt declared ?
>If you mean local_flush_tlb_all, it is declared in linux-xen/tlb.c

I mean following code:
@@ -723,17 +719,7 @@
 domain_page_flush(struct domain* d, unsigned long mpaddr,
                   unsigned long old_mfn, unsigned long new_mfn)
 {
-    struct vcpu* v;
-    //XXX SMP
-    for_each_vcpu(d, v) {
-        vcpu_purge_tr_entry(&v->arch.dtlb);
-        vcpu_purge_tr_entry(&v->arch.itlb);
-    }
-
-    // flush vhpt
-    vhpt_flush();
-    // flush tlb
-    flush_tlb_all();
+    flush_local_tlb_vhpt (d);
 }

>Using my naming scheme, vcpu_* means local, while domain_* means

However you also passes in a struct vcpu * pointer into vcpu_*, which 
gives illusion that it can be used for any vcpu.

>> - Similarly, remote_flush_tlb_vhpt is not "remote" which is still used as
>a
>> local version.
>Sure, but it is static.

Yes, it's static, and only executed in the local processor. So that's why
 I say 'remote' misleading, since you also has vhpt_*_remote to operate 
content on other processor.

>
>> So, I suggest making a full interface list first with clear name defined,
>> Following are some of my thoughts:
>> - Domain_ prefix: it doesn't need special local or remote version
>> - Vcpu_ prefix: may need local version. Vcpu_local_function (...)
>means
>> a local vtlb operation on current vcpu. Vcpu_function(struct vcpu*,...)
>> means that operation can happen on any vcpu assigned by the first
>> parameter. The vcpu pointer already includes remote possibility here.
>Many of the vcpu_ functions only work on current vcpu.  I keep this
>restriction.

If only work on current vcpu, no need to pass struct vcpu* pointer as 
parameter, which is confusing.

>> /* Affects target vcpu/target vtlb/all vhpt/all tlb. Here all vhpt/tlb
>> should mean the processors where target vcpu is ever running on. In
>current
>> stage, this can be considered same as above domain wise */
>> void vcpu_flush_vtlb_all(struct vcpu *v)
>> void vcpu_local_flush_vtlb_all(void)
>> void vcpu_flush_vtlb_range(struct vcpu *v, u64 vadr, u64 range)
>> void vcpu_local_flush_vtlb_range(void)
>We don't need both version (ie with and without vcpu parameter).
>vcpu_local_flush_vtlb_range without parameters is strange too :-)

My typo, and sorry. Up to you to decide whether keeping local version. 
You may define local version as wrapping to common ones by passing 
current as parameter. Local version may help later developers easier to 
understand when/where local flush can be used.

>
>> /* Affects target vhpt only and give caller choice how to purge target
>> tlb. For example caller may issue ptc.g after vhpt purge on all
>> processors like ptc.g emulation */
>> void vhpt_flush_range(int cpu, u64 vadr, u64 range)
>Shouldn't be exported.

Yes, should be static. I didn't think deeply and just list some skeleton 
for your reference. :-)

>> BTW, agree that domain_dirty_cpumask is required. So do
>> vcpu_dirty_cpumask. Not sure how much performance influence if
>> we update them at context switch.
>vcpu_dirty_cpumask is not used by Xen.

It's used by xen/x86 and you can grep. Having both vcpu_dirty_cpumask 
and domain_dirty_cpumask can help you better control which processors 
to be flushed. For example, if domain has 16 vcpus individually bound to
 different 16 physical processors. Having vcpu_dirty_cpumask can help 
reduce system-bus traffic.

Thanks,
Kevin

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