From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 1/5] IOMMU: make page table population
	preemptible
Date: Fri, 13 Dec 2013 12:18:27 +0000
Message-ID: <52AAFB13.7020205@citrix.com>
References: <52A744B7020000780010BEF1@nat28.tlf.novell.com>
	<52A74539020000780010BF1F@nat28.tlf.novell.com>
	<52A8B1A5.9040302@citrix.com>
	<52AAE6B6020000780010CE61@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Andrew.Cooper3@citrix.com>) id 1VrRhR-0004pr-DZ
	for xen-devel@lists.xenproject.org; Fri, 13 Dec 2013 12:18:33 +0000
In-Reply-To: <52AAE6B6020000780010CE61@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>, xen-devel <xen-devel@lists.xenproject.org>, Keir Fraser <keir@xen.org>, xiantao.zhang@intel.com, Tim Deegan <tim@xen.org>
List-Id: xen-devel@lists.xenproject.org

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