Re: [Xen-devel] [PATCH v4 3/4] VT-d PI: restrict the vcpu number on a given pcpu

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.07.17 at 08:48, <chao.gao@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -95,22 +95,91 @@ static DEFINE_PER_CPU(struct vmx_pi_blocking_vcpu, 
> vmx_pi_blocking);
>  uint8_t __read_mostly posted_intr_vector;
>  static uint8_t __read_mostly pi_wakeup_vector;
>  
> +/*
> + * Protect critical sections to avoid adding a blocked vcpu to a destroyed
> + * blocking list.
> + */
> +static DEFINE_SPINLOCK(remote_pbl_operation);

What is "pbl" supposed to stand for?

> +#define remote_pbl_operation_begin(flags)                   \
> +({                                                          \
> +    spin_lock_irqsave(&remote_pbl_operation, flags);        \
> +})
> +
> +#define remote_pbl_operation_done(flags)                    \
> +({                                                          \
> +    spin_unlock_irqrestore(&remote_pbl_operation, flags);   \
> +})

No need for the ({ }) here.

But then I don't understand what this is needed for in the first
place. If this is once again about CPU offlining, then I can only
repeat that such happens in stop_machine context. Otherwise
I'm afraid the comment ahead of this code section needs
adjustment, as I can't interpret it in another way.

> +/*
> + * By default, the local pcpu (means the one the vcpu is currently running 
> on)
> + * is chosen as the destination of wakeup interrupt. But if the vcpu number 
> of
> + * the pcpu exceeds a limit, another pcpu is chosen until we find a suitable
> + * one.
> + *
> + * Currently, choose (v_tot/p_tot) + K as the limit of vcpu count, where
> + * v_tot is the total number of hvm vcpus on the system, p_tot is the total
> + * number of pcpus in the system, and K is a fixed number. An experment on a
> + * skylake server which has 112 cpus and 64G memory shows the maximum time to
> + * wakeup a vcpu from a 128-entry blocking list takes about 22us, which is
> + * tolerable. So choose 128 as the fixed number K.
> + *
> + * This policy makes sure:
> + * 1) for common cases, the limit won't be reached and the local pcpu is used
> + * which is beneficial to performance (at least, avoid an IPI when unblocking
> + * vcpu).
> + * 2) for the worst case, the blocking list length scales with the vcpu count
> + * divided by the pcpu count.
> + */
> +#define PI_LIST_FIXED_NUM 128
> +#define PI_LIST_LIMIT     (atomic_read(&num_hvm_vcpus) / num_online_cpus() + 
> \
> +                           PI_LIST_FIXED_NUM)
> +static inline bool pi_over_limit(int cpu)

unsigned int

> +{
> +    return per_cpu(vmx_pi_blocking, cpu).counter > PI_LIST_LIMIT;
> +}
> +
>  static void vmx_vcpu_block(struct vcpu *v)
>  {
> -    unsigned long flags;
> -    unsigned int dest;
> +    unsigned long flags[2];

??? You almost never need two flags instances in a function.

> +    unsigned int dest, pi_cpu;
>      spinlock_t *old_lock;
> -    spinlock_t *pi_blocking_list_lock =
> -             &per_cpu(vmx_pi_blocking, v->processor).lock;
>      struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +    spinlock_t *pi_blocking_list_lock;
> +    bool in_remote_operation = false;
> +
> +    pi_cpu = v->processor;
> +
> +    if ( unlikely(pi_over_limit(pi_cpu)) )
> +    {
> +        remote_pbl_operation_begin(flags[0]);
> +        in_remote_operation = true;
> +        while (true)

Coding style (and "for ( ; ; )" is generally better anyway).

Jan


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