Re: [PATCH RESEND 0/1] AHCI: Optimize interrupt processing
On Thu, Oct 03, 2013 at 04:06:51AM -0700, Christoph Hellwig wrote: On Sat, Jul 20, 2013 at 09:48:28AM -0500, Mike Christie wrote: What about the attached only compile tested patch. The patch has the mq block code work like the non mq code for bio cleanups. blk-mq: blk-mq should free bios in pass through case For non mq calls, the block layer will free the bios when blk_finish_request is called. e For mq calls, the blk mq code wants the caller to do this. This patch has the blk mq code work like the non mq code and has the block layer free the bios. Signed-off-by: Mike Christie micha...@cs.wisc.edu This patch breaks booting for me in the current blk multiqueue tree, with an apparent double free of a bio when using virtio-blk in writeback mode (cache=writeback or cache=none in qemu): I am not sure if the root cause the same, but the panic I experience with mounting a ahci device (and those double-tag usage described in another thread) is somehow similar: [ 181.184510] general protection fault: [#1] SMP [ 181.184546] Modules linked in: lockd sunrpc snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm mperf snd_page_alloc i5000_edac coretemp snd_timer edac_core iTCO_wdt snd kvm_intel iTCO_vendor_support lpc_ich mfd_core igb dca i5k_amb ppdev soundcore hp_wmi tg3 kvm sparse_keymap serio_raw ptp microcode pcspkr rfkill pps_core shpchp parport_pc parport mptsas scsi_transport_sas mptscsih mptbase floppy nouveau video mxm_wmi wmi i2c_algo_bit drm_kms_helper ttm drm i2c_core [ 181.184550] CPU: 6 PID: 0 Comm: swapper/6 Tainted: GW 3.10.0-rc5.debug+ #180 [ 181.184552] Hardware name: Hewlett-Packard HP xw6400 Workstation/0A04h, BIOS 786D4 v02.31 03/14/2008 [ 181.184554] task: 88007b1a8000 ti: 88007b19c000 task.ti: 88007b19c000 [ 181.184563] RIP: 0010:[811fa97b] [811fa97b] bio_endio+0x1b/0x40 [ 181.184565] RSP: 0018:88007d203a28 EFLAGS: 00010002 [ 181.184567] RAX: 6b6b6b6b6b6b6b6b RBX: RCX: dead00200200 [ 181.184568] RDX: RSI: RDI: 880068e29200 [ 181.184570] RBP: 88007d203a28 R08: e8201240 R09: [ 181.184571] R10: R11: 0001 R12: 880074d86000 [ 181.184572] R13: 6b6b6b6b6b6b6b6b R14: 6b6b6b6b R15: 0001 [ 181.184575] FS: () GS:88007d20() knlGS: [ 181.184576] CS: 0010 DS: ES: CR0: 8005003b [ 181.184578] CR2: 7f8afac8c45c CR3: 5f431000 CR4: 07e0 [ 181.184580] DR0: DR1: DR2: [ 181.184581] DR3: DR6: 0ff0 DR7: 0400 [ 181.184582] Stack: [ 181.184587] 88007d203a78 813202aa 88007d203a98 0046 [ 181.184591] 0046 880072d08000 880074d860d8 [ 181.184594] 0001 88007d203a88 81320445 [ 181.184595] Call Trace: [ 181.184597] IRQ [ 181.184603] [813202aa] blk_mq_complete_request+0x5a/0x1d0 [ 181.184607] [81320445] __blk_mq_end_io+0x25/0x30 [ 181.184609] [81320535] blk_mq_end_io+0xe5/0xf0 [ 181.184613] [81319754] blk_flush_complete_seq+0xf4/0x360 [ 181.184616] [81319a4b] ? flush_end_io+0x4b/0x210 [ 181.184619] [81319b2a] flush_end_io+0x12a/0x210 [ 181.184622] [813202da] blk_mq_complete_request+0x8a/0x1d0 [ 181.184626] [8147b9fd] ? scsi_device_unbusy+0x9d/0xd0 [ 181.184629] [81320445] __blk_mq_end_io+0x25/0x30 [ 181.184632] [81320535] blk_mq_end_io+0xe5/0xf0 [ 181.184635] [8147cae5] scsi_mq_end_request+0x15/0x20 [ 181.184638] [8147bf20] scsi_io_completion+0xa0/0x650 [ 181.184643] [810bbc3d] ? trace_hardirqs_off+0xd/0x10 [ 181.184648] [814722f7] scsi_finish_command+0x87/0xe0 [ 181.184650] [8147bccf] scsi_softirq_done+0x13f/0x160 [ 181.184653] [8147cba5] scsi_mq_done+0x15/0x20 [ 181.184658] [81495a73] ata_scsi_qc_complete+0x63/0x470 [ 181.184661] [8148fad0] __ata_qc_complete+0x90/0x140 [ 181.184664] [8148fc1d] ata_qc_complete+0x9d/0x230 [ 181.184667] [8148fe51] ata_qc_complete_multiple+0xa1/0xe0 [ 181.184673] [814aa449] ahci_handle_port_interrupt+0x109/0x560 [ 181.184676] [814ab63f] ahci_port_intr+0x2f/0x40 [ 181.184678] [814ab6f1] ahci_interrupt+0xa1/0x100 [ 181.184683] [810ff7b5] handle_irq_event_percpu+0x75/0x3d0 [ 181.184686] [810ffb58] handle_irq_event+0x48/0x70 [ 181.184689] [81102d9e] ? handle_fasteoi_irq+0x1e/0x100 [ 181.184692] [81102dda] handle_fasteoi_irq+0x5a/0x100 [ 181.184696] [81004320] handle_irq+0x60/0x150 [ 181.184702] [816ff846] ? atomic_notifier_call_chain+0x16/0x20 [
Re: [PATCH RFC 54/77] ntb: Ensure number of MSIs on SNB is enough for the link interrupt
On Sat, Oct 05, 2013 at 11:43:04PM +0200, Alexander Gordeev wrote:
On Wed, Oct 02, 2013 at 05:48:05PM -0700, Jon Mason wrote:
On Wed, Oct 02, 2013 at 12:49:10PM +0200, Alexander Gordeev wrote:
Signed-off-by: Alexander Gordeev agord...@redhat.com
---
drivers/ntb/ntb_hw.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index de2062c..eccd5e5 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1066,7 +1066,7 @@ static int ntb_setup_msix(struct ntb_device *ndev)
/* On SNB, the link interrupt is always tied to 4th vector. If
* we can't get all 4, then we can't use MSI-X.
*/
- if (ndev-hw_type != BWD_HW) {
+ if ((rc SNB_MSIX_CNT) (ndev-hw_type != BWD_HW)) {
Nack, this check is unnecessary.
If SNB can do more than SNB_MSIX_CNT MSI-Xs then this check is needed
to enable less than maximum MSI-Xs in case the maximum was not allocated.
Otherwise SNB will fallback to single MSI instead of multiple MSI-Xs.
Per the comment in the code snippet above, If we can't get all 4,
then we can't use MSI-X. There is already a check to see if more
than 4 were acquired. So it's not possible to hit this. Even if it
was, don't use SNB_MSIX_CNT here (limits.msix_cnt is the preferred
variable). Also, the () are unnecessary.
Thanks,
Jon
--
Regards,
Alexander Gordeev
agord...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
Hey, guys. On Sun, Oct 06, 2013 at 09:10:30AM +0200, Alexander Gordeev wrote: On Sun, Oct 06, 2013 at 05:19:46PM +1100, Benjamin Herrenschmidt wrote: On Sun, 2013-10-06 at 08:02 +0200, Alexander Gordeev wrote: In fact, in the current design to address the quota race decently the drivers would have to protect the *loop* to prevent the quota change between a pci_enable_msix() returned a positive number and the the next call to pci_enable_msix() with that number. Is it doable? I am not advocating for the current design, simply saying that your proposal doesn't address this issue while Ben's does. Hmmm... yean, the race condition could be an issue as multiple msi allocation might fail even if the driver can and explicitly handle multiple allocation if the quota gets reduced inbetween. There is one major flaw in min-max approach - the generic MSI layer will have to take decisions on exact number of MSIs to request, not device drivers. The min-max approach would actually be pretty nice for the users which actually care about this. This will never work for all devices, because there might be specific requirements which are not covered by the min-max. That is what Ben described ...say, any even number within a certain range. Ben suggests to leave the existing loop scheme to cover such devices, which I think is not right. if it could work. What about introducing pci_lock_msi() and pci_unlock_msi() and let device drivers care about their ranges and specifics in race-safe manner? I do not call to introduce it right now (since it appears pSeries has not been hitting the race for years) just as a possible alternative to Ben's proposal. I don't think the same race condition would happen with the loop. The problem case is where multiple msi(x) allocation fails completely because the global limit went down before inquiry and allocation. In the loop based interface, it'd retry with the lower number. As long as the number of drivers which need this sort of adaptive allocation isn't too high and the common cases can be made simple, I don't think the complex part of interface is all that important. Maybe we can have reserve / cancel type interface or just keep the loop with more explicit function names (ie. try_enable or something like that). Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 05/77] PCI/MSI: Convert pci_msix_table_size() to a public interface
Hello, On Wed, Oct 02, 2013 at 12:48:21PM +0200, Alexander Gordeev wrote: Make pci_msix_table_size() to return a error code if the device does not support MSI-X. This update is needed to facilitate a forthcoming re-design MSI/MSI-X interrupts enabling pattern. Device drivers will use this interface to obtain maximum number of MSI-X interrupts the device supports and use that value in the following call to pci_enable_msix() interface. Signed-off-by: Alexander Gordeev agord...@redhat.com Hmmm... I probably missed something but why is this necessary? To discern between -EINVAL and -ENOSPC? If so, does that really matter? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 07/77] PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern
Hello,
On Wed, Oct 02, 2013 at 12:48:23PM +0200, Alexander Gordeev wrote:
+static int foo_driver_enable_msi(struct foo_adapter *adapter, int nvec)
+{
+ rc = pci_get_msi_cap(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ rc = pci_enable_msi_block(adapter-pdev, nvec);
+ return rc;
+}
If there are many which duplicate the above pattern, it'd probably be
worthwhile to provide a helper? It's usually a good idea to reduce
the amount of boilerplate code in drivers.
static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
{
+ rc = pci_msix_table_size(adapter-pdev);
+ if (rc 0)
+ return rc;
+
+ nvec = min(nvec, rc);
+ if (nvec FOO_DRIVER_MINIMUM_NVEC) {
+ return -ENOSPC;
+
+ for (i = 0; i nvec; i++)
+ adapter-msix_entries[i].entry = i;
+
+ rc = pci_enable_msix(adapter-pdev, adapter-msix_entries, nvec);
+ return rc;
}
Ditto.
@@ -975,7 +951,7 @@ int pci_enable_msix(struct pci_dev *dev, struct
msix_entry *entries, int nvec)
if (nr_entries 0)
return nr_entries;
if (nvec nr_entries)
- return nr_entries;
+ return -EINVAL;
/* Check for any invalid entries */
for (i = 0; i nvec; i++) {
If we do things this way, it breaks all drivers using this interface
until they're converted, right? Also, it probably isn't the best idea
to flip the behavior like this as this can go completely unnoticed (no
compiler warning or anything, the same function just behaves
differently). Maybe it'd be a better idea to introduce a simpler
interface that most can be converted to?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
Hello, Alexander. On Wed, Oct 02, 2013 at 12:48:16PM +0200, Alexander Gordeev wrote: Alexander Gordeev (77): PCI/MSI: Fix return value when populate_msi_sysfs() failed PCI/MSI/PPC: Fix wrong RTAS error code reporting PCI/MSI/s390: Fix single MSI only check PCI/MSI/s390: Remove superfluous check of MSI type PCI/MSI: Convert pci_msix_table_size() to a public interface PCI/MSI: Factor out pci_get_msi_cap() interface PCI/MSI: Re-design MSI/MSI-X interrupts enablement pattern PCI/MSI: Get rid of pci_enable_msi_block_auto() interface ahci: Update MSI/MSI-X interrupts enablement code ahci: Check MRSM bit when multiple MSIs enabled ... Whee that's a lot more than I expected. I was just scanning multiple msi users. Maybe we can stage the work in more manageable steps so that you don't have to go through massive conversion only to do it all over again afterwards and likewise people don't get bombarded on each iteration? Maybe we can first update pci / msi code proper, msi and then msix? Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 54/77] ntb: Ensure number of MSIs on SNB is enough for the link interrupt
On Mon, Oct 07, 2013 at 09:50:57AM -0700, Jon Mason wrote:
On Sat, Oct 05, 2013 at 11:43:04PM +0200, Alexander Gordeev wrote:
On Wed, Oct 02, 2013 at 05:48:05PM -0700, Jon Mason wrote:
On Wed, Oct 02, 2013 at 12:49:10PM +0200, Alexander Gordeev wrote:
Signed-off-by: Alexander Gordeev agord...@redhat.com
---
drivers/ntb/ntb_hw.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index de2062c..eccd5e5 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1066,7 +1066,7 @@ static int ntb_setup_msix(struct ntb_device *ndev)
/* On SNB, the link interrupt is always tied to 4th
vector. If
* we can't get all 4, then we can't use MSI-X.
*/
- if (ndev-hw_type != BWD_HW) {
+ if ((rc SNB_MSIX_CNT) (ndev-hw_type != BWD_HW)) {
Nack, this check is unnecessary.
If SNB can do more than SNB_MSIX_CNT MSI-Xs then this check is needed
to enable less than maximum MSI-Xs in case the maximum was not allocated.
Otherwise SNB will fallback to single MSI instead of multiple MSI-Xs.
Per the comment in the code snippet above, If we can't get all 4,
then we can't use MSI-X. There is already a check to see if more
than 4 were acquired. So it's not possible to hit this. Even if it
was, don't use SNB_MSIX_CNT here (limits.msix_cnt is the preferred
variable). Also, the () are unnecessary.
The changelog is definitely bogus. I meant here an improvement to the
existing scheme, not a conversion to the new one:
msix_entries = msix_table_size(val);
Getting i.e. 16 vectors here.
if (msix_entries ndev-limits.msix_cnt) {
rc = -EINVAL;
goto err;
}
Upper limit check i.e. succeeds.
[...]
rc = pci_enable_msix(pdev, ndev-msix_entries, msix_entries);
pci_enable_msix() does not success and returns i.e. 8 here, should retry.
if (rc 0)
goto err1;
if (rc 0) {
/* On SNB, the link interrupt is always tied to 4th vector. If
* we can't get all 4, then we can't use MSI-X.
*/
if (ndev-hw_type != BWD_HW) {
On SNB bail out here, although could have continue with 8 vectors.
Can only use SNB_MSIX_CNT here, since limits.msix_cnt is the upper limit.
rc = -EIO;
goto err1;
}
[...]
}
--
Regards,
Alexander Gordeev
agord...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] eisa: standardize on eisa_register_driver like similar bus registrations
From: Matthew Whitehead tedheads...@gmail.com
Date: Sat, 5 Oct 2013 22:35:58 -0400
The other buses (isa, pci, pnp, parport, usb, tty, etc) all use the convention
of ${BUSNAME}_register_driver. Rewrite the little remaining code that uses
EISA
to follow this convention for easier readability.
This affects the EISA bus, SCSI, and networking subsystems so only one should
ultimately merge the patch if it is accepted.
Signed-off-by: Matthew Whitehead tedheads...@gmail.com
I'm fine with someone else taking this, for networking parts:
Acked-by: David S. Miller da...@davemloft.net
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
On Mon, 2013-10-07 at 14:01 -0400, Tejun Heo wrote: I don't think the same race condition would happen with the loop. The problem case is where multiple msi(x) allocation fails completely because the global limit went down before inquiry and allocation. In the loop based interface, it'd retry with the lower number. As long as the number of drivers which need this sort of adaptive allocation isn't too high and the common cases can be made simple, I don't think the complex part of interface is all that important. Maybe we can have reserve / cancel type interface or just keep the loop with more explicit function names (ie. try_enable or something like that). I'm thinking a better API overall might just have been to request individual MSI-X one by one :-) We want to be able to request an MSI-X at runtime anyway ... if I want to dynamically add a queue to my network interface, I want it to be able to pop a new arbitrary MSI-X. And we don't want to lock drivers into contiguous MSI-X sets either. And for the cleanup ... well that's what the pcim functions are for, we can just make MSI-X variants. Ben. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 54/77] ntb: Ensure number of MSIs on SNB is enough for the link interrupt
On Mon, Oct 07, 2013 at 08:38:45PM +0200, Alexander Gordeev wrote:
On Mon, Oct 07, 2013 at 09:50:57AM -0700, Jon Mason wrote:
On Sat, Oct 05, 2013 at 11:43:04PM +0200, Alexander Gordeev wrote:
On Wed, Oct 02, 2013 at 05:48:05PM -0700, Jon Mason wrote:
On Wed, Oct 02, 2013 at 12:49:10PM +0200, Alexander Gordeev wrote:
Signed-off-by: Alexander Gordeev agord...@redhat.com
---
drivers/ntb/ntb_hw.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ntb/ntb_hw.c b/drivers/ntb/ntb_hw.c
index de2062c..eccd5e5 100644
--- a/drivers/ntb/ntb_hw.c
+++ b/drivers/ntb/ntb_hw.c
@@ -1066,7 +1066,7 @@ static int ntb_setup_msix(struct ntb_device
*ndev)
/* On SNB, the link interrupt is always tied to 4th
vector. If
* we can't get all 4, then we can't use MSI-X.
*/
- if (ndev-hw_type != BWD_HW) {
+ if ((rc SNB_MSIX_CNT) (ndev-hw_type != BWD_HW)) {
Nack, this check is unnecessary.
If SNB can do more than SNB_MSIX_CNT MSI-Xs then this check is needed
to enable less than maximum MSI-Xs in case the maximum was not allocated.
Otherwise SNB will fallback to single MSI instead of multiple MSI-Xs.
Per the comment in the code snippet above, If we can't get all 4,
then we can't use MSI-X. There is already a check to see if more
than 4 were acquired. So it's not possible to hit this. Even if it
was, don't use SNB_MSIX_CNT here (limits.msix_cnt is the preferred
variable). Also, the () are unnecessary.
The changelog is definitely bogus. I meant here an improvement to the
existing scheme, not a conversion to the new one:
msix_entries = msix_table_size(val);
Getting i.e. 16 vectors here.
if (msix_entries ndev-limits.msix_cnt) {
On SNB HW, limits.msix_cnt is set to SNB_MSIX_CNT (4)
http://lxr.free-electrons.com/source/drivers/ntb/ntb_hw.c#L558
rc = -EINVAL;
goto err;
}
Upper limit check i.e. succeeds.
[...]
rc = pci_enable_msix(pdev, ndev-msix_entries, msix_entries);
pci_enable_msix() does not success and returns i.e. 8 here, should retry.
Per the above, since our upper bound is 4. We will either have this
return 0 for all 4 or a number between 1 and 3 (or an error, but
that's not relevant to this discussion).
if (rc 0)
goto err1;
if (rc 0) {
/* On SNB, the link interrupt is always tied to 4th vector. If
* we can't get all 4, then we can't use MSI-X.
*/
if (ndev-hw_type != BWD_HW) {
On SNB bail out here, although could have continue with 8 vectors.
Can only use SNB_MSIX_CNT here, since limits.msix_cnt is the upper limit.
Since we can guarantee that rc is between 1 and 3 at this point (on
SNB HW), we should error out.
Thanks,
Jon
rc = -EIO;
goto err1;
}
[...]
}
--
Regards,
Alexander Gordeev
agord...@redhat.com
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
On Tue, 2013-10-08 at 07:10 +1100, Benjamin Herrenschmidt wrote: On Mon, 2013-10-07 at 14:01 -0400, Tejun Heo wrote: I don't think the same race condition would happen with the loop. The problem case is where multiple msi(x) allocation fails completely because the global limit went down before inquiry and allocation. In the loop based interface, it'd retry with the lower number. As long as the number of drivers which need this sort of adaptive allocation isn't too high and the common cases can be made simple, I don't think the complex part of interface is all that important. Maybe we can have reserve / cancel type interface or just keep the loop with more explicit function names (ie. try_enable or something like that). I'm thinking a better API overall might just have been to request individual MSI-X one by one :-) We want to be able to request an MSI-X at runtime anyway ... if I want to dynamically add a queue to my network interface, I want it to be able to pop a new arbitrary MSI-X. Yes, this would be very useful. And we don't want to lock drivers into contiguous MSI-X sets either. I don't think there's any such limitation now. The entries array passed to pci_enable_msix() specifies which MSI-X vectors the driver wants to enable. It's usually filled with 0..nvec-1 in order, but not always. And the IRQ numbers returned aren't usually contiguous either, on x86. Ben. And for the cleanup ... well that's what the pcim functions are for, we can just make MSI-X variants. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
On Sun, 2013-10-06 at 09:10 +0200, Alexander Gordeev wrote: On Sun, Oct 06, 2013 at 05:19:46PM +1100, Benjamin Herrenschmidt wrote: On Sun, 2013-10-06 at 08:02 +0200, Alexander Gordeev wrote: In fact, in the current design to address the quota race decently the drivers would have to protect the *loop* to prevent the quota change between a pci_enable_msix() returned a positive number and the the next call to pci_enable_msix() with that number. Is it doable? I am not advocating for the current design, simply saying that your proposal doesn't address this issue while Ben's does. There is one major flaw in min-max approach - the generic MSI layer will have to take decisions on exact number of MSIs to request, not device drivers. [... No, the min-max functions should be implemented using the same loop that drivers are expected to use now. Ben. -- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked. -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xcopy testing with ddpt
On Mon, 2013-10-07 at 06:03 +0200, Thomas Glanzmann wrote: Hello Doug, * Douglas Gilbert dgilb...@interlog.com [2013-10-07 00:58]: Great, another one working. (CC'ing Hannes) BTW list_id=0 has a special meaning in some context (buried deep in T10 documents: spc4r36j.pdf). That is probably why Hannes Reinecke defaulted that list_id to 1. I could understand the target XCOPY implementation only accepting one xcopy sequence at a time, but why restrict it to list_id=0 ? A question for NaB ... Nab, do you have any input for us? It was my original understanding that when OPERATING_PARAMETERS is reporting SNLID=1 (Supports No ListID), the initiator is expected to send EXTENDED_COPY parameter lists with ListID Usage 11b + ListID=0. Since we're ignoring the value of ListID for now anyways, I agree that it doesn't make much sense to fail for a non zero value here.. However, the main concern that made me add this check to begin with was the case with ListID Usage 00b + 10b, where the copy server is expected to keep a per I_T list of in-use ListIDs, and return CHECK_CONDITION + ILLEGAL REQUEST/OPERATION IN PROGRESS for a ListID for a copy sequence already in progress. Given that don't have this per I_T list that tracks ListIDs yet, it seemed wrong at the time to allow non zero ListIDs to be processed.. ;) Also, it's worth mentioning that the target XCOPY implementation does in fact support multiple copy sequences per device at a time, and there is currently no hard limit enforced for the number of copies, aside from the normal fabric dependent NodeACL queue_depth, et al. OPERATING PARAMETERS is currently reporting 1 for TOTAL CONCURRENT COPIES and MAXIMUM CONCURRENT COPIES, and I'll likely be adding a device attribute to control this depth, and enforce it's usage for v3.13 code. --nab -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xcopy testing with ddpt
On 07/10/2013 23:38, Nicholas A. Bellinger wrote:
On Mon, 2013-10-07 at 15:18 -0700, Nicholas A. Bellinger wrote:
On Mon, 2013-10-07 at 06:03 +0200, Thomas Glanzmann wrote:
Hello Doug,
* Douglas Gilbert dgilb...@interlog.com [2013-10-07 00:58]:
Great, another one working.
(CC'ing Hannes)
BTW list_id=0 has a special meaning in some context
(buried deep in T10 documents: spc4r36j.pdf). That is
probably why Hannes Reinecke defaulted that list_id to
1. I could understand the target XCOPY implementation
only accepting one xcopy sequence at a time, but why
restrict it to list_id=0 ? A question for NaB ...
Nab, do you have any input for us?
It was my original understanding that when OPERATING_PARAMETERS is
reporting SNLID=1 (Supports No ListID), the initiator is expected to
send EXTENDED_COPY parameter lists with ListID Usage 11b + ListID=0.
Since we're ignoring the value of ListID for now anyways, I agree that
it doesn't make much sense to fail for a non zero value here..
However, the main concern that made me add this check to begin with was
the case with ListID Usage 00b + 10b, where the copy server is expected
to keep a per I_T list of in-use ListIDs, and return CHECK_CONDITION +
ILLEGAL REQUEST/OPERATION IN PROGRESS for a ListID for a copy sequence
already in progress.
How about the following patch to allow non zero ListIDs, but only when
ListID Usage is set to 11b..?
diff --git a/drivers/target/target_core_xcopy.c
b/drivers/target/target_core_xcopy.c
index 6b9774c..3a3ea31 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -911,11 +911,12 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
}
list_id = p[0];
- if (list_id != 0x00) {
- pr_err(XCOPY with non zero list_id: 0x%02x\n, list_id);
+ list_id_usage = (p[1] 0x18);
+ if (list_id != 0x00 list_id_usage != 0x11) {
+ pr_err(XCOPY with non zero list_id: 0x%02x, and
list_id_usage:
+ 0x%02x\n, list_id, list_id_usage);
goto out;
}
- list_id_usage = (p[1] 0x18);
/*
* Determine TARGET DESCRIPTOR LIST LENGTH + SEGMENT DESCRIPTOR LIST
LENGTH
*/
AFAICT this should make ddpt happy, as it's already be setting ListID
Usage = 11b when it gets OPERATING PARAMETERS - HELD_DATA = 0.
0x11 != 11b (but == 11h)
If 0x18 is the correct mask I think you want to compare against 0x18,
otherwise you probably want to shift down by 3 bits and compare against
0x03 or 0b11...
HTH,
Chris
--
Chris Boot
bo...@bootc.net
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: xcopy testing with ddpt
On Tue, 2013-10-08 at 00:07 +0100, Chris Boot wrote:
On 07/10/2013 23:38, Nicholas A. Bellinger wrote:
On Mon, 2013-10-07 at 15:18 -0700, Nicholas A. Bellinger wrote:
On Mon, 2013-10-07 at 06:03 +0200, Thomas Glanzmann wrote:
Hello Doug,
* Douglas Gilbert dgilb...@interlog.com [2013-10-07 00:58]:
Great, another one working.
(CC'ing Hannes)
BTW list_id=0 has a special meaning in some context
(buried deep in T10 documents: spc4r36j.pdf). That is
probably why Hannes Reinecke defaulted that list_id to
1. I could understand the target XCOPY implementation
only accepting one xcopy sequence at a time, but why
restrict it to list_id=0 ? A question for NaB ...
Nab, do you have any input for us?
It was my original understanding that when OPERATING_PARAMETERS is
reporting SNLID=1 (Supports No ListID), the initiator is expected to
send EXTENDED_COPY parameter lists with ListID Usage 11b + ListID=0.
Since we're ignoring the value of ListID for now anyways, I agree that
it doesn't make much sense to fail for a non zero value here..
However, the main concern that made me add this check to begin with was
the case with ListID Usage 00b + 10b, where the copy server is expected
to keep a per I_T list of in-use ListIDs, and return CHECK_CONDITION +
ILLEGAL REQUEST/OPERATION IN PROGRESS for a ListID for a copy sequence
already in progress.
How about the following patch to allow non zero ListIDs, but only when
ListID Usage is set to 11b..?
diff --git a/drivers/target/target_core_xcopy.c
b/drivers/target/target_core_xcopy.c
index 6b9774c..3a3ea31 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -911,11 +911,12 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
}
list_id = p[0];
- if (list_id != 0x00) {
- pr_err(XCOPY with non zero list_id: 0x%02x\n, list_id);
+ list_id_usage = (p[1] 0x18);
+ if (list_id != 0x00 list_id_usage != 0x11) {
+ pr_err(XCOPY with non zero list_id: 0x%02x, and
list_id_usage:
+ 0x%02x\n, list_id, list_id_usage);
goto out;
}
- list_id_usage = (p[1] 0x18);
/*
* Determine TARGET DESCRIPTOR LIST LENGTH + SEGMENT DESCRIPTOR
LIST LENGTH
*/
AFAICT this should make ddpt happy, as it's already be setting ListID
Usage = 11b when it gets OPERATING PARAMETERS - HELD_DATA = 0.
0x11 != 11b (but == 11h)
If 0x18 is the correct mask I think you want to compare against 0x18,
otherwise you probably want to shift down by 3 bits and compare against
0x03 or 0b11...
Er, duh, yes.. Looking at what sg_xcopy and ddpt are doing here again,
they are in fact using list_id_usage=10b (0x02) by default, so enforcing
a check for 11b (0x03) is not going to work as originally expected..
How about the following to simply ignore the list_id..?
--nab
diff --git a/drivers/target/target_core_xcopy.c
b/drivers/target/target_core_xcopy.c
index 6b9774c..fe98555 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -911,11 +911,8 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
}
list_id = p[0];
- if (list_id != 0x00) {
- pr_err(XCOPY with non zero list_id: 0x%02x\n, list_id);
- goto out;
- }
- list_id_usage = (p[1] 0x18);
+ list_id_usage = (p[1] 0x18) 3;
+
/*
* Determine TARGET DESCRIPTOR LIST LENGTH + SEGMENT DESCRIPTOR LIST
LENGTH
*/
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC 00/77] Re-design MSI/MSI-X interrupts enablement pattern
On Wed, Oct 02, 2013 at 12:29:04PM +0200, Alexander Gordeev wrote:
This series is against next branch in Bjorn's repo:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
Currently pci_enable_msi_block() and pci_enable_msix() interfaces
return a error code in case of failure, 0 in case of success and a
positive value which indicates the number of MSI-X/MSI interrupts
that could have been allocated. The latter value should be passed
to a repeated call to the interfaces until a failure or success:
for (i = 0; i FOO_DRIVER_MAXIMUM_NVEC; i++)
adapter-msix_entries[i].entry = i;
while (nvec = FOO_DRIVER_MINIMUM_NVEC) {
rc = pci_enable_msix(adapter-pdev,
adapter-msix_entries, nvec);
if (rc 0)
nvec = rc;
else
return rc;
}
return -ENOSPC;
This technique proved to be confusing and error-prone. Vast share
of device drivers simply fail to follow the described guidelines.
To clarify Vast share of device drivers:
- 58 drivers call pci_enable_msix()
- 24 try a single allocation and then fallback to MSI/LSI
- 19 use the loop style allocation as above
- 14 try an allocation, and if it fails retry once
- 1 incorrectly continues when pci_enable_msix() returns 0
So 33 drivers ( 50%) successfully make use of the confusing and
error-prone return value.
Another 24 happily ignore it, which is also entirely fine.
And yes, one is buggy, so obviously the interface is too complex. Thanks
drivers/ntb/ntb_hw.c
cheers
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] eisa: standardize on eisa_register_driver like similar bus registrations
On Mon, 2013-10-07 at 16:02 -0400, David Miller wrote:
From: Matthew Whitehead tedheads...@gmail.com
Date: Sat, 5 Oct 2013 22:35:58 -0400
The other buses (isa, pci, pnp, parport, usb, tty, etc) all use the
convention
of ${BUSNAME}_register_driver. Rewrite the little remaining code that uses
EISA
to follow this convention for easier readability.
This affects the EISA bus, SCSI, and networking subsystems so only one
should
ultimately merge the patch if it is accepted.
Signed-off-by: Matthew Whitehead tedheads...@gmail.com
I'm fine with someone else taking this, for networking parts:
I don't really see much value in the rename, especially as eisa is
probably not long for this world. However, its a trivial rename, so
bundle it all up into a single patch and send it to Jiří Kosina
triv...@vger.kernel.org he'll take care of it.
James
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
