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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.12.13 at 19:40, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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?

>> --- 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


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