Re: [Xen-devel] [PATCH v3.1 03/15] xen/x86: allow calling {sh/hap}_set_allocation with the idle domain

[Tim Deegan <tim@xxxxxxx>]

At 10:34 -0600 on 31 Oct (1477910088), Jan Beulich wrote:
> >>> On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> wrote:
> > ... and using the "preempted" parameter. The solution relies on just calling
> > softirq_pending if the current domain is the idle domain. If such preemption
> > happens, the caller should then call process_pending_softirqs in order to
> > drain the pending softirqs, and then call {sh/hap}_set_allocation again to
> > continue with it's execution.
> > 
> > This allows us to call *_set_allocation() when building domain 0.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> > ---
> > Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> Cc: Tim
> 
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> > Changes since v2:
> >  - Fix commit message.
> > ---
> >  xen/arch/x86/mm/hap/hap.c       | 4 +++-
> >  xen/arch/x86/mm/shadow/common.c | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
> > index f099e94..0645521 100644
> > --- a/xen/arch/x86/mm/hap/hap.c
> > +++ b/xen/arch/x86/mm/hap/hap.c
> > @@ -379,7 +379,9 @@ hap_set_allocation(struct domain *d, unsigned int 
> > pages, 
> > int *preempted)
> >              break;
> >  
> >          /* Check to see if we need to yield and try again */
> > -        if ( preempted && hypercall_preempt_check() )
> > +        if ( preempted &&
> > +             (is_idle_vcpu(current) ? softirq_pending(smp_processor_id()) :
> > +                                      hypercall_preempt_check()) )

This is a bit clunky, and is open-coded twice.  Can the existing checks in
hypercall_preempt_check() be made safe to run on the idle vcpu?
If not, please make a wrapper to call here that DTRT on idle and
non-idle, e.g. something like:

 /*
  * For long-running operations that may be in hypercall context or on
  * the idle vcpu (e.g. during dom0 construction), check if there is
  * background work to be done that should interrupt this operation.
  */
 static inline bool general_preempt_check(void)
 {
     return unlikely(softirq_pending(smp_processor_id()) ||
                     (!is_idle_vcpu(current) && local_events_need_delivery()));
 }

If you're feeling keen, you could convert hypercall_preempt_check() to
an inline function and comment it too. :)

Apart from that, ack.

Cheers,

Tim.

> >          {
> >              *preempted = 1;
> >              return 0;
> > diff --git a/xen/arch/x86/mm/shadow/common.c 
> > b/xen/arch/x86/mm/shadow/common.c
> > index 065bdc7..b2e99c2 100644
> > --- a/xen/arch/x86/mm/shadow/common.c
> > +++ b/xen/arch/x86/mm/shadow/common.c
> > @@ -1679,7 +1679,9 @@ static int sh_set_allocation(struct domain *d,
> >              break;
> >  
> >          /* Check to see if we need to yield and try again */
> > -        if ( preempted && hypercall_preempt_check() )
> > +        if ( preempted &&
> > +             (is_idle_vcpu(current) ? softirq_pending(smp_processor_id()) :
> > +                                      hypercall_preempt_check()) )
> >          {
> >              *preempted = 1;
> >              return 0;
> > -- 
> > 2.7.4 (Apple Git-66)
> 
> 

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