Re: [Xen-devel] [PATCH v4 3/3] VT-d: Fix vt-d Device-TLB flush timeout issue.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.01.16 at 13:38, <quan.xu@xxxxxxxxx> wrote:
>>  On January 27, 2016 at 7:24pm, <JBeulich@xxxxxxxx> wrote:
>> >>> On 27.01.16 at 12:09, <quan.xu@xxxxxxxxx> wrote:
>> >>  On January 27, 2016 at 6:48am, <Tian, Kevin> wrote:
>> >> > From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> >> > Sent: Tuesday, January 26, 2016 11:53 PM
>> >
>> >
>> >> > Once again: Before getting started, please assess which route is
>> >> > going to be the better one. Remember that we had already discussed
>> >> > and put aside some form of deferring the hiding of devices, so if
>> >> > you come back with a patch doing that again, you'll have to be able
>> >> > to explain why the alternative(s) are worse.
>> >> >
>> >>
>> >> Quan, could you list pros/cons of those alternatives based on discussion 
>> >> so
>> far?
>> >> Then we can decide which way should be done before you go to actual
>> coding.
>> >> Earlier suggestion on hiding device immediately is under the
>> >> assumption that all locks have been held. If this part becomes too
>> >> complex, and you can explain clearly that deferring the hiding action
>> >> doesn't lead to any race condition, then people can see why you are
>> proposing defer again.
>> >
>> >
>> > The following are pros/cons of those alternatives. It is also why I
>> > propose defer again.
>> >
>> > -- --
>> > 1. Hiding the devices immediately
>> > Pros:
>> >      * it makes whatever changes are ASAP after the Device-TLB flush error.
>> >
>> > Cons:
>> >      * It may break the code path.
>> >      * It may lead to any race condition.
>> >      * Hiding the devices immediately is under the assumption that all 
> locks
>> have been held.
>> >       Different locking state is possible for different call trees. This 
> part
>> becomes too complex.
>> 
>> So you just repeat what you've already said before. "This part becomes too
>> complex" you say without any kind of proof, yet that's what we need to
>> understand whether the alternative of doing the locking correctly really is 
> this
>> bad (and I continue to not see why it would).
> 
> 
> Such as pcidevs_lock:
> 
> 1. as I mentioned, it is indeed different locking state is possible for 
> different call trees of flush Device-TLB. When Device-TLB flush is error, It 
> is 
> really challenge to judge whether to acquire the pcidevs_lock or not.
> 
>    For example,
>    *It is _not_under_ lock for the following call tree:
> $ flush_iotlb_qi()--- iommu_flush_iotlb_psi() -- __intel_iommu_iotlb_flush() 
> --intel_iommu_iotlb_flush() --iommu_iotlb_flush() 
> --xenmem_add_to_physmap()--do_memory_op() 
> 
>    *It is _under_ lock for the following call tree:
> $flush_iotlb_qi()--iommu_flush_iotlb_dsi()--domain_context_unmap_one()--domain_con
> text_unmap()--reassign_device_ownership()--deassign_device()-iommu_do_pci_domctl()
> 
> 2. if I try to acquire the pcidevs_lock for some _not_under_ lock call tree, 
> it makes things worse. As the pcidevs_lock is a big lock, then
>   Frequent memory modification may block the pci-device assign due to the 
> pcidevs_lock. if I try to split the pcidevs_lock into small locks.
>   It may takes a great deal of time to make it stable.

I don't understand this, namely in the context of my suggestion to
simply pass down a flag indicating whether the lock is being held
(and hence acquiring it only in the most narrow scope if not already
owning it).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel