andres at lagarcavilla
Apr 27, 2012, 2:08 PM
Post #41 of 45
> At 20:02 -0700 on 26 Apr (1335470547), Andres Lagar-Cavilla wrote:
>> > Can you please try the attached patch? I think you'll need this one
>> > plus the ones that take the locks out of the hpet code.
>> Right off the bat I'm getting a multitude of
>> (XEN) mm.c:3294:d0 Error while clearing mfn 100cbb7
>> And a hung dom0 during initramfs. I'm a little baffled as to why, but
>> there (32 bit dom0, XenServer6).
> Curses, I knew there'd be one somewhere. I've been replacing
> get_page_and_type_from_pagenr()s (which return 0 for success) with
> old-school get_page_type()s (which return 1 for success) and not always
> getting the right number of inversions. That's a horrible horrible
> beartrap of an API, BTW, which had me cursing at the screen, but I had
> enough on my plate yesterday without touching _that_ code too!
I now am quite pleased with the testing results on our end. I have a
four-patch series to top up your monster patch, which I'll submit shortly.
I encourage everyone interested to test this (obviously a lot of code is
touched). Including AMD, as I've expanded the code to touch svm too.
>> > Andres, this is basically the big-hammer version of your "take a
>> > pagecount" changes, plus the change you made to hvmemul_rep_movs().
>> > If this works I intend to follow it up with a patch to make some of
>> > read-modify-write paths avoid taking the lock (by using a
>> > compare-exchange operation so they only take the lock on a write). If
>> > that succeeds I might drop put_gfn() altogether.
>> You mean cmpxchg the whole p2m entry? I don't think I parse the plan.
>> There are code paths that do get_gfn_query -> p2m_change_type ->
>> But I guess those could lock the p2m up-front if they become the only
>> consumers of put_gfn left.
> Well, that's more or less what happens now. I was thinking of replacing
> any remaining
> (implicit) lock ; read ; think a bit ; maybe write ; unlock
> code with the fast-path-friendlier:
> read ; think ; maybe-cmpxchg (and on failure undo or retry
> which avoids taking the write lock altogether if there's no work to do.
> But maybe there aren't many of those left now. Obviously any path
> which will always write should just take the write-lock first.
After my four patches there aren't really any paths like the above left
(exhaustion disclaimer). I believe one or two iterative paths (like
HVMOP_set_mem_type) could be optimized to take the p2m lock up front,
instead of many get_gfn's.
The nice thing about get_gfn/put_gfn is that it will allow for seamless
(har har) transition to a fine-grained p2m. But then maybe we won't ever
need that with a p2m rwlock.
>> > - grant-table operations still use the lock, because frankly I
>> > could not follow the current code, and it's quite late in the
>> It's pretty complex with serious nesting, and ifdef's for arm and 32
>> gfn_to_mfn_private callers will suffer from altering the current
>> as put_gfn resolves to the right thing for the ifdef'ed arch. The other
>> user is grant_transfer which also relies on the page *not* having an
>> ref in steal_page. So it's a prime candidate to be left alone.
> Sadly, I think it's not. The PV backends will be doing lots of grant
> ops, which shouldn't get serialized against all other P2M lookups.
Those are addressed in my patch series now, which should case waves of panic.
>> > I also have a long list of uglinesses in the mm code that I found
>> Uh, ugly stuff, how could that have happened?
> I can't imagine. :) Certainly nothing to do with me thinking "I'll
> clean that up when I get some time."
>> I have a few preliminary observations on the patch. Pasting relevant
>> here, since the body of the patch seems to have been lost by the email
>> @@ -977,23 +976,25 @@ int arch_set_info_guest(
>> + if (!paging_mode_refcounts(d)
>> + && !get_page_and_type(cr3_page, d, PGT_l3_page_table) )
>> replace with && !get_page_type() )
>> @@ -2404,32 +2373,33 @@ static enum hvm_copy_result __hvm_copy(
>> gfn = addr >> PAGE_SHIFT;
>> - mfn = mfn_x(get_gfn_unshare(curr->domain, gfn, &p2mt));
>> + page = get_page_from_gfn(curr->domain, gfn, &p2mt,
>> replace with (flags & HVMCOPY_to_guest) ? P2M_UNSHARE : P2M_ALLOC (and
>> same logic when checking p2m_is_shared). Not truly related to your patch
>> bit since we're at it.
> OK, but not in this patch.
>> Same, further down
>> - if ( !p2m_is_ram(p2mt) )
>> + if ( !page )
>> - put_gfn(curr->domain, gfn);
>> + if ( page )
>> + put_page(page);
>> Last two lines are redundant
>> @@ -4019,35 +3993,16 @@ long do_hvm_op(unsigned long op, XEN_GUE
>> case HVMOP_modified_memory: a lot of error checking has been
> Yes, but it was bogus - there's a race between the actual modification
> and the call, during which anything might have happened. The best we
> can do is throw log-dirty bits at everything, and the caller can't do
> anything with the error anyway.
> When I come to tidy up I'll just add a new mark_gfn_dirty function
> and skip the pointless gfn->mfn->gfn translation on this path.
>> arch/x86/mm.c:do_mmu_update -> you blew up all the paging/sharing
>> for target gfns of mmu updates of l2/3/4 entries. It seems that this
>> wouldn't work anyways, that's why you killed it?
> Yeah - since only L1es can point at foreign mappings it was all just
> noise, and even if there had been real p2m lookups on those paths there
> was no equivalent to the translate-in-place that happens in
> mod_l1_entry so it would have been broken in a much worse way.
>> +++ b/xen/arch/x86/mm/hap/guest_walk.c
>> @@ -54,34 +54,37 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>> + if ( !top_page )
>> pfec &= ~PFEC_page_present;
>> - __put_gfn(p2m, top_gfn);
>> + put_page(top_page);
>> top_page is NULL here, remove put_page
>> get_page_from_gfn_p2m, slow path: no need for p2m_lock/unlock since
>> locking is already done by get_gfn_type_access/__put_gfn
> Yeah, but I was writing that with half an eye on killing that lock. :)
> I'll drop them for now.
>> (hope those observations made sense without inlining them in the actual
> Yes, absolutely - thanks for the review!
> If we can get this to work well enough I'll tidy it up into a sensible
> series next week. In the meantime, an updated verison of the
> monster patch is attached.
Xen-devel mailing list
Xen-devel [at] lists