WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ia64-devel

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

To: "Tian, Kevin" <kevin.tian@xxxxxxxxx>, <xen-ia64-devel@xxxxxxxxxxxxxxxxxxx>, "Magenheimer, Dan \(HP Labs Fort Collins\)" <dan.magenheimer@xxxxxx>, "Alex Williamson" <alex.williamson@xxxxxx>
Subject: Re: [Xen-ia64-devel] PATCH: cleanup of tlbflush
From: Tristan Gingold <Tristan.Gingold@xxxxxxxx>
Date: Wed, 10 May 2006 09:29:08 +0200
Delivery-date: Wed, 10 May 2006 00:25:07 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <571ACEFD467F7749BC50E0A98C17CDD8094E7BF2@pdsmsx403>
List-help: <mailto:xen-ia64-devel-request@lists.xensource.com?subject=help>
List-id: Discussion of the ia64 port of Xen <xen-ia64-devel.lists.xensource.com>
List-post: <mailto:xen-ia64-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-ia64-devel>, <mailto:xen-ia64-devel-request@lists.xensource.com?subject=unsubscribe>
References: <571ACEFD467F7749BC50E0A98C17CDD8094E7BF2@pdsmsx403>
Sender: xen-ia64-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: KMail/1.5
Le Mercredi 10 Mai 2006 07:39, Tian, Kevin a écrit :
> 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?
flushtlb.h is a Xen standard header.

> - 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

> - 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.
Maybe should I write a comment.  remote_flush_tlb_vhpt should be renamed 
cpu_flush_vhpt and can work on the local cpu.

> - It's better to add vcpu_flush_vtlb_all and vcpu_flush_vtlb_range too.
vcpu_flush_vtlb_all is in fact vcpu_flush_vtlb.
vcpu_flush_vtlb_range can be added for emulation of ptc.l (which is not 
currently emulated).

> 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.
I though I did this way: *_range functions only flush a range, while 
*_flush_vtlb functions flush the whole tlb.  Maybe should I rename the later 
into *_flush_vtlb_all ?

> - 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.
Using my naming scheme, vcpu_* means local, while domain_* means global.
*_range means range, while no suffix means global.
This is at least consistent.

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

> 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.

> - 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 */
Ok for these.

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

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

> /* 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)
Shoudn't be exported.

> /* 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.
vcpu_dirty_cpumask is not used by Xen.
Updating domain_dirty_cpumask means killing performance.

> 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.
One use of domain_dirty_cpumask is to flush vtlb when a page is ungranted.
If this mask is ever set, doing IOs trash the machine.
IMHO, this is the next major Xen/ia64 challenge: dealing correctly with 
granted page.

Tristan.

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