Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1413641 > unrolled thread

Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Andy Lutomirski <luto@amacapital.net> - 2016-06-04 07:10 +0200
    Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Paolo Bonzini <pbonzini@redhat.com> - 2016-06-06 17:50 +0200
      Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Ingo Molnar <mingo@kernel.org> - 2016-06-08 14:20 +0200
        Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Paolo Bonzini <pbonzini@redhat.com> - 2016-06-08 14:40 +0200
          Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Ingo Molnar <mingo@kernel.org> - 2016-06-08 15:00 +0200
            Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Ingo Molnar <mingo@kernel.org> - 2016-06-08 15:50 +0200
            Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore Paolo Bonzini <pbonzini@redhat.com> - 2016-06-08 15:50 +0200

#1413641 — Re: [PATCH 1/2] x86/entry: Avoid interrupt flag save and restore

On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
> Thanks to all the work that was done by Andy Lutomirski and others,
> enter_from_user_mode and prepare_exit_to_usermode are now called only with
> interrupts disabled.  Let's provide them a version of user_enter/user_exit
> that skips saving and restoring the interrupt flag.

> +/* Called with interrupts disabled.  */
> +static inline void __user_enter(void)
> +{
> +       if (context_tracking_is_enabled())
> +               __context_tracking_enter(CONTEXT_USER);
> +
> +}

Would user_enter_irqs_off be a better name?

--Andy

[toc] | [next] | [standalone]

#1415231

On 04/06/2016 07:07, Andy Lutomirski wrote:
> On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>>
>> Thanks to all the work that was done by Andy Lutomirski and others,
>> enter_from_user_mode and prepare_exit_to_usermode are now called only with
>> interrupts disabled.  Let's provide them a version of user_enter/user_exit
>> that skips saving and restoring the interrupt flag.
> 
>> +/* Called with interrupts disabled.  */
>> +static inline void __user_enter(void)
>> +{
>> +       if (context_tracking_is_enabled())
>> +               __context_tracking_enter(CONTEXT_USER);
>> +
>> +}
> 
> Would user_enter_irqs_off be a better name?

I'm just mimicking __context_tracking_enter and vs.
context_tracking_enter.  So it is at least consistent with those functions.

The guest ones are not quite as consistent.  I can fix that later,
there's no reason also to have guest context tracking split between
include/linux/context_tracking.h and include/linux/kvm_host.h.

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1417347

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 04/06/2016 07:07, Andy Lutomirski wrote:
> > On May 30, 2016 5:30 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
> >>
> >> Thanks to all the work that was done by Andy Lutomirski and others,
> >> enter_from_user_mode and prepare_exit_to_usermode are now called only with
> >> interrupts disabled.  Let's provide them a version of user_enter/user_exit
> >> that skips saving and restoring the interrupt flag.
> > 
> >> +/* Called with interrupts disabled.  */
> >> +static inline void __user_enter(void)
> >> +{
> >> +       if (context_tracking_is_enabled())
> >> +               __context_tracking_enter(CONTEXT_USER);
> >> +
> >> +}
> > 
> > Would user_enter_irqs_off be a better name?
> 
> I'm just mimicking __context_tracking_enter and vs.
> context_tracking_enter.  So it is at least consistent with those functions.
> 
> The guest ones are not quite as consistent.  I can fix that later,
> there's no reason also to have guest context tracking split between
> include/linux/context_tracking.h and include/linux/kvm_host.h.

Could we please first do the cleanups before complicating the code and applying 
more substantial changes?

Doing cleanups first makes it easier to review the substantial patches as well, so 
it's a win-win.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1417370

On 08/06/2016 14:16, Ingo Molnar wrote:
> > The guest ones are not quite as consistent.  I can fix that later,
> > there's no reason also to have guest context tracking split between
> > include/linux/context_tracking.h and include/linux/kvm_host.h.
>
> Could we please first do the cleanups before complicating the code and applying 
> more substantial changes?

The further cleanups wouldn't complicate the code.  It's just that
guest_enter/guest_exit require IRQs off but don't have __.

I'm thinking of something like this (untested):

diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
index d259274238db..c2dc581ddb0e 100644
--- a/include/linux/context_tracking.h
+++ b/include/linux/context_tracking.h
@@ -84,7 +84,7 @@ static inline void context_tracking_init(void) { }
 
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
-static inline void guest_enter(void)
+static inline void __guest_enter(void)
 {
 	if (vtime_accounting_cpu_enabled())
 		vtime_guest_enter(current);
@@ -93,9 +93,19 @@ static inline void guest_enter(void)
 
 	if (context_tracking_is_enabled())
 		__context_tracking_enter(CONTEXT_GUEST);
+
+	/* KVM does not hold any references to rcu protected data when it
+	 * switches CPU into a guest mode. In fact switching to a guest mode
+	 * is very similar to exiting to userspace from rcu point of view. In
+	 * addition CPU may stay in a guest mode for quite a long time (up to
+	 * one time slice). Lets treat guest mode as quiescent state, just like
+	 * we do with user-mode execution.
+	 */
+	if (!context_tracking_cpu_is_enabled())
+		rcu_virt_note_context_switch(smp_processor_id());
 }
 
-static inline void guest_exit(void)
+static inline void __guest_exit(void)
 {
 	if (context_tracking_is_enabled())
 		__context_tracking_exit(CONTEXT_GUEST);
@@ -107,7 +117,7 @@ static inline void guest_exit(void)
 }
 
 #else
-static inline void guest_enter(void)
+static inline void __guest_enter(void)
 {
 	/*
 	 * This is running in ioctl context so its safe
@@ -118,7 +128,7 @@ static inline void guest_enter(void)
 	current->flags |= PF_VCPU;
 }
 
-static inline void guest_exit(void)
+static inline void __guest_exit(void)
 {
 	/* Flush the guest cputime we spent on the guest */
 	vtime_account_system(current);
@@ -126,4 +136,23 @@ static inline void guest_exit(void)
 }
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */
 
+static inline void guest_enter(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__guest_enter();
+	local_irq_restore(flags);
+}
+
+/* must be called with irqs disabled */
+static inline void guest_exit(void)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	__guest_exit();
+	local_irq_restore(flags);
+}
+
 #endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5276fe0916fc..d00fdaa8da15 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -861,40 +861,23 @@ static inline void kvm_iommu_unmap_pages(struct kvm *kvm,
 /* must be called with irqs disabled */
 static inline void __kvm_guest_enter(void)
 {
-	guest_enter();
-	/* KVM does not hold any references to rcu protected data when it
-	 * switches CPU into a guest mode. In fact switching to a guest mode
-	 * is very similar to exiting to userspace from rcu point of view. In
-	 * addition CPU may stay in a guest mode for quite a long time (up to
-	 * one time slice). Lets treat guest mode as quiescent state, just like
-	 * we do with user-mode execution.
-	 */
-	if (!context_tracking_cpu_is_enabled())
-		rcu_virt_note_context_switch(smp_processor_id());
+	__guest_enter();
 }
 
 /* must be called with irqs disabled */
 static inline void __kvm_guest_exit(void)
 {
-	guest_exit();
+	__guest_exit();
 }
 
 static inline void kvm_guest_enter(void)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__kvm_guest_enter();
-	local_irq_restore(flags);
+	guest_enter();
 }
 
 static inline void kvm_guest_exit(void)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	__kvm_guest_exit();
-	local_irq_restore(flags);
+	guest_exit();
 }
 
 /*


and then removing the kvm_-prefixed functions.  It's little
more than code movement.

Paolo

[toc] | [prev] | [next] | [standalone]

#1417380

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 08/06/2016 14:16, Ingo Molnar wrote:
> > > The guest ones are not quite as consistent.  I can fix that later,
> > > there's no reason also to have guest context tracking split between
> > > include/linux/context_tracking.h and include/linux/kvm_host.h.
> >
> > Could we please first do the cleanups before complicating the code and applying 
> > more substantial changes?
> 
> The further cleanups wouldn't complicate the code.  It's just that
> guest_enter/guest_exit require IRQs off but don't have __.
> 
> I'm thinking of something like this (untested):
> 
> diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h
> index d259274238db..c2dc581ddb0e 100644
> --- a/include/linux/context_tracking.h
> +++ b/include/linux/context_tracking.h
> @@ -84,7 +84,7 @@ static inline void context_tracking_init(void) { }
>  
>  
>  #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
> -static inline void guest_enter(void)
> +static inline void __guest_enter(void)
>  {
>  	if (vtime_accounting_cpu_enabled())
>  		vtime_guest_enter(current);
> @@ -93,9 +93,19 @@ static inline void guest_enter(void)
>  
>  	if (context_tracking_is_enabled())
>  		__context_tracking_enter(CONTEXT_GUEST);
> +
> +	/* KVM does not hold any references to rcu protected data when it
> +	 * switches CPU into a guest mode. In fact switching to a guest mode

Nit, please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

> +	 * is very similar to exiting to userspace from rcu point of view. In

s/RCU

> +	 * addition CPU may stay in a guest mode for quite a long time (up to
> +	 * one time slice). Lets treat guest mode as quiescent state, just like
> +	 * we do with user-mode execution.
> +	 */
> +	if (!context_tracking_cpu_is_enabled())
> +		rcu_virt_note_context_switch(smp_processor_id());
>  }
>  
> -static inline void guest_exit(void)
> +static inline void __guest_exit(void)

> +static inline void guest_enter(void)
> +{
> +	unsigned long flags;
> +
> +	local_irq_save(flags);
> +	__guest_enter();
> +	local_irq_restore(flags);

So I believe it would be cleaner to name the irqs-off code paths explicitly: 
__guest_enter_irqsoff(), and propagate that naming into other parts as well?

>  /* must be called with irqs disabled */
>  static inline void __kvm_guest_exit(void)

This way all these random comments about irqs-off requirements would become 
unnecessary - the code becomes self-documenting.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1417444

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > So I believe it would be cleaner to name the irqs-off code paths explicitly:
> > __guest_enter_irqsoff(), and propagate that naming into other parts as well?
> 
> Ok, I'll send v2 with both the KVM cleanups and the entry optimizations.  It should
> be four patches putting all things together.  Thanks for the review!

Sounds good to me, thanks!

	Ingo

[toc] | [prev] | [next] | [standalone]

#1417449

> So I believe it would be cleaner to name the irqs-off code paths explicitly:
> __guest_enter_irqsoff(), and propagate that naming into other parts as well?

Ok, I'll send v2 with both the KVM cleanups and the entry optimizations.  It should
be four patches putting all things together.  Thanks for the review!

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web