From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Bruce Rogers" Subject: Re: [PATCH] respin of mprotect performance patch Date: Thu, 10 Jan 2008 10:13:56 -0700 Message-ID: <4785EFE2.092E.0048.1@novell.com> References: <47826C1C.5C6B.0048.1@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser , xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org >>> On 1/10/2008 at 8:52 AM, in message , Keir Fraser wrote: > It's taken me a while to work out that MMU_ATOMIC_PT_UPDATE is actually > 'atomic' in that it preserves the existing A/D bits (and should ignore = the > ones specified in the given new value). Am I correct? >>From the point where the new bits are determined to the point where the = pte is actually updated, the only way that the new value can be out of = date is if the hardware mmu has updated (set) the A/D bits. I don't = believe they could have been cleared. So I see it as not ignoring the A/D = bits, but accepting that they may have been updated (set) and allowing for = that change. >=20 > In which case: > (1) The name and description in xen.h should be better. E.g., > MMU_PT_UPDATE_PRESERVE_AD, with a corresponding comment explaining the > atomicity/consistency guarantee. That would be fine. I didn't know what other architectures might have = slightly different semantics so I stuck with a general (wider coverage) = *atomic* nomenclature. Since it's been pointed out that mmu_ops is x86 = specific, then specifying that we're talking about the treatment of the = A/D bits would be fine. > (2) The implementation is actually wrong, as it only attempts to = propagate > A/D bits into the 'new' value if the first cmpxchg attempt fails. = Otherwise > the A/D values passed in by the guest are used, but the PTE might change > before Xen itself reads the ol1e. Yes - I see that. So we need to OR in those bits from ol1e into the new = value before the cmpxchg. Thanks for catching that blunder. > (3) In general, the hypercall implementation can probably be slimmed = down a > bunch. There's lots of code duplication. Agreed. I wasn't sure how much I wanted to disrupt the other (existing) = code paths, but it could certainly be combined. >=20 > I can take a swing at the Xen side of this, if I haven't got the wrong = end > of the stick? That would be great. Do you have any comment on whether the second hypercall is useful enough = to keep? It would be quite simple for me to rip it out if not. - Bruce >=20 > -- Keir >=20 > On 8/1/08 01:05, "Bruce Rogers" wrote: >=20 >> I've incorporated the comments of Jan and Keir into the attached = patches,=20 > with >> the exception of moving mmu_ops related entries from public/xen.h into >> architecture specific headers. >> Thanks for your review. Let me know if I've missed anything. >>=20 >> Signed-off-by: Bruce Rogers >>=20 >> - Bruce >>=20 >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com=20 >> http://lists.xensource.com/xen-devel