[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.
On 9/23/2016 2:06 AM, George Dunlap wrote:
On Tue, Sep 20, 2016 at 3:57 AM, Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
Well, for the logic of p2m type recalculation, similarities between
p2m_ioreq_server
and other changeable types exceeds their differences. As to the special
cases, how
about we use a macro, i.e. p2m_is_ioreq?
That'd be better than the open coded check, but would still result
in (taking the above example)
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
? What I'd prefer is a predicate that can be applied here on its own,
without involving && or ||.
OK. I can think of 2 scenarios that p2m_ioreq_server needs special
handling:
1> In ept_get_entry()/recal_type(), the p2m types are supposed to return
as it is, instead of
changing to p2m_log_dirty. So we can use a macro or a inline function
like p2m_check_changeable(),
which combines the
p2m_is_changeable(ept_entry->sa_p2mt) &&
!p2m_is_ioreq(ept_entry->sa_p2mt) )
together.
2> In resolve_misconfig()/do_recalc(), the entry_count gets decremented.
We
do not need this new inline function, because they are in a separate
if() statement.
Is this OK? :)
Sounds reasonable. But please give George and others a chance to
voice their opinions before you go that route.
Jan
Hi George,
Any comments on this series? :)
Well regarding the question you and Jan have been discussing, of what
to call / how to do the checks for changeable-but-not-ioreq, I don't
really care very much. :-)
I'm sorry it's taking so long to look at this series -- the code
you're trying to modify is already a bit of a tangled mess, and I
think this patch has a ways to go before it's ready. I do think this
series is important, so I'll be coming back to it first thing Monday.
Regarding the pausing added in this patch -- you listed two reasons in
the first patch for the domain pausing. The first was detecting
read-modify-write and acting appropriately; I think that can be done
with the patch that I sent you.
The second was the deadlock due to grabbing locks in a different
order. I'm afraid having such a thing lying around, even if you've
avoided it for now by pausing the domain, is another major trap that
we should try to avoid laying for future developers if we can at all
help it. The normal thing to do here is to simply have a locking
discipline -- in this case, I don't think it would be too difficult to
work out a locking order that would avoid the deadlock in a more
robust way than pausing and unpausing the domain.
With those two handled, we shouldn't need to pause the domain any more.
Thank you, George. Hope we can find a more elegant approach. :-)
B.R.
Yu
Thanks for your work on this -- I'll get back to patch 4/4 next week.
Peace,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
