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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 9/23/2016 2:06 AM, George Dunlap wrote:
> On Tue, Sep 20, 2016 at 3:57 AM, Yu Zhang <yu.c.zhang@linux.intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel