Re: [PATCH 1/5] IOMMU: make page table population preemptible

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/12/2013 09:51, Jan Beulich wrote:
>>>> On 11.12.13 at 19:40, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 10/12/2013 15:45, Jan Beulich wrote:
>>> @@ -321,7 +342,32 @@ static int iommu_populate_page_table(str
>>>                  d, mfn_to_gmfn(d, page_to_mfn(page)), page_to_mfn(page),
>>>                  IOMMUF_readable|IOMMUF_writable);
>>>              if ( rc )
>>> +            {
>>> +                page_list_add(page, &d->page_list);
>>>                  break;
>>> +            }
>>> +        }
>>> +        page_list_add_tail(page, &d->arch.relmem_list);
>>> +        if ( !(++n & 0xff) && !page_list_empty(&d->page_list) &&
>> Why the forced restart here?  If nothing needs pre-empting, surely it is
>> better to continue?
>>
>> Or is this about equality on the pcidevs_lock ?
>>
>>> +             hypercall_preempt_check() )
> Did you overlook this part of the condition?

No, but I did mentally get the logic inverted when trying to work out
what was going on.

How about (++n > 0xff) ?

If we have already spent a while in this loop, and the
hypercall_preempt_check() doesn't flip to 1 until a few iterations after
n is congruent with 0x100, waiting for another 0x100 iterations before
checking again seems a little long.

>
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -323,7 +323,7 @@ struct domain
>>>  
>>>  #ifdef HAS_PASSTHROUGH
>>>      /* Does this guest need iommu mappings? */
>>> -    bool_t           need_iommu;
>>> +    s8               need_iommu;
>> I think this change from bool_t to s8 needs a comment explaining that -1
>> indicates "the iommu mappings are pending creation"
> Will do.
>
>> Is there any particular reason that -ERESTART is used when -EAGAIN is
>> the prevailing style for hypercall continuations?
> I meanwhile realized that using -EAGAIN was a mistake (iirc taken
> from certain domctl-s having passed this back up to the caller to
> request re-invocation a long time ago) - -EAGAIN really has a
> different meaning, and hence we ought to switch all its current
> mis-uses to -ERESTART.
>
> Jan
>

Ok.

~Andrew