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

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

>From: Tristan Gingold
>Sent: 2006年5月9日 21:23
>
>Hi,
>
>this patch tries to make vtlb/vhpt management interface cleaner.  No
>new
>features added.
>It also allow to flush region 7 (which fixes a bug).
>
>We really have to think on domain_dirty_cpumask and tlb_flush_mask.
>Currently, domain_dirty_cpumask is never set. Setting it kills
>performance.
>
>Tested by boot+halt of dom0+domU SMP.
>
>Tristan.

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?

- I didn't find definition for flush_local_tlb_vhpt. Is it a typo?

- remote_flush_tlb_vhpt has mismatched logic inside. You call 
vhpt_flush_address_remote together with a pct.l, that makes people 
confusing about the exact purpose of that function.

- It's better to add vcpu_flush_vtlb_all and vcpu_flush_vtlb_range too. 
Currently due to fact that only one vtlb entry for para-domain, you always 
explicitly purge one-entry itlb/dtlb directly in all places. However you 
know some places meaning range to be purged, while others meaning 
full vtlb purge like vcpu_ptc_e. Since you're refactoring vtlb purge 
interfaces, I prefer to differentiate those places though you can implement 
*_range same as *_all in current stage. Then later interfaces can keep 
untouched even if vtlb implementation is changed.

- vcpu_flush_vtlb is misleading, which seems meaning 
vcpu_local_flush_vtlb, since you call vhpt_flush() and local_flush_tlb_all() 
inside which is instead a local stub. Or you may add check upon current 
pointer to add branch to call remote version if keeping vcpu pointer as 
parameter.

- Similarly, remote_flush_tlb_vhpt is not "remote" which is still used as a 
local version.

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.
- local_ prefix: just mean a local version simply related to mTLB/VHPT.

By following above criteria, a possible list as below. (The local version 
can be also removed since included by generic one)

/* Affects all vcpu/all vtlb/all vhpt/all tlb. Here all vhpt/tlb should mean 
the processors where domain is running on */
void domain_flush_vtlb_all (struct domain *d) /* with IPI */
void domain_flush_vtlb_range(struct domain *d, u64 vadr, u64 range)
void domain_flush_destroy(struct domain *d) (A better name?)
/* with IPI */

/* 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)

/* 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)

/* Affect target vhpt/target tlb. */
void vhpt_local_flush_range(u64 vadr, u64 range)
void vhpt_flush_all(int cpu) /* with IPI */
void vhpt_local_flush_all(void)

/* Can base on vhpt_flush_all */
void flush_tlb_mask(cpumask_t mask)

Based on above base interfaces, people can wrap more or simply 
supplement several lines code before/after the invocation.

How do you think? :-)

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. At least one simple enhancement 
we can do is to change syntax of domain_dirty_cpumask. We can 
change it to indicate processors that domain is ever running on. Then 
update point only happens at creation/destroy/migration, or even 
pause/unpause. Though this simple strategy is not fine-grained, we 
can still achieve benefit especially when domain is bound.

Thanks,
Kevin

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