| Message ID | alpine.LFD.2.02.1207141253490.32033@ionos (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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?
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; }