Re: [Xen-devel] [PATCH v6 4/4] x86/ioreq server: Reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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.

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