linux-kernel - Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE handler

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <506540C7.9070105@linux.vnet.ibm.com>
Date:	Fri, 28 Sep 2012 11:46:39 +0530
From:	Raghavendra K T <raghavendra.kt@...ux.vnet.ibm.com>
To:	Jiannan Ouyang <ouyang@...pitt.edu>
CC:	Avi Kivity <avi@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
	Rik van Riel <riel@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
	Marcelo Tosatti <mtosatti@...hat.com>,
	Srikar <srikar@...ux.vnet.ibm.com>,
	"Nikunj A. Dadhania" <nikunj@...ux.vnet.ibm.com>,
	KVM <kvm@...r.kernel.org>, chegu vinod <chegu_vinod@...com>,
	"Andrew M. Theurer" <habanero@...ux.vnet.ibm.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Srivatsa Vaddagiri <srivatsa.vaddagiri@...il.com>,
	Gleb Natapov <gleb@...hat.com>
Subject: Re: [PATCH RFC 1/2] kvm: Handle undercommitted guest case in PLE
 handler

On 09/28/2012 02:37 AM, Jiannan Ouyang wrote:
>
>
> On Thu, Sep 27, 2012 at 4:50 AM, Avi Kivity <avi@...hat.com
> <mailto:avi@...hat.com>> wrote:
>
>     On 09/25/2012 04:43 PM, Jiannan Ouyang wrote:
>      > I've actually implemented this preempted_bitmap idea.
>
>     Interesting, please share the code if you can.
>
>      > However, I'm doing this to expose this information to the guest,
>     so the
>      > guest is able to know if the lock holder is preempted or not before
>      > spining. Right now, I'm doing experiment to show that this idea
>     works.
>      >
>      > I'm wondering what do you guys think of the relationship between the
>      > pv_ticketlock approach and PLE handler approach. Are we going to
>     adopt
>      > PLE instead of the pv ticketlock, and why?
>
>     Right now we're searching for the best solution.  The tradeoffs are more
>     or less:
>
>     PLE:
>     - works for unmodified / non-Linux guests
>     - works for all types of spins (e.g. smp_call_function*())
>     - utilizes an existing hardware interface (PAUSE instruction) so likely
>     more robust compared to a software interface
>
>     PV:
>     - has more information, so it can perform better
>
>     Given these tradeoffs, if we can get PLE to work for moderate amounts of
>     overcommit then I'll prefer it (even if it slightly underperforms PV).
>     If we are unable to make it work well, then we'll have to add PV.
>
>     --
>     error compiling committee.c: too many arguments to function
>
>
> FYI. The preempted_bitmap patch.
>
> I delete some unrelated code in the generated patch file and seems
> broken the patch file format... I hope anyone could teach me some
> solutions.
> However, it's pretty straight forward, four things: declaration,
> initialization, set and clear. I think you guys can figure it out easily!
>
> As Avi sugguested, you could check task state TASK_RUNNING in sched_out.
>
> Signed-off-by: Jiannan Ouyang <ouyang@...pitt.edu
> <mailto:ouyang@...pitt.edu>>
>
> diff --git a/arch/x86/include/asm/
>
>     paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
>     index 8613cbb..4fcb648 100644
>     --- a/arch/x86/include/asm/paravirt_types.h
>     +++ b/arch/x86/include/asm/paravirt_types.h
>     @@ -73,6 +73,16 @@ struct pv_info {
>              const char *name;
>       };

I suppose we need this in common place since s390 also should have this,
if we are using this information in vcpu_on_spin()..

>
>     +struct pv_sched_info {
>     +       unsigned long   sched_bitmap;

Thinking, whether we need something similar to cpumask here?
Only thing is we are representing guest (v)cpumask.

>     +} __attribute__((__packed__));
>     +
>       struct pv_init_ops {
>              /*
>               * Patch may replace one of the defined code sequences with
>     diff --git a/arch/x86/kernel/paravirt-spinlocks.c
>     b/arch/x86/kernel/paravirt-spinlocks.c
>     index 676b8c7..2242d22 100644
>     --- a/arch/x86/kernel/paravirt-spinlocks.c
>     +++ b/arch/x86/kernel/paravirt-spinlocks.c
>
>     +struct pv_sched_info pv_sched_info = {
>     +        .sched_bitmap = (unsigned long)-1,
>     +};
>     diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>     index 44ee712..3eb277e 100644
>     --- a/virt/kvm/kvm_main.c
>     +++ b/virt/kvm/kvm_main.c
>     @@ -494,6 +494,11 @@ static struct kvm *kvm_create_vm(unsigned long
>     type)
>              mutex_init(&kvm->slots_lock);
>              atomic_set(&kvm->users_count, 1);
>
>     +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>     +        kvm->pv_sched_info.sched_bitmap = (unsigned long)-1;
>     +#endif
>     +
>              r = kvm_init_mmu_notifier(kvm);
>              if (r)
>                      goto out_err;
>     @@ -2697,7 +2702,13 @@ struct kvm_vcpu
>     *preempt_notifier_to_vcpu(struct preempt_notifier *pn)
>       static void kvm_sched_in(struct preempt_notifier *pn, int cpu)
>       {
>              struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
>
>     +       set_bit(vcpu->vcpu_id, &vcpu->kvm->pv_sched_info.sched_bitmap);
>              kvm_arch_vcpu_load(vcpu, cpu);
>       }
>
>     @@ -2705,7 +2716,13 @@ static void kvm_sched_out(struct
>     preempt_notifier *pn,
>                                struct task_struct *next)
>       {
>              struct kvm_vcpu *vcpu = preempt_notifier_to_vcpu(pn);
>
>     +       clear_bit(vcpu->vcpu_id,
>     &vcpu->kvm->pv_sched_info.sched_bitmap);
>              kvm_arch_vcpu_put(vcpu);
>       }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/