[GIT PULL] KVM fixes for 3.5-rc6

[GIT PULL] KVM fixes for 3.5-rc6

[Avi Kivity]

Linus, please pull a couple of KVM fixes from:

  git://git.kernel.org/pub/scm/virt/kvm/kvm.git master

One is an adjustment for an irq layer change that affected device
assignment, the other a one-liner ppc fix.

----------------------------------------------------------------

Alex Williamson (1):
      KVM: Fix device assignment threaded irq handler

Avi Kivity (1):
      Merge branch 'for-upstream-master' of git://github.com/agraf/linux-2.6

Benjamin Herrenschmidt (1):
      powerpc/kvm: Fix "PR" KVM implementation of H_CEDE

-- 
error compiling committee.c: too many arguments to function

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Linus Torvalds]

Missing diffstat. Please please *please* always make sure you have
diffstats, because I really want to know that what I'm pulling matches
what you *think* that I'm pulling. And the diffstat isn't just for me
- it hopefully really makes you look at the whole "this is what I'm
asking Linus to pull" thing too.

              Linus

On Thu, Jul 12, 2012 at 4:55 AM, Avi Kivity <avi@redhat.com> wrote:
> Linus, please pull a couple of KVM fixes from:
>
>   git://git.kernel.org/pub/scm/virt/kvm/kvm.git master
>
> One is an adjustment for an irq layer change that affected device
> assignment, the other a one-liner ppc fix.
>
> ----------------------------------------------------------------
>
> Alex Williamson (1):
>       KVM: Fix device assignment threaded irq handler
>
> Avi Kivity (1):
>       Merge branch 'for-upstream-master' of git://github.com/agraf/linux-2.6
>
> Benjamin Herrenschmidt (1):
>       powerpc/kvm: Fix "PR" KVM implementation of H_CEDE
>
> --
> error compiling committee.c: too many arguments to function
>

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Linus Torvalds]

On Fri, Jul 13, 2012 at 8:45 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Missing diffstat. Please please *please* always make sure you have
> diffstats, because I really want to know that what I'm pulling matches
> what you *think* that I'm pulling. And the diffstat isn't just for me
> - it hopefully really makes you look at the whole "this is what I'm
> asking Linus to pull" thing too.

Btw, I'm cc'ing the irq people here too, because since I went and
looked at the details of the low-level commits due to the lack of
diffstat verification, I noticed that your fix for the lack of
IRQF_ONESHOT thing was interestingly different from the other peoples
"just add IRQF_ONESHOT".

I didn't realize that IRQF_ONESHOT had the kind of extra overhead that
you'd have noticed. And I do think that your solution is a prime
example of why we should never *ever* do the "just assume the user
meant xyz" solutions in things like the irq layer. Because just
assuming IRQF_ONESHOT was clearly the suboptimal thing to do in this
case. So it just reinforces my point that we did the right thing by
just making it an error.

At the same time, I do wonder if maybe MSI + IRQF_ONESHOT couldn't be
improved. The fact that the KVM people think that the extra overhead
of IRQF_ONESHOT is a bad thing for MSI interrupts makes me wonder if
maybe this wouldn't be an area the irq layer couldn't be improved on.
Maybe the MSI+IRQF_ONESHOT case could be improved. Because MSI is kind
of fundamentally one-shot, since it's a message-based irq scheme.  So
maybe the extra overhead is unnecessary in general, not just in this
particular KVM case. Hmm?

Thomas, see the commentary of a76beb14123a ("KVM: Fix device
assignment threaded irq handler").

                  Linus

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Thomas Gleixner]

On Fri, 13 Jul 2012, Linus Torvalds wrote:

> On Fri, Jul 13, 2012 at 8:45 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> At the same time, I do wonder if maybe MSI + IRQF_ONESHOT couldn't be
> improved. The fact that the KVM people think that the extra overhead
> of IRQF_ONESHOT is a bad thing for MSI interrupts makes me wonder if
> maybe this wouldn't be an area the irq layer couldn't be improved on.
> Maybe the MSI+IRQF_ONESHOT case could be improved. Because MSI is kind
> of fundamentally one-shot, since it's a message-based irq scheme.  So
> maybe the extra overhead is unnecessary in general, not just in this
> particular KVM case. Hmm?
> 
> Thomas, see the commentary of a76beb14123a ("KVM: Fix device
> assignment threaded irq handler").

Groan.

We already discussed to let the irq chip (in this case MSI) tell the
core that it does not need the extra oneshot handling. That way the
code which requests an threaded irq with the NULL primary handler
works on both MSI and normal interrupts.

Untested patch below.

Thanks,

	tglx

-----
Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -3109,6 +3109,7 @@ static struct irq_chip msi_chip = {
 	.irq_set_affinity	= msi_set_affinity,
 #endif
 	.irq_retrigger		= ioapic_retrigger_irq,
+	.flags			= IRQCHIP_ONESHOT_SAFE,
 };
 
 static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -351,6 +351,7 @@ enum {
 	IRQCHIP_MASK_ON_SUSPEND		= (1 <<  2),
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
+	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -1004,35 +1004,48 @@ __setup_irq(unsigned int irq, struct irq
 	 */
 	if (new->flags & IRQF_ONESHOT) {
 		/*
-		 * Unlikely to have 32 resp 64 irqs sharing one line,
-		 * but who knows.
+		 * Drivers are often written to work w/o knowledge
+		 * about the underlying irq chip implementation, so a
+		 * request for a threaded irq without a primary hard
+		 * irq context handler requires the ONESHOT flag to be
+		 * set. Some irq chips like MSI based interrupts are
+		 * per se one shot safe. Check the chip flags, so we
+		 * can avoid the unmask dance at the end of the
+		 * threaded handler for those.
 		 */
-		if (thread_mask == ~0UL) {
-			ret = -EBUSY;
-			goto out_mask;
+		if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) {
+			new->flags &= ~IRQF_ONESHOT;
+		} else {
+			/*
+			 * Unlikely to have 32 resp 64 irqs sharing one line,
+			 * but who knows.
+			 */
+			if (thread_mask == ~0UL) {
+				ret = -EBUSY;
+				goto out_mask;
+			}
+			/*
+			 * The thread_mask for the action is or'ed to
+			 * desc->thread_active to indicate that the
+			 * IRQF_ONESHOT thread handler has been woken, but not
+			 * yet finished. The bit is cleared when a thread
+			 * completes. When all threads of a shared interrupt
+			 * line have completed desc->threads_active becomes
+			 * zero and the interrupt line is unmasked. See
+			 * handle.c:irq_wake_thread() for further information.
+			 *
+			 * If no thread is woken by primary (hard irq context)
+			 * interrupt handlers, then desc->threads_active is
+			 * also checked for zero to unmask the irq line in the
+			 * affected hard irq flow handlers
+			 * (handle_[fasteoi|level]_irq).
+			 *
+			 * The new action gets the first zero bit of
+			 * thread_mask assigned. See the loop above which or's
+			 * all existing action->thread_mask bits.
+			 */
+			new->thread_mask = 1 << ffz(thread_mask);
 		}
-		/*
-		 * The thread_mask for the action is or'ed to
-		 * desc->thread_active to indicate that the
-		 * IRQF_ONESHOT thread handler has been woken, but not
-		 * yet finished. The bit is cleared when a thread
-		 * completes. When all threads of a shared interrupt
-		 * line have completed desc->threads_active becomes
-		 * zero and the interrupt line is unmasked. See
-		 * handle.c:irq_wake_thread() for further information.
-		 *
-		 * If no thread is woken by primary (hard irq context)
-		 * interrupt handlers, then desc->threads_active is
-		 * also checked for zero to unmask the irq line in the
-		 * affected hard irq flow handlers
-		 * (handle_[fasteoi|level]_irq).
-		 *
-		 * The new action gets the first zero bit of
-		 * thread_mask assigned. See the loop above which or's
-		 * all existing action->thread_mask bits.
-		 */
-		new->thread_mask = 1 << ffz(thread_mask);
-
 	} else if (new->handler == irq_default_primary_handler) {
 		/*
 		 * The interrupt was requested with handler = NULL, so

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Linus Torvalds]

On Fri, Jul 13, 2012 at 11:28 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> We already discussed to let the irq chip (in this case MSI) tell the
> core that it does not need the extra oneshot handling. That way the
> code which requests an threaded irq with the NULL primary handler
> works on both MSI and normal interrupts.

So I  don't think your patch is quite right.

If you want to clear the IRQF_ONESHOT for MSI irq's (and other ones
where the interrupt controller is fundamentally ONESHOT), I think you
should do it a few lines higher up - *before* you check the "does the
IRQF_ONESHOT mask match other shared interrupts"?

Now, irq sharing presumably doesn't happen with MSI, but there's
nothing fundamentally wrong with message-based irq schemes that have
shared interrupt handlers.

I think. Hmm?

                  Linus

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Thomas Gleixner]

On Fri, 13 Jul 2012, Linus Torvalds wrote:

> On Fri, Jul 13, 2012 at 11:28 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > We already discussed to let the irq chip (in this case MSI) tell the
> > core that it does not need the extra oneshot handling. That way the
> > code which requests an threaded irq with the NULL primary handler
> > works on both MSI and normal interrupts.
> 
> So I  don't think your patch is quite right.
> 
> If you want to clear the IRQF_ONESHOT for MSI irq's (and other ones
> where the interrupt controller is fundamentally ONESHOT), I think you
> should do it a few lines higher up - *before* you check the "does the
> IRQF_ONESHOT mask match other shared interrupts"?
> 
> Now, irq sharing presumably doesn't happen with MSI, but there's
> nothing fundamentally wrong with message-based irq schemes that have
> shared interrupt handlers.
> 
> I think. Hmm?

Shared irqs are not supported by MSI, but yes, the check should be
done way up. Makes it less ugly as well :)

Thanks,

	tglx

Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
+++ linux-2.6/arch/x86/kernel/apic/io_apic.c
@@ -3109,6 +3109,7 @@ static struct irq_chip msi_chip = {
 	.irq_set_affinity	= msi_set_affinity,
 #endif
 	.irq_retrigger		= ioapic_retrigger_irq,
+	.flags			= IRQCHIP_ONESHOT_SAFE,
 };
 
 static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
Index: linux-2.6/include/linux/irq.h
===================================================================
--- linux-2.6.orig/include/linux/irq.h
+++ linux-2.6/include/linux/irq.h
@@ -351,6 +351,7 @@ enum {
 	IRQCHIP_MASK_ON_SUSPEND		= (1 <<  2),
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
+	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
Index: linux-2.6/kernel/irq/manage.c
===================================================================
--- linux-2.6.orig/kernel/irq/manage.c
+++ linux-2.6/kernel/irq/manage.c
@@ -960,6 +960,18 @@ __setup_irq(unsigned int irq, struct irq
 	}
 
 	/*
+	 * Drivers are often written to work w/o knowledge about the
+	 * underlying irq chip implementation, so a request for a
+	 * threaded irq without a primary hard irq context handler
+	 * requires the ONESHOT flag to be set. Some irq chips like
+	 * MSI based interrupts are per se one shot safe. Check the
+	 * chip flags, so we can avoid the unmask dance at the end of
+	 * the threaded handler for those.
+	 */
+	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
+		new->flags &= ~IRQF_ONESHOT;
+
+	/*
 	 * The following block of code has to be executed atomically
 	 */
 	raw_spin_lock_irqsave(&desc->lock, flags);
@@ -1033,7 +1045,8 @@ __setup_irq(unsigned int irq, struct irq
 		 */
 		new->thread_mask = 1 << ffz(thread_mask);
 
-	} else if (new->handler == irq_default_primary_handler) {
+	} else if (new->handler == irq_default_primary_handler &&
+		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
 		/*
 		 * The interrupt was requested with handler = NULL, so
 		 * we use the default primary handler for it. But it

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Thomas Gleixner]

On Fri, 13 Jul 2012, Thomas Gleixner wrote:
> On Fri, 13 Jul 2012, Linus Torvalds wrote:
> > On Fri, Jul 13, 2012 at 8:45 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > At the same time, I do wonder if maybe MSI + IRQF_ONESHOT couldn't be
> > improved. The fact that the KVM people think that the extra overhead
> > of IRQF_ONESHOT is a bad thing for MSI interrupts makes me wonder if
> > maybe this wouldn't be an area the irq layer couldn't be improved on.
> > Maybe the MSI+IRQF_ONESHOT case could be improved. Because MSI is kind
> > of fundamentally one-shot, since it's a message-based irq scheme.  So
> > maybe the extra overhead is unnecessary in general, not just in this
> > particular KVM case. Hmm?
> > 
> > Thomas, see the commentary of a76beb14123a ("KVM: Fix device
> > assignment threaded irq handler").
> 
> Groan.
> 
> We already discussed to let the irq chip (in this case MSI) tell the
> core that it does not need the extra oneshot handling. That way the
> code which requests an threaded irq with the NULL primary handler
> works on both MSI and normal interrupts.

That's the kind of stuff which makes me go berserk, and just for the
record: the most complaints I get for going berserk are coming from
the virt folks.

I really can't understand why the virt folks think they are
special and do not have to talk to core maintainers.

Back then when I was doing the big irq cleanup, virt crap stood out by
far with the most idiotic^Wcreative "workarounds". Of course nobody
complained about me being moronic enough to come up with generic
solutions for their problems.

Though especially that commit including its changelog proves once
again the ignorance and desinterest of the virt crowd in solving
problems which are not only relevant to themself.

I whish you'd just refused to pull that nonsense and instead made them
talk to those folks who had a proper solution in mind already.

In fact we could have solved that proper weeks ago, if only people
would have raised the issue.

I'm tired of that kind of crap, really.

    Thomas

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

On 2012-07-14 04:25, Thomas Gleixner wrote:
> On Fri, 13 Jul 2012, Thomas Gleixner wrote:
>> On Fri, 13 Jul 2012, Linus Torvalds wrote:
>>> On Fri, Jul 13, 2012 at 8:45 AM, Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>> At the same time, I do wonder if maybe MSI + IRQF_ONESHOT couldn't be
>>> improved. The fact that the KVM people think that the extra overhead
>>> of IRQF_ONESHOT is a bad thing for MSI interrupts makes me wonder if
>>> maybe this wouldn't be an area the irq layer couldn't be improved on.
>>> Maybe the MSI+IRQF_ONESHOT case could be improved. Because MSI is kind
>>> of fundamentally one-shot, since it's a message-based irq scheme.  So
>>> maybe the extra overhead is unnecessary in general, not just in this
>>> particular KVM case. Hmm?
>>>
>>> Thomas, see the commentary of a76beb14123a ("KVM: Fix device
>>> assignment threaded irq handler").
>>
>> Groan.
>>
>> We already discussed to let the irq chip (in this case MSI) tell the
>> core that it does not need the extra oneshot handling. That way the
>> code which requests an threaded irq with the NULL primary handler
>> works on both MSI and normal interrupts.
> 
> That's the kind of stuff which makes me go berserk, and just for the
> record: the most complaints I get for going berserk are coming from
> the virt folks.
> 
> I really can't understand why the virt folks think they are
> special and do not have to talk to core maintainers.
> 
> Back then when I was doing the big irq cleanup, virt crap stood out by
> far with the most idiotic^Wcreative "workarounds". Of course nobody
> complained about me being moronic enough to come up with generic
> solutions for their problems.
> 
> Though especially that commit including its changelog proves once
> again the ignorance and desinterest of the virt crowd in solving
> problems which are not only relevant to themself.
> 
> I whish you'd just refused to pull that nonsense and instead made them
> talk to those folks who had a proper solution in mind already.
> 
> In fact we could have solved that proper weeks ago, if only people
> would have raised the issue.

June 1: http://thread.gmane.org/gmane.linux.kernel/1306742

The proper solution for us will be conditional direct IRQ delivery
anyway [1], but those patches were not considered ready for 3.5.

This patch here is a workaround to unbreak devices assignment in 3.5
after the IRQ layer changes without regressing noticeable /wrt overhead.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/92249


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Thomas Gleixner]

On Sat, 14 Jul 2012, Jan Kiszka wrote:
> On 2012-07-14 04:25, Thomas Gleixner wrote:
> This patch here is a workaround to unbreak devices assignment in 3.5
> after the IRQ layer changes without regressing noticeable /wrt overhead.

Yeah, workaround and regression are the proper marketing buzzwords to
excuse mindless hackery.

It took me a minute to figure out that there is no reason at all to
use a threaded interrupt handler for MSI and MSIX.

If you folks are so concerned about performance and overhead then
someone familiar with that code should have done the obvious change
long before the oneshot modifications took place.

I can't be bothered to do a performance test myself, but I bet it's
making an order of magnitude more of a difference than the "oh so
noticeable" few cycles in irq_finalize_oneshot().

Thanks,

	tglx


Index: linux-2.6/virt/kvm/assigned-dev.c
===================================================================
--- linux-2.6.orig/virt/kvm/assigned-dev.c
+++ linux-2.6/virt/kvm/assigned-dev.c
@@ -105,7 +105,7 @@ static irqreturn_t kvm_assigned_dev_thre
 }
 
 #ifdef __KVM_HAVE_MSI
-static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
+static irqreturn_t kvm_assigned_dev_msi_handler(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
 
@@ -117,7 +117,7 @@ static irqreturn_t kvm_assigned_dev_thre
 #endif
 
 #ifdef __KVM_HAVE_MSIX
-static irqreturn_t kvm_assigned_dev_thread_msix(int irq, void *dev_id)
+static irqreturn_t kvm_assigned_dev_msix_handler(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
 	int index = find_index_from_host_irq(assigned_dev, irq);
@@ -334,11 +334,6 @@ static int assigned_device_enable_host_i
 }
 
 #ifdef __KVM_HAVE_MSI
-static irqreturn_t kvm_assigned_dev_msi(int irq, void *dev_id)
-{
-	return IRQ_WAKE_THREAD;
-}
-
 static int assigned_device_enable_host_msi(struct kvm *kvm,
 					   struct kvm_assigned_dev_kernel *dev)
 {
@@ -351,9 +346,8 @@ static int assigned_device_enable_host_m
 	}
 
 	dev->host_irq = dev->dev->irq;
-	if (request_threaded_irq(dev->host_irq, kvm_assigned_dev_msi,
-				 kvm_assigned_dev_thread_msi, 0,
-				 dev->irq_name, dev)) {
+	if (request_irq(dev->host_irq, kvm_assigned_dev_msi_handler, 0,
+			dev->irq_name, dev)) {
 		pci_disable_msi(dev->dev);
 		return -EIO;
 	}
@@ -363,11 +357,6 @@ static int assigned_device_enable_host_m
 #endif
 
 #ifdef __KVM_HAVE_MSIX
-static irqreturn_t kvm_assigned_dev_msix(int irq, void *dev_id)
-{
-	return IRQ_WAKE_THREAD;
-}
-
 static int assigned_device_enable_host_msix(struct kvm *kvm,
 					    struct kvm_assigned_dev_kernel *dev)
 {
@@ -383,10 +372,9 @@ static int assigned_device_enable_host_m
 		return r;
 
 	for (i = 0; i < dev->entries_nr; i++) {
-		r = request_threaded_irq(dev->host_msix_entries[i].vector,
-					 kvm_assigned_dev_msix,
-					 kvm_assigned_dev_thread_msix,
-					 0, dev->irq_name, dev);
+		r = request_irq(dev->host_msix_entries[i].vector,
+				kvm_assigned_dev_msix_handler,
+				0, dev->irq_name, dev);
 		if (r)
 			goto err;
 	}

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

On 2012-07-14 13:16, Thomas Gleixner wrote:
> On Sat, 14 Jul 2012, Jan Kiszka wrote:
>> On 2012-07-14 04:25, Thomas Gleixner wrote:
>> This patch here is a workaround to unbreak devices assignment in 3.5
>> after the IRQ layer changes without regressing noticeable /wrt overhead.
> 
> Yeah, workaround and regression are the proper marketing buzzwords to
> excuse mindless hackery.
> 
> It took me a minute to figure out that there is no reason at all to
> use a threaded interrupt handler for MSI and MSIX.

Thomas, we also explained to you in the cited thread that your simple
approach for this doesn't work as is. We will have a proper solution
soon, but it takes a bit more than a minute - at least us.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Thomas Gleixner]

On Sat, 14 Jul 2012, Jan Kiszka wrote:
> On 2012-07-14 13:16, Thomas Gleixner wrote:
> > On Sat, 14 Jul 2012, Jan Kiszka wrote:
> >> On 2012-07-14 04:25, Thomas Gleixner wrote:
> >> This patch here is a workaround to unbreak devices assignment in 3.5
> >> after the IRQ layer changes without regressing noticeable /wrt overhead.
> > 
> > Yeah, workaround and regression are the proper marketing buzzwords to
> > excuse mindless hackery.
> > 
> > It took me a minute to figure out that there is no reason at all to
> > use a threaded interrupt handler for MSI and MSIX.
> 
> Thomas, we also explained to you in the cited thread that your simple
> approach for this doesn't work as is. We will have a proper solution
> soon, but it takes a bit more than a minute - at least us.

And I explained to you in that very thread that the proper solution to
avoid the "overhead" of finalize_oneshot is exaclty the patch I sent
to Linus yesterday:

> The only way we can avoid that, is that we get a hint from the
> underlying irq chip/ handler setup with an extra flag to tell the
> core, that it's safe to avoid the ONESHOT/finalize magic.

So now it took a full month of ignorance to come up with the
mindboggling solution of working around the core change with a private
hack instead of sitting down and doing what was said to be the correct
solution.

And that's what seriously annoys me. Instead of doing it yourself or
at least politely poking me to get it done, stuff just gets hacked
into submission and sold as the "performance regression" saviour.

Of course you are free to ignore my advice, but that does not mean
that I take bullshit from you.

Thanks,

	tglx

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

On 2012-07-14 14:33, Thomas Gleixner wrote:
> On Sat, 14 Jul 2012, Jan Kiszka wrote:
>> On 2012-07-14 13:16, Thomas Gleixner wrote:
>>> On Sat, 14 Jul 2012, Jan Kiszka wrote:
>>>> On 2012-07-14 04:25, Thomas Gleixner wrote:
>>>> This patch here is a workaround to unbreak devices assignment in 3.5
>>>> after the IRQ layer changes without regressing noticeable /wrt overhead.
>>>
>>> Yeah, workaround and regression are the proper marketing buzzwords to
>>> excuse mindless hackery.
>>>
>>> It took me a minute to figure out that there is no reason at all to
>>> use a threaded interrupt handler for MSI and MSIX.
>>
>> Thomas, we also explained to you in the cited thread that your simple
>> approach for this doesn't work as is. We will have a proper solution
>> soon, but it takes a bit more than a minute - at least us.
> 
> And I explained to you in that very thread that the proper solution to
> avoid the "overhead" of finalize_oneshot is exaclty the patch I sent
> to Linus yesterday:
> 
>> The only way we can avoid that, is that we get a hint from the
>> underlying irq chip/ handler setup with an extra flag to tell the
>> core, that it's safe to avoid the ONESHOT/finalize magic.
> 
> So now it took a full month of ignorance to come up with the
> mindboggling solution of working around the core change with a private
> hack instead of sitting down and doing what was said to be the correct
> solution.

We sat down and tried to avoid the core problem of our use case: IRQ
threading. That we now have to fall back to something else is
unfortunate and was surely not planned.

However, if you push your patch for 3.5, I'm sure Avi will happily drop
the disliked workaround and replace it with ordinary IRQF_ONESHOT tagging.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [GIT PULL] KVM fixes for 3.5-rc6

[Avi Kivity]

On 07/14/2012 03:55 PM, Jan Kiszka wrote:

>>> The only way we can avoid that, is that we get a hint from the
>>> underlying irq chip/ handler setup with an extra flag to tell the
>>> core, that it's safe to avoid the ONESHOT/finalize magic.
>> 
>> So now it took a full month of ignorance to come up with the
>> mindboggling solution of working around the core change with a private
>> hack instead of sitting down and doing what was said to be the correct
>> solution.
> 
> We sat down and tried to avoid the core problem of our use case: IRQ
> threading. That we now have to fall back to something else is
> unfortunate and was surely not planned.
> 
> However, if you push your patch for 3.5, I'm sure Avi will happily drop
> the disliked workaround and replace it with ordinary IRQF_ONESHOT tagging.

Fine by me, of course, but is mucking around in the irq layer something
we want to do in -rc7?

-- 
error compiling committee.c: too many arguments to function

[tip:irq/urgent] genirq: Allow irq chips to mark themself oneshot safe

[tip-bot for Thomas Gleixner]

Commit-ID:  dc9b229a58dc0dfed34272ff26c6d5fd17c674e0
Gitweb:     http://git.kernel.org/tip/dc9b229a58dc0dfed34272ff26c6d5fd17c674e0
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Fri, 13 Jul 2012 19:29:45 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 25 Jul 2012 12:46:38 +0200

genirq: Allow irq chips to mark themself oneshot safe

Some interrupt chips like MSI are oneshot safe by implementation. For
those interrupts we can avoid the mask/unmask sequence for threaded
interrupt handlers.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/alpine.LFD.2.02.1207132056540.32033@ionos
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Jan Kiszka <jan.kiszka@web.de>
---
 include/linux/irq.h |    1 +
 kernel/irq/manage.c |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 553fb66..216b0ba 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -349,6 +349,7 @@ enum {
 	IRQCHIP_MASK_ON_SUSPEND		= (1 <<  2),
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
+	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 8c54823..2e326d1 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -960,6 +960,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	/*
+	 * Drivers are often written to work w/o knowledge about the
+	 * underlying irq chip implementation, so a request for a
+	 * threaded irq without a primary hard irq context handler
+	 * requires the ONESHOT flag to be set. Some irq chips like
+	 * MSI based interrupts are per se one shot safe. Check the
+	 * chip flags, so we can avoid the unmask dance at the end of
+	 * the threaded handler for those.
+	 */
+	if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
+		new->flags &= ~IRQF_ONESHOT;
+
+	/*
 	 * The following block of code has to be executed atomically
 	 */
 	raw_spin_lock_irqsave(&desc->lock, flags);
@@ -1033,7 +1045,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		 */
 		new->thread_mask = 1 << ffz(thread_mask);
 
-	} else if (new->handler == irq_default_primary_handler) {
+	} else if (new->handler == irq_default_primary_handler &&
+		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {
 		/*
 		 * The interrupt was requested with handler = NULL, so
 		 * we use the default primary handler for it. But it