Bruce Rogers
2008-Jan-08 01:05 UTC
[Xen-devel] [PATCH] respin of mprotect performance patch
I''ve incorporated the comments of Jan and Keir into the attached patches, 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. Signed-off-by: Bruce Rogers <brogers@novell.com> - Bruce _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-10 15:52 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
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? 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. (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. (3) In general, the hypercall implementation can probably be slimmed down a bunch. There''s lots of code duplication. I can take a swing at the Xen side of this, if I haven''t got the wrong end of the stick? -- Keir On 8/1/08 01:05, "Bruce Rogers" <brogers@novell.com> wrote:> I''ve incorporated the comments of Jan and Keir into the attached patches, 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. > > Signed-off-by: Bruce Rogers <brogers@novell.com> > > - Bruce > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Bruce Rogers
2008-Jan-10 17:13 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
>>> On 1/10/2008 at 8:52 AM, in message <C3ABEFCB.1A5C8%Keir.Fraser@cl.cam.ac.uk>,Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 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. > > 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.> > 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> > -- Keir > > On 8/1/08 01:05, "Bruce Rogers" <brogers@novell.com> wrote: > >> I''ve incorporated the comments of Jan and Keir into the attached patches, > 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. >> >> Signed-off-by: Bruce Rogers <brogers@novell.com> >> >> - Bruce >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-10 17:42 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On 10/1/08 17:13, "Bruce Rogers" <BROGERS@novell.com> wrote:>> 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.All right, I''ll take a look.> 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.It''s kind of odd, being a batched sub-command within a batched hypercall. I think we''ll leave it out for now unless there''s a really compelling reason to introduce it. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-12 11:47 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
Bruce, Attached is my equivalent implementation of ATOMIC_PT_UPDATE. Notice that I changed the name to MMU_PT_UPDATE_PRESERVE_AD, also that I removed a bunch of code duplication albeit at the expense of passing yet another parameter around. Could you rev your Linux patch and give it a spin? Thanks, Keir On 8/1/08 01:05, "Bruce Rogers" <brogers@novell.com> wrote:> I''ve incorporated the comments of Jan and Keir into the attached patches, 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. > > Signed-off-by: Bruce Rogers <brogers@novell.com> > > - Bruce > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Bruce Rogers
2008-Jan-13 03:08 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
I made a few tweaks to your patch as follows: As I see it we need to preserve the existing AD bits but additionally OR in the AD bits being passed in (see behavior of pte_modify(), where it preserves current AD in use of _PAGE_CHG_MASK along with ORing in newprot bits, which include AD bits (A at least).) See update_intpte() for this change. Also we need to remove cmd value from req.ptr in do_mmu_update.. Updated Linux patch is also included. Let me know if I''m missing the point some how. Signed-off-by: Bruce Rogers <brogers@novell.com> - Bruce>>> On Sat, Jan 12, 2008 at 4:47 AM, in message<C3AE5951.12129%Keir.Fraser@cl.cam.ac.uk>, Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:> Bruce, > > Attached is my equivalent implementation of ATOMIC_PT_UPDATE. Notice that I > changed the name to MMU_PT_UPDATE_PRESERVE_AD, also that I removed a bunch > of code duplication albeit at the expense of passing yet another parameter > around. > > Could you rev your Linux patch and give it a spin? > > Thanks, > Keir > > On 8/1/08 01:05, "Bruce Rogers" <brogers@novell.com> wrote: > >> I''ve incorporated the comments of Jan and Keir into the attached patches, > 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. >> >> Signed-off-by: Bruce Rogers <brogers@novell.com> >> >> - Bruce >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Jan-13 09:21 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On 13/1/08 03:08, "Bruce Rogers" <brogers@novell.com> wrote:> I made a few tweaks to your patch as follows: > As I see it we need to preserve the existing AD bits but additionally OR in > the AD bits being passed in (see behavior of pte_modify(), where it preserves > current AD in use of _PAGE_CHG_MASK along with ORing in newprot bits, which > include AD bits (A at least).) > See update_intpte() for this change.This way is strictly more expressive (you can get my behaviour by clearing A/D bits in the passed-in ''val''). So seems good to me.> Also we need to remove cmd value from req.ptr in do_mmu_update..Yes, missed that. Looks good! -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Rik van Riel
2008-Jan-21 15:41 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On Sat, 12 Jan 2008 20:08:06 -0700 "Bruce Rogers" <brogers@novell.com> wrote:> I made a few tweaks to your patch as follows: > As I see it we need to preserve the existing AD bits but additionally OR in the AD bits being passed in (see behavior of pte_modify(), where it preserves current AD in use of _PAGE_CHG_MASK along with ORing in newprot bits, which include AD bits (A at least).) > See update_intpte() for this change. > > Also we need to remove cmd value from req.ptr in do_mmu_update.. > > Updated Linux patch is also included. > > Let me know if I''m missing the point some how.I tested a quick backport of these patches to XEN 3.1.2 and kernel 2.6.18. Unfortunately, performance and scalability do not seem to have improved with these patches, with the SAP mprotect test program. I do not know why - maybe their program calls mprotect with really small areas so the batching does not do much? This is on a quad CPU Intel x86-64 system. -- All rights reversed. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Rik van Riel
2008-Jan-22 18:55 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On Mon, 21 Jan 2008 10:41:59 -0500 Rik van Riel <riel@redhat.com> wrote:> I tested a quick backport of these patches to XEN 3.1.2 and > kernel 2.6.18. > > Unfortunately, performance and scalability do not seem to > have improved with these patches,OK, it turned out the guys with the SAP mprotect benchmark program made the mistake of only upgrading the dom0 kernel and not the guest. With both dom0 and guest, mprotect performance has improved slightly on our dual cpu, dual core (4 cores total) Intel x86-64 test system: CPULOAD WITH STREAMS PATCH STANDARD 1 13108.77 11188.68 2 13135.63 11146.06 3 10522.82 9910.10 4 8622.47 9399.66 6 6571.49 7078.12 8 5141.23 5389.15 10 4514.37 4372.87 12 4226.79 3802.98 So we have about a 10-15% performance improvement at some numbers of streams, and a performance decrease at some other numbers of streams. Over-all performance impact of the patch seems to be about even :( I wonder if this is just an artifact of the system we tested this on, or if it can be seen across multiple systems. Bruce, what kinds of systems have you tested the patch on and what kind of performance numbers are you seeing? -- All Rights Reversed _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Bruce Rogers
2008-Jan-22 21:51 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
Your results are quite different from what I''ve seen. But then again I''ve been doing slightly different scaling tests, and of course the hardware is different. In the original problem report we received from SAP, the # of cpus was varied while running their test which runs 3 cpuload streams on each cpu. I''ve continued down that path since it shows a steady degradation as more cpus are added. I''ve not played with varying the number of cpuload streams in my testing. Other than that, it sounds like we are running the same SAP test (cpu_load.sh). Here''s what I''ve seen (numbers are the throughput per cpu, test assigns 3 streams per cpu, test was run in Dom0, as it was in the SAP problem report): #cpus native with patch without patch 1 9434 8264 6095 2 9884 8227 4530 3 9790 8008 2962 4 9392 7531 1801 <intervening cpu counts not test, trend seemed obvious> 8 6507 5870 669 So as you can see the performance degradation I am seeing is quite marked, and the benefit of the patch quite consistent and obviously beneficial. The machine I used is a 8 processor (4 dual core AMD Opteron 8218 CPUs, ea. w/ 1MB L2 Cache) HP Proliant Server with 16GB memory. I do wish I had been able to do testing with more than 1 machine; perhaps that would have provided better insights into the performance problem. Out lab is in a state of flux right now as we''re moving floors and I hence probably won''t be able to do a comparative run on other hardware very soon. I believe SAP is doing some broad based testing of their own which may help identify how much the hardware has to do with the performance of this test. What results do you get with native (non-smp) on that same hardware? Have you done runs on other hardware and seen consistent results? - Bruce>>> On 1/22/2008 at 11:55 AM, in message<20080122135534.1225627e@cuia.boston.redhat.com>, Rik van Riel <riel@redhat.com> wrote:> On Mon, 21 Jan 2008 10:41:59 -0500 > Rik van Riel <riel@redhat.com> wrote: > >> I tested a quick backport of these patches to XEN 3.1.2 and >> kernel 2.6.18. >> >> Unfortunately, performance and scalability do not seem to >> have improved with these patches, > > OK, it turned out the guys with the SAP mprotect benchmark program > made the mistake of only upgrading the dom0 kernel and not the guest. > > With both dom0 and guest, mprotect performance has improved slightly > on our dual cpu, dual core (4 cores total) Intel x86-64 test system: > > CPULOAD WITH > STREAMS PATCH STANDARD > 1 13108.77 11188.68 > 2 13135.63 11146.06 > 3 10522.82 9910.10 > 4 8622.47 9399.66 > 6 6571.49 7078.12 > 8 5141.23 5389.15 > 10 4514.37 4372.87 > 12 4226.79 3802.98 > > So we have about a 10-15% performance improvement at some numbers of > streams, and a performance decrease at some other numbers of streams. > Over-all performance impact of the patch seems to be about even :( > > I wonder if this is just an artifact of the system we tested this on, > or if it can be seen across multiple systems. > > Bruce, what kinds of systems have you tested the patch on and what > kind of performance numbers are you seeing?_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hannes Kuehnemund
2008-Jan-28 17:53 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On 22.01.2008 22:51, Bruce Rogers wrote:> [...] > I believe SAP is doing some broad based testing of their own which may help identify how much the hardware has to do with the performance of this test.We are currently testing the patched kernels we got from Red Hat and Novell with several Intel Tigerton (Quad Socket, Quad Core, shared L2 Cache @ 2.93GHz) machines. I can''t say for sure, if we''ll have AMD too. It''ll take a week until I can provide exact results. Thanks, Hannes> What results do you get with native (non-smp) on that same hardware? Have you done runs on other hardware and seen consistent results? > > - Bruce > >>>> On 1/22/2008 at 11:55 AM, in message > <20080122135534.1225627e@cuia.boston.redhat.com>, Rik van Riel > <riel@redhat.com> wrote: >> On Mon, 21 Jan 2008 10:41:59 -0500 >> Rik van Riel <riel@redhat.com> wrote: >> >>> I tested a quick backport of these patches to XEN 3.1.2 and >>> kernel 2.6.18. >>> >>> Unfortunately, performance and scalability do not seem to >>> have improved with these patches, >> OK, it turned out the guys with the SAP mprotect benchmark program >> made the mistake of only upgrading the dom0 kernel and not the guest. >> >> With both dom0 and guest, mprotect performance has improved slightly >> on our dual cpu, dual core (4 cores total) Intel x86-64 test system: >> >> CPULOAD WITH >> STREAMS PATCH STANDARD >> 1 13108.77 11188.68 >> 2 13135.63 11146.06 >> 3 10522.82 9910.10 >> 4 8622.47 9399.66 >> 6 6571.49 7078.12 >> 8 5141.23 5389.15 >> 10 4514.37 4372.87 >> 12 4226.79 3802.98 >> >> So we have about a 10-15% performance improvement at some numbers of >> streams, and a performance decrease at some other numbers of streams. >> Over-all performance impact of the patch seems to be about even :( >> >> I wonder if this is just an artifact of the system we tested this on, >> or if it can be seen across multiple systems. >> >> Bruce, what kinds of systems have you tested the patch on and what >> kind of performance numbers are you seeing? > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >-- * Hannes Kuehnemund * SAP LinuxLab * SAP AG * Dietmar Hopp Allee 16 * 69190 Walldorf, Germany T +49 6227 7-40615 F +49 6227 78-34584 mailto: hannes.kuehnemund@sap.com http://www.sap.com Sitz der Gesellschaft/Registered Office: Walldorf, Germany Vorstand/SAP Executive Board: Henning Kagermann (Sprecher/CEO), Léo Apotheker (stellvertretender Sprecher/Deputy CEO), Werner Brandt, Claus Heinrich, Gerhard Oswald, Peter Zencke Vorsitzender des Aufsichtsrats/Chairperson of the SAP Supervisory Board: Hasso Plattner Registergericht/Commercial Register Mannheim No HRB 350269 Diese E-Mail kann Betriebs- oder Geschäftsgeheimnisse oder sonstige vertrauliche Informationen enthalten. Sollten Sie diese E-Mail irrtümlich erhalten haben, ist Ihnen eine Kenntnisnahme des Inhalts, eine Vervielfältigung oder Weitergabe der E-Mail ausdrücklich untersagt. Bitte benachrichtigen Sie uns und vernichten Sie die empfangene E-Mail. Vielen Dank. This e-mail may contain trade secrets or privileged, undisclosed, or otherwise confidential information. If you have received this e-mail in error, you are hereby notified that any review, copying, or distribution of it is strictly prohibited. Please inform us immediately and destroy the original transmittal. Thank you for your cooperation. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Rik van Riel
2008-Jan-28 20:00 UTC
Re: [Xen-devel] [PATCH] respin of mprotect performance patch
On Mon, 28 Jan 2008 18:53:41 +0100 Hannes Kuehnemund <hannes.kuehnemund@sap.com> wrote:> On 22.01.2008 22:51, Bruce Rogers wrote: > > [...] > > I believe SAP is doing some broad based testing of their own which > > may help identify how much the hardware has to do with the > > performance of this test. > > We are currently testing the patched kernels we got from Red Hat and > Novell with several Intel Tigerton (Quad Socket, Quad Core, shared L2 > Cache @ 2.93GHz) machines. I can''t say for sure, if we''ll have AMD > too. It''ll take a week until I can provide exact results.I would like to know exactly which of our patched kernels you are trying. The older versions on my people page had bad performance due to an unrelated bug. Version 2.6.18-69.el5.bz412731.3 and newer should work right. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel