Re: [Xen-devel] [PATCH RFC 01/12] x86/paging: introduce paging_set_allocation

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Wed, Aug 3, 2016 at 4:37 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 03.08.16 at 17:28, <george.dunlap@xxxxxxxxxx> wrote:
>> On 03/08/16 16:25, Jan Beulich wrote:
>>>>>> On 03.08.16 at 17:11, <George.Dunlap@xxxxxxxxxxxxx> wrote:
>>>> On Tue, Aug 2, 2016 at 5:12 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>>> On 02.08.16 at 17:49, <roger.pau@xxxxxxxxxx> wrote:
>>>>>> On Tue, Aug 02, 2016 at 11:47:22AM +0200, Roger Pau Monne wrote:
>>>>>>> On Fri, Jul 29, 2016 at 05:47:24PM +0100, Andrew Cooper wrote:
>>>>>>>> As this is for the construction of dom0, it would be better to take a
>>>>>>>> preemptible pointer, loop in construct_dom0(), with a
>>>>>>>> process_pending_softirqs() call in between.
>>>>>>>
>>>>>>> Now fixed.
>>>>>>
>>>>>> Hm, I have to stand corrected, using hypercall_preempt_check (as
>>>>>> any of the *_set_allocation function use), is not safe at this point:
>>>>>>
>>>>>> (XEN) ----[ Xen-4.8-unstable  x86_64  debug=y  Tainted:    C  ]----
>>>>>> (XEN) CPU:    0
>>>>>> (XEN) RIP:    e008:[<ffff82d08022fd47>]
>>>> hap.c#local_events_need_delivery+0x27/0x40
>>>>>> (XEN) RFLAGS: 0000000000010246   CONTEXT: hypervisor
>>>>>> (XEN) rax: 0000000000000000   rbx: ffff83023f5a5000   rcx: 
>>>>>> ffff82d080312900
>>>>>> (XEN) rdx: 0000000000000001   rsi: ffff83023f5a56c8   rdi: 
>>>>>> ffff8300b213d000
>>>>>> (XEN) rbp: ffff82d080307cc8   rsp: ffff82d080307cc8   r8:  
>>>>>> 0180000000000000
>>>>>> (XEN) r9:  0000000000000000   r10: 0000000000247000   r11: 
>>>>>> ffff82d08029a5b0
>>>>>> (XEN) r12: 0000000000000011   r13: 00000000000023ac   r14: 
>>>>>> ffff82d080307d4c
>>>>>> (XEN) r15: ffff83023f5a56c8   cr0: 000000008005003b   cr4: 
>>>>>> 00000000001526e0
>>>>>> (XEN) cr3: 00000000b20fc000   cr2: 0000000000000000
>>>>>> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
>>>>>> (XEN) Xen code around <ffff82d08022fd47>
>>>> (hap.c#local_events_need_delivery+0x27/0x40):
>>>>>> (XEN)  0d ad fa ff 48 8b 47 08 <80> 38 00 74 09 80 78 01 00 0f 94 c0 eb 
>>>>>> 02 31
>>>> c0
>>>>>> (XEN) Xen stack trace from rsp=ffff82d080307cc8:
>>>>>> (XEN)    ffff82d080307d08 ffff82d08022fc47 0000000000000000 
>>>>>> ffff83023f5a5000
>>>>>> (XEN)    ffff83023f5a5648 0000000000000000 ffff82d080307d4c 
>>>>>> 0000000000002400
>>>>>> (XEN)    ffff82d080307d38 ffff82d08020008c 00000000000ffffd 
>>>>>> ffff8300b1efd000
>>>>>> (XEN)    ffff83023f5a5000 ffff82d080307d4c ffff82d080307d78 
>>>>>> ffff82d0802cad30
>>>>>> (XEN)    0000000000203000 ffff83023f5a5000 ffff82d0802bf860 
>>>>>> 0000000000000000
>>>>>> (XEN)    0000000000000001 ffff83000008bef0 ffff82d080307de8 
>>>>>> ffff82d0802c91e0
>>>>>> (XEN)    ffff82d080307de8 ffff82d080143900 ffff82d080307de8 
>>>>>> 0000000000000000
>>>>>> (XEN)    ffff83000008bf00 ffff82d0802eb480 ffff82d080307dc4 
>>>>>> ffff82d08028b1cd
>>>>>> (XEN)    ffff83000008bf00 0000000000000000 0000000000000001 
>>>>>> ffff83023f5a5000
>>>>>> (XEN)    ffff82d080307f08 ffff82d0802bf0c9 0000000000000000 
>>>>>> 0000000000000000
>>>>>> (XEN)    0000000000000000 ffff82d080307f18 ffff83000008bee0 
>>>>>> 0000000000000001
>>>>>> (XEN)    0000000000000001 0000000000000001 0000000000000000 
>>>>>> 0000000000100000
>>>>>> (XEN)    0000000000000001 0000000000247000 ffff83000008bef4 
>>>>>> 0000000000100000
>>>>>> (XEN)    ffff830100000000 0000000000247001 0000000000000014 
>>>>>> 0000000100000000
>>>>>> (XEN)    ffff8300ffffffec ffff83000008bef0 ffff82d0802e0640 
>>>>>> ffff83000008bfb0
>>>>>> (XEN)    0000000000000000 0000000000000000 0000000000000111 
>>>>>> 0000000800000000
>>>>>> (XEN)    000000010000006e 0000000000000003 00000000000002f8 
>>>>>> 0000000000000000
>>>>>> (XEN)    00000000ad5c0bd0 0000000000000000 0000000000000001 
>>>>>> 0000000000000008
>>>>>> (XEN)    0000000000000000 ffff82d080100073 0000000000000000 
>>>>>> 0000000000000000
>>>>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>>>>> 0000000000000000
>>>>>> (XEN) Xen call trace:
>>>>>> (XEN)    [<ffff82d08022fd47>] hap.c#local_events_need_delivery+0x27/0x40
>>>>>> (XEN)    [<ffff82d08022fc47>] hap_set_allocation+0x107/0x130
>>>>>> (XEN)    [<ffff82d08020008c>] paging_set_allocation+0x4c/0x80
>>>>>> (XEN)    [<ffff82d0802cad30>] domain_build.c#hvm_setup_p2m+0x70/0x1a0
>>>>>> (XEN)    [<ffff82d0802c91e0>] 
>>>>>> domain_build.c#construct_dom0_hvm+0x60/0x120
>>>>>> (XEN)    [<ffff82d0802bf0c9>] __start_xen+0x1ea9/0x23a0
>>>>>> (XEN)    [<ffff82d080100073>] __high_start+0x53/0x60
>>>>>> (XEN)
>>>>>> (XEN) Pagetable walk from 0000000000000000:
>>>>>
>>>>> Sadly you don't make clear what pointer it is that is NULL at that point.
>>>>
>>>> It sounds from what he says in the following paragraph like current is 
>>>> NULL.
>>>
>>> I don't recall us re-setting current to this late in the boot process.
>>> Even during early boot we set it to a bogus non-NULL value rather
>>> than NULL.
>>>
>>>>>> I've tried setting current to d->vcpu[0], but that just makes the call to
>>>>>> hypercall_preempt_check crash in some scheduler assert. In any case, I've
>>>>>> added the preempt parameter to the paging_set_allocation function, but I
>>>>>> don't plan to use it in the domain builder for the time being. Does that
>>>>>> sound right?
>>>>>
>>>>> Not really, new huge latency issues like this shouldn't be reintroduced;
>>>>> we've been fighting hard to get rid of those (and we still occasionally
>>>>> find some no-one had noticed before).
>>>>
>>>> You mean latency in processing softirqs?
>>>>
>>>> Maybe what we need to do is to make local_events_need_delivery() safe
>>>> to call at this point by having it return 0 if current is NULL rather
>>>> than crashing?
>>>
>>> That would have the same effect - no softirq processing, and hence
>>> possible time issues on huge systems.
>>
>> No, local_events_delivery() only checks to see if the current vcpu has
>> outstanding virtual interrupts.  The other half of
>> hypercall_preempt_check() checks for softirqs, which doesn't appear to
>> rely on having current available.
>
> Good point, but
> - current should nevertheless not be NULL (afaict at least),

Well then it's likely to be one of the vcpu fields that the idle
domain doesn't have.  I'd be willing to bet that v->vcpu_info is NULL
for the idle domain.

> - hypercall_preempt_check() is probably the wrong construct,
>   as we're no in a hypercall.
> The latter of course could be addressed by, as you did suggest,
> some refinement to one of the pieces it's being made up from,
> but I'm not sure that would be better than perhaps making its
> invocation conditional (with some better alternative in the "else"
> case) in hap_set_allocation(). Not the least because any
> adjustment to hypercall_preempt_check() itself would affect all
> other of its users.

Well at the moment any user that calls it with current == NULL (or
perhaps is_idle_vcpu(current)) will crash.  So the only cost will be
an extra check for all the other callers.

The only other option would be to make {hap,sh}_set_allocation() check
to see if is_idle_vcpu(current), and if so just call
softirq_pending(smp_processor_id()) instead.  Or make *another* macro,
boot_softirq_check_or_hypercall_preempt_check(), which will do the
check.  But both seem a bit excessive compared to just modifying
hypercall_preempt_check() to handle being called during boot.

 -George

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