Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status
Hi Everyone, Due to lack of information, I think I'll go ahead and pick up a simple solution for this(i.e. the code I attached previously to set a flag event_driven in scsi_device to let sr skip events poll) and send a new series out for you to review. After all, I can't wait forever... Please feel free to let me know if you don't think I should send a new series with the above mentioned solution. Thanks a lot for your(all of you) kind help and comments. -Aaron On 12/11/2012 01:10 PM, Tejun Heo wrote: Hello, guys. On Mon, Dec 10, 2012 at 11:26:07AM +0800, Aaron Lu wrote: The problem here is there's no easy to reach genhd from libata (or the other way around) without going through sr. I think we're gonna have to have something in sr one way or the other. Can't we do that via an event? It's a bit clunky because we need the callback in the layer that sees the sdev, which is libata-scsi, we just need an analogue of ata_scsi_media_change_notify, but ignoring and allowing polling is essentially event driven as well, so it should all work. We'll need a listener in genhd, which might be trickier. I'm not really following what you mean. Can you please elaborate? One way or the other, doesn't the notification have to bubble up through SCSI? A colleague of mine reminded me that it's impolite to write something like this, and here is my apology. I didn't mean to be rude, I'm sorry for this if it made you feel uncomfortable. Oh, no, it's not rude at all. I was just being distracted w/ other stuff and lazy. Sorry about that. Thanks. -- 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: [GIT PULL] exofs: 3 changes to exofs osd
On Mon, 2012-12-17 at 18:53 +0200, Boaz Harrosh wrote: Hi Linus. Please pull the following changes since commit [ddffeb8c] Linux 3.7-rc1 They are available in the git repository at: git://git.open-osd.org/linux-open-osd.git for-linus for you to fetch changes up to [861d6660] exofs: don't leak io_state and pages on read error (2012-12-14 12:17:32 +0200) These are just 3 patches, the last two are bug fixes on the error paths in exofs. The important patch is the one to osd_uld which adds sysfs info to osd devices for use by user-mode clustering discovery software. I'm already sitting on this patch since before February this year, It is important for some of the big installation cluster systems, who's been compiling their own kernel just for that patch. I'm a bit perplexed by this. You got notice when it was added to the SCSI tree and now it's already upstream: commit 51976a8c85cec0c62e410bc38b8a11dbc690764d Author: Boaz Harrosh bharr...@panasas.com Date: Wed Oct 24 14:51:41 2012 -0700 [SCSI] osd_uld: Add osdname systemid sysfs at scsi_osd class But the authorship info differs ... it looks like you forgot to include the From: tag in your original patch send. 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
Re: [GIT PULL] exofs: 3 changes to exofs osd
On 12/18/2012 12:58 PM, James Bottomley wrote: On Mon, 2012-12-17 at 18:53 +0200, Boaz Harrosh wrote: Hi Linus. Please pull the following changes since commit [ddffeb8c] Linux 3.7-rc1 They are available in the git repository at: git://git.open-osd.org/linux-open-osd.git for-linus for you to fetch changes up to [861d6660] exofs: don't leak io_state and pages on read error (2012-12-14 12:17:32 +0200) These are just 3 patches, the last two are bug fixes on the error paths in exofs. The important patch is the one to osd_uld which adds sysfs info to osd devices for use by user-mode clustering discovery software. I'm already sitting on this patch since before February this year, It is important for some of the big installation cluster systems, who's been compiling their own kernel just for that patch. I'm a bit perplexed by this. You got notice when it was added to the SCSI tree and now it's already upstream: commit 51976a8c85cec0c62e410bc38b8a11dbc690764d Author: Boaz Harrosh bharr...@panasas.com Date: Wed Oct 24 14:51:41 2012 -0700 [SCSI] osd_uld: Add osdname systemid sysfs at scsi_osd class But the authorship info differs ... it looks like you forgot to include the From: tag in your original patch send. I'm so sorry, I completely goofed on this one. It's what happens when you are swamped with other work and are doing things without thinking. I totally forgot that I need to remove this patch. Both these patches where in linux-next for a long time. So I believe the merge will go just fine. Lets leave it like this, or I can rebase and remove it? James Thanks Boaz -- 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: [GIT PULL] exofs: 3 changes to exofs osd
On Tue, 2012-12-18 at 13:18 +0200, Boaz Harrosh wrote: On 12/18/2012 12:58 PM, James Bottomley wrote: On Mon, 2012-12-17 at 18:53 +0200, Boaz Harrosh wrote: Hi Linus. Please pull the following changes since commit [ddffeb8c] Linux 3.7-rc1 They are available in the git repository at: git://git.open-osd.org/linux-open-osd.git for-linus for you to fetch changes up to [861d6660] exofs: don't leak io_state and pages on read error (2012-12-14 12:17:32 +0200) These are just 3 patches, the last two are bug fixes on the error paths in exofs. The important patch is the one to osd_uld which adds sysfs info to osd devices for use by user-mode clustering discovery software. I'm already sitting on this patch since before February this year, It is important for some of the big installation cluster systems, who's been compiling their own kernel just for that patch. I'm a bit perplexed by this. You got notice when it was added to the SCSI tree and now it's already upstream: commit 51976a8c85cec0c62e410bc38b8a11dbc690764d Author: Boaz Harrosh bharr...@panasas.com Date: Wed Oct 24 14:51:41 2012 -0700 [SCSI] osd_uld: Add osdname systemid sysfs at scsi_osd class But the authorship info differs ... it looks like you forgot to include the From: tag in your original patch send. I'm so sorry, I completely goofed on this one. It's what happens when you are swamped with other work and are doing things without thinking. I totally forgot that I need to remove this patch. Both these patches where in linux-next for a long time. So I believe the merge will go just fine. Lets leave it like this, or I can rebase and remove it? If it merges OK, I'd just leave it as is. It wouldn't be anywhere close to the first time we've had the same patch via different trees. 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
Re: [GIT PULL] exofs: 3 changes to exofs osd
On 12/18/2012 01:28 PM, James Bottomley wrote: Both these patches where in linux-next for a long time. So I believe the merge will go just fine. Lets leave it like this, or I can rebase and remove it? If it merges OK, I'd just leave it as is. It wouldn't be anywhere close to the first time we've had the same patch via different trees. Thanks James, it will not happen again. I just had a look on linux-next of Dec 17 they both where merged in and I did not see any complains. James Boaz -- 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
[PATCH v2 5/5] virtio-scsi: introduce multiqueue support
This patch adds queue steering to virtio-scsi. When a target is sent
multiple requests, we always drive them to the same queue so that FIFO
processing order is kept. However, if a target was idle, we can choose
a queue arbitrarily. In this case the queue is chosen according to the
current VCPU, so the driver expects the number of request queues to be
equal to the number of VCPUs. This makes it easy and fast to select
the queue, and also lets the driver optimize the IRQ affinity for the
virtqueues (each virtqueue's affinity is set to the CPU that owns
the queue).
The speedup comes from improving cache locality and giving CPU affinity
to the virtqueues, which is why this scheme was selected. Assuming that
the thread that is sending requests to the device is I/O-bound, it is
likely to be sleeping at the time the ISR is executed, and thus executing
the ISR on the same processor that sent the requests is cheap.
However, the kernel will not execute the ISR on the best processor
unless you explicitly set the affinity. This is because in practice
you will have many such I/O-bound processes and thus many otherwise
idle processors. Then the kernel will execute the ISR on a random
processor, rather than the one that is sending requests to the device.
The alternative to per-CPU virtqueues is per-target virtqueues. To
achieve the same locality, we could dynamically choose the virtqueue's
affinity based on the CPU of the last task that sent a request. This
is less appealing because we do not set the affinity directly---we only
provide a hint to the irqbalanced running in userspace. Dynamically
changing the affinity only works if the userspace applies the hint
fast enough.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: improved comments and commit messages, added memory barriers
drivers/scsi/virtio_scsi.c | 234 +--
1 files changed, 201 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 4f6c6a3..ca9d29d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -26,6 +26,7 @@
#define VIRTIO_SCSI_MEMPOOL_SZ 64
#define VIRTIO_SCSI_EVENT_LEN 8
+#define VIRTIO_SCSI_VQ_BASE 2
/* Command queue element */
struct virtio_scsi_cmd {
@@ -57,24 +58,57 @@ struct virtio_scsi_vq {
struct virtqueue *vq;
};
-/* Per-target queue state */
+/*
+ * Per-target queue state.
+ *
+ * This struct holds the data needed by the queue steering policy. When a
+ * target is sent multiple requests, we need to drive them to the same queue so
+ * that FIFO processing order is kept. However, if a target was idle, we can
+ * choose a queue arbitrarily. In this case the queue is chosen according to
+ * the current VCPU, so the driver expects the number of request queues to be
+ * equal to the number of VCPUs. This makes it easy and fast to select the
+ * queue, and also lets the driver optimize the IRQ affinity for the virtqueues
+ * (each virtqueue's affinity is set to the CPU that owns the queue).
+ *
+ * An interesting effect of this policy is that only writes to req_vq need to
+ * take the tgt_lock. Read can be done outside the lock because:
+ *
+ * - writes of req_vq only occur when atomic_inc_return(tgt-reqs) returns 1.
+ * In that case, no other CPU is reading req_vq: even if they were in
+ * virtscsi_queuecommand_multi, they would be spinning on tgt_lock.
+ *
+ * - reads of req_vq only occur when the target is not idle (reqs != 0).
+ * A CPU that enters virtscsi_queuecommand_multi will not modify req_vq.
+ *
+ * Similarly, decrements of reqs are never concurrent with writes of req_vq.
+ * Thus they can happen outside the tgt_lock, provided of course we make reqs
+ * an atomic_t.
+ */
struct virtio_scsi_target_state {
- /* Never held at the same time as vq_lock. */
+ /* This spinlock ever held at the same time as vq_lock. */
spinlock_t tgt_lock;
+
+ /* Count of outstanding requests. */
+ atomic_t reqs;
+
+ /* Currently active virtqueue for requests sent to this target. */
+ struct virtio_scsi_vq *req_vq;
};
/* Driver instance state */
struct virtio_scsi {
struct virtio_device *vdev;
- struct virtio_scsi_vq ctrl_vq;
- struct virtio_scsi_vq event_vq;
- struct virtio_scsi_vq req_vq;
-
/* Get some buffers ready for event vq */
struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
struct virtio_scsi_target_state *tgt;
+
+ u32 num_queues;
+
+ struct virtio_scsi_vq ctrl_vq;
+ struct virtio_scsi_vq event_vq;
+ struct virtio_scsi_vq req_vqs[];
};
static struct kmem_cache *virtscsi_cmd_cache;
@@ -109,6 +143,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi
*vscsi, void *buf)
struct virtio_scsi_cmd *cmd = buf;
struct scsi_cmnd *sc = cmd-sc;
struct virtio_scsi_cmd_resp *resp = cmd-resp.cmd;
+ struct
[PATCH v2 2/5] virtio-scsi: use functions for piecewise composition of buffers
Using the new virtio_scsi_add_sg function lets us simplify the queueing
path. In particular, all data protected by the tgt_lock is just gone
(multiqueue will find a new use for the lock).
The speedup is relatively small (2-4%) but it is worthwhile because of
the code simplification---both in this patches and in the next ones.
Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: new
drivers/scsi/virtio_scsi.c | 94 +++
1 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 74ab67a..2b93b6e 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -59,11 +59,8 @@ struct virtio_scsi_vq {
/* Per-target queue state */
struct virtio_scsi_target_state {
- /* Protects sg. Lock hierarchy is tgt_lock - vq_lock. */
+ /* Never held at the same time as vq_lock. */
spinlock_t tgt_lock;
-
- /* For sglist construction when adding commands to the virtqueue. */
- struct scatterlist sg[];
};
/* Driver instance state */
@@ -351,57 +348,58 @@ static void virtscsi_event_done(struct virtqueue *vq)
spin_unlock_irqrestore(vscsi-event_vq.vq_lock, flags);
};
-static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
-struct scsi_data_buffer *sdb)
-{
- struct sg_table *table = sdb-table;
- struct scatterlist *sg_elem;
- unsigned int idx = *p_idx;
- int i;
-
- for_each_sg(table-sgl, sg_elem, table-nents, i)
- sg[idx++] = *sg_elem;
-
- *p_idx = idx;
-}
-
/**
- * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
+ * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
* @vscsi : virtio_scsi state
* @cmd: command structure
- * @out_num: number of read-only elements
- * @in_num : number of write-only elements
* @req_size : size of the request buffer
* @resp_size : size of the response buffer
- *
- * Called with tgt_lock held.
+ * @gfp: flags to use for memory allocations
*/
-static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
-struct virtio_scsi_cmd *cmd,
-unsigned *out_num, unsigned *in_num,
-size_t req_size, size_t resp_size)
+static int virtscsi_add_cmd(struct virtqueue *vq,
+ struct virtio_scsi_cmd *cmd,
+ size_t req_size, size_t resp_size, gfp_t gfp)
{
struct scsi_cmnd *sc = cmd-sc;
- struct scatterlist *sg = tgt-sg;
- unsigned int idx = 0;
+ struct scatterlist sg;
+ unsigned int count, count_sg;
+ struct sg_table *out, *in;
+ struct virtqueue_buf buf;
+ int ret;
+
+ out = in = NULL;
+
+ if (sc sc-sc_data_direction != DMA_NONE) {
+ if (sc-sc_data_direction != DMA_FROM_DEVICE)
+ out = scsi_out(sc)-table;
+ if (sc-sc_data_direction != DMA_TO_DEVICE)
+ in = scsi_in(sc)-table;
+ }
+
+ count_sg = 2 + (out ? 1 : 0) + (in ? 1 : 0);
+ count= 2 + (out ? out-nents : 0) + (in ? in-nents : 0);
+ ret = virtqueue_start_buf(vq, buf, cmd, count, count_sg, gfp);
+ if (ret 0)
+ return ret;
/* Request header. */
- sg_set_buf(sg[idx++], cmd-req, req_size);
+ sg_init_one(sg, cmd-req, req_size);
+ virtqueue_add_sg(buf, sg, 1, DMA_TO_DEVICE);
/* Data-out buffer. */
- if (sc sc-sc_data_direction != DMA_FROM_DEVICE)
- virtscsi_map_sgl(sg, idx, scsi_out(sc));
-
- *out_num = idx;
+ if (out)
+ virtqueue_add_sg(buf, out-sgl, out-nents, DMA_TO_DEVICE);
/* Response header. */
- sg_set_buf(sg[idx++], cmd-resp, resp_size);
+ sg_init_one(sg, cmd-resp, resp_size);
+ virtqueue_add_sg(buf, sg, 1, DMA_FROM_DEVICE);
/* Data-in buffer */
- if (sc sc-sc_data_direction != DMA_TO_DEVICE)
- virtscsi_map_sgl(sg, idx, scsi_in(sc));
+ if (in)
+ virtqueue_add_sg(buf, in-sgl, in-nents, DMA_FROM_DEVICE);
- *in_num = idx - *out_num;
+ virtqueue_end_buf(buf);
+ return 0;
}
static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
@@ -409,25 +407,20 @@ static int virtscsi_kick_cmd(struct
virtio_scsi_target_state *tgt,
struct virtio_scsi_cmd *cmd,
size_t req_size, size_t resp_size, gfp_t gfp)
{
- unsigned int out_num, in_num;
unsigned long flags;
- int err;
+ int ret;
bool needs_kick = false;
- spin_lock_irqsave(tgt-tgt_lock, flags);
- virtscsi_map_cmd(tgt, cmd, out_num, in_num, req_size, resp_size);
-
- spin_lock(vq-vq_lock);
-
[PATCH v2 1/5] virtio: add functions for piecewise addition of buffers
The virtqueue_add_buf function has two limitations:
1) it requires the caller to provide all the buffers in a single call;
2) it does not support chained scatterlists: the buffers must be
provided as an array of struct scatterlist;
Because of these limitations, virtio-scsi has to copy each request into
a scatterlist internal to the driver. It cannot just use the one that
was prepared by the upper SCSI layers.
This patch adds a different set of APIs for adding a buffer to a virtqueue.
The new API lets you pass the buffers piecewise, wrapping multiple calls
to virtqueue_add_sg between virtqueue_start_buf and virtqueue_end_buf.
virtio-scsi can then call virtqueue_add_sg 3/4 times: for the request
header, for the write buffer (if present), for the response header, and
finally for the read buffer (again if present). It saves the copying
and the related locking.
Note that this API is not needed in virtio-blk, because it does all the
work of the upper SCSI layers itself in the blk_map_rq_sg call. Then
it simply hands the resulting scatterlist to virtqueue_add_buf.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: new
drivers/virtio/virtio_ring.c | 205 ++
include/linux/virtio.h | 21
2 files changed, 226 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ffd7e7d..ccfa97c 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -394,6 +394,211 @@ static void detach_buf(struct vring_virtqueue *vq,
unsigned int head)
vq-vq.num_free++;
}
+/**
+ * virtqueue_start_buf - start building buffer for the other end
+ * @vq: the struct virtqueue we're talking about.
+ * @buf: a struct keeping the state of the buffer
+ * @data: the token identifying the buffer.
+ * @count: the number of buffers that will be added
+ * @count_sg: the number of sg lists that will be added
+ * @gfp: how to do memory allocations (if necessary).
+ *
+ * Caller must ensure we don't call this with other virtqueue operations
+ * at the same time (except where noted), and that a successful call is
+ * followed by one or more calls to virtqueue_add_sg, and finally a call
+ * to virtqueue_end_buf.
+ *
+ * Returns zero or a negative error (ie. ENOSPC).
+ */
+int virtqueue_start_buf(struct virtqueue *_vq,
+ struct virtqueue_buf *buf,
+ void *data,
+ unsigned int count,
+ unsigned int count_sg,
+ gfp_t gfp)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ struct vring_desc *desc = NULL;
+ int head;
+ int ret = -ENOMEM;
+
+ START_USE(vq);
+
+ BUG_ON(data == NULL);
+
+#ifdef DEBUG
+ {
+ ktime_t now = ktime_get();
+
+ /* No kick or get, with .1 second between? Warn. */
+ if (vq-last_add_time_valid)
+ WARN_ON(ktime_to_ms(ktime_sub(now, vq-last_add_time))
+100);
+ vq-last_add_time = now;
+ vq-last_add_time_valid = true;
+ }
+#endif
+
+ BUG_ON(count count_sg);
+ BUG_ON(count_sg == 0);
+
+ /* If the host supports indirect descriptor tables, and there is
+* no space for direct buffers or there are multi-item scatterlists,
+* go indirect.
+*/
+ head = vq-free_head;
+ if (vq-indirect (count count_sg || vq-vq.num_free count)) {
+ if (vq-vq.num_free == 0)
+ goto no_space;
+
+ desc = kmalloc(count * sizeof(struct vring_desc), gfp);
+ if (!desc)
+ goto error;
+
+ /* We're about to use a buffer */
+ vq-vq.num_free--;
+
+ /* Use a single buffer which doesn't continue */
+ vq-vring.desc[head].flags = VRING_DESC_F_INDIRECT;
+ vq-vring.desc[head].addr = virt_to_phys(desc);
+ vq-vring.desc[head].len = count * sizeof(struct vring_desc);
+
+ /* Update free pointer */
+ vq-free_head = vq-vring.desc[head].next;
+ }
+
+ /* Set token. */
+ vq-data[head] = data;
+
+ pr_debug(Started buffer head %i for %p\n, head, vq);
+
+ buf-vq = _vq;
+ buf-indirect = desc;
+ buf-tail = NULL;
+ buf-head = head;
+ return 0;
+
+no_space:
+ ret = -ENOSPC;
+error:
+ pr_debug(Can't add buf (%d) - count = %i, avail = %i\n,
+ret, count, vq-vq.num_free);
+ END_USE(vq);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(virtqueue_start_buf);
+
+/**
+ * virtqueue_add_sg - add sglist to buffer
+ * @buf: the struct that was passed to virtqueue_start_buf
+ * @sgl: the description of the buffer(s).
+ * @count: the number of items to process in sgl
+ * @dir: whether the sgl is read or written (DMA_TO_DEVICE/DMA_FROM_DEVICE
[PATCH v2 4/5] virtio-scsi: pass struct virtio_scsi to virtqueue completion function
This will be needed soon in order to retrieve the per-target
struct.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
drivers/scsi/virtio_scsi.c | 17 +
1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 4a3abaf..4f6c6a3 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -104,7 +104,7 @@ static void virtscsi_compute_resid(struct scsi_cmnd *sc,
u32 resid)
*
* Called with vq_lock held.
*/
-static void virtscsi_complete_cmd(void *buf)
+static void virtscsi_complete_cmd(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_cmd *cmd = buf;
struct scsi_cmnd *sc = cmd-sc;
@@ -165,7 +165,8 @@ static void virtscsi_complete_cmd(void *buf)
sc-scsi_done(sc);
}
-static void virtscsi_vq_done(struct virtqueue *vq, void (*fn)(void *buf))
+static void virtscsi_vq_done(struct virtio_scsi *vscsi, struct virtqueue *vq,
+void (*fn)(struct virtio_scsi *vscsi, void *buf))
{
void *buf;
unsigned int len;
@@ -173,7 +174,7 @@ static void virtscsi_vq_done(struct virtqueue *vq, void
(*fn)(void *buf))
do {
virtqueue_disable_cb(vq);
while ((buf = virtqueue_get_buf(vq, len)) != NULL)
- fn(buf);
+ fn(vscsi, buf);
} while (!virtqueue_enable_cb(vq));
}
@@ -184,11 +185,11 @@ static void virtscsi_req_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(vscsi-req_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_cmd);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_cmd);
spin_unlock_irqrestore(vscsi-req_vq.vq_lock, flags);
};
-static void virtscsi_complete_free(void *buf)
+static void virtscsi_complete_free(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_cmd *cmd = buf;
@@ -205,7 +206,7 @@ static void virtscsi_ctrl_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(vscsi-ctrl_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_free);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_free);
spin_unlock_irqrestore(vscsi-ctrl_vq.vq_lock, flags);
};
@@ -329,7 +330,7 @@ static void virtscsi_handle_event(struct work_struct *work)
virtscsi_kick_event(vscsi, event_node);
}
-static void virtscsi_complete_event(void *buf)
+static void virtscsi_complete_event(struct virtio_scsi *vscsi, void *buf)
{
struct virtio_scsi_event_node *event_node = buf;
@@ -344,7 +345,7 @@ static void virtscsi_event_done(struct virtqueue *vq)
unsigned long flags;
spin_lock_irqsave(vscsi-event_vq.vq_lock, flags);
- virtscsi_vq_done(vq, virtscsi_complete_event);
+ virtscsi_vq_done(vscsi, vq, virtscsi_complete_event);
spin_unlock_irqrestore(vscsi-event_vq.vq_lock, flags);
};
--
1.7.1
--
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
[PATCH v2 0/5] Multiqueue virtio-scsi, and API for piecewise buffer submission
Hi all, this series adds multiqueue support to the virtio-scsi driver, based on Jason Wang's work on virtio-net. It uses a simple queue steering algorithm that expects one queue per CPU. LUNs in the same target always use the same queue (so that commands are not reordered); queue switching occurs when the request being queued is the only one for the target. Also based on Jason's patches, the virtqueue affinity is set so that each CPU is associated to one virtqueue. I tested the patches with fio, using up to 32 virtio-scsi disks backed by tmpfs on the host. These numbers are with 1 LUN per target. FIO configuration - [global] rw=read bsrange=4k-64k ioengine=libaio direct=1 iodepth=4 loops=20 overall bandwidth (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 795 965 925 4 997 13761500 8 1136 21302060 161440 22692474 241408 21792436 321515 19782319 (These numbers for single-queue are with 4 VCPUs, but the impact of adding more VCPUs is very limited). avg bandwidth per LUN (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 397 482 462 4 249 344 375 8 142 266 257 16 90 141 154 24 5890 101 32 4761 72 Patch 1 adds a new API to add functions for piecewise addition for buffers, which enables various simplifications in virtio-scsi (patches 2-3) and a small performance improvement of 2-6%. Patches 4 and 5 add multiqueuing. I'm mostly looking for comments on the new API of patch 1 for inclusion into the 3.9 kernel. Thanks to Wao Ganlong for help rebasing and benchmarking these patches. Paolo Bonzini (5): virtio: add functions for piecewise addition of buffers virtio-scsi: use functions for piecewise composition of buffers virtio-scsi: redo allocation of target data virtio-scsi: pass struct virtio_scsi to virtqueue completion function virtio-scsi: introduce multiqueue support drivers/scsi/virtio_scsi.c | 374 +- drivers/virtio/virtio_ring.c | 205 include/linux/virtio.h | 21 +++ 3 files changed, 485 insertions(+), 115 deletions(-) -- 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 v2 1/5] virtio: add functions for piecewise addition of buffers
Some comments without arguing about whether the performance
benefit is worth it.
On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index cf8adb1..39d56c4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -7,6 +7,7 @@
#include linux/spinlock.h
#include linux/device.h
#include linux/mod_devicetable.h
+#include linux/dma-direction.h
#include linux/gfp.h
/**
@@ -40,6 +41,26 @@ int virtqueue_add_buf(struct virtqueue *vq,
void *data,
gfp_t gfp);
+struct virtqueue_buf {
+ struct virtqueue *vq;
+ struct vring_desc *indirect, *tail;
This is wrong: virtio.h does not include virito_ring.h,
and it shouldn't by design depend on it.
+ int head;
+};
+
Can't we track state internally to the virtqueue?
Exposing it seems to buy us nothing since you can't
call add_buf between start and end anyway.
+int virtqueue_start_buf(struct virtqueue *_vq,
+ struct virtqueue_buf *buf,
+ void *data,
+ unsigned int count,
+ unsigned int count_sg,
+ gfp_t gfp);
+
+void virtqueue_add_sg(struct virtqueue_buf *buf,
+ struct scatterlist sgl[],
+ unsigned int count,
+ enum dma_data_direction dir);
+
And idea: in practice virtio scsi seems to always call sg_init_one, no?
So how about we pass in void* or something and avoid using sg and count?
This would make it useful for -net BTW.
+void virtqueue_end_buf(struct virtqueue_buf *buf);
+
void virtqueue_kick(struct virtqueue *vq);
bool virtqueue_kick_prepare(struct virtqueue *vq);
--
1.7.1
--
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 v2 2/5] virtio-scsi: use functions for piecewise composition of buffers
On Tue, Dec 18, 2012 at 01:32:49PM +0100, Paolo Bonzini wrote:
Using the new virtio_scsi_add_sg function lets us simplify the queueing
path. In particular, all data protected by the tgt_lock is just gone
(multiqueue will find a new use for the lock).
vq access still needs some protection: virtio is not reentrant
by itself. with tgt_lock gone what protects vq against
concurrent add_buf calls?
The speedup is relatively small (2-4%) but it is worthwhile because of
the code simplification---both in this patches and in the next ones.
Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: new
drivers/scsi/virtio_scsi.c | 94 +++
1 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 74ab67a..2b93b6e 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -59,11 +59,8 @@ struct virtio_scsi_vq {
/* Per-target queue state */
struct virtio_scsi_target_state {
- /* Protects sg. Lock hierarchy is tgt_lock - vq_lock. */
+ /* Never held at the same time as vq_lock. */
spinlock_t tgt_lock;
-
- /* For sglist construction when adding commands to the virtqueue. */
- struct scatterlist sg[];
};
/* Driver instance state */
@@ -351,57 +348,58 @@ static void virtscsi_event_done(struct virtqueue *vq)
spin_unlock_irqrestore(vscsi-event_vq.vq_lock, flags);
};
-static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
- struct scsi_data_buffer *sdb)
-{
- struct sg_table *table = sdb-table;
- struct scatterlist *sg_elem;
- unsigned int idx = *p_idx;
- int i;
-
- for_each_sg(table-sgl, sg_elem, table-nents, i)
- sg[idx++] = *sg_elem;
-
- *p_idx = idx;
-}
-
/**
- * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
+ * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
* @vscsi: virtio_scsi state
* @cmd : command structure
- * @out_num : number of read-only elements
- * @in_num : number of write-only elements
* @req_size : size of the request buffer
* @resp_size: size of the response buffer
- *
- * Called with tgt_lock held.
+ * @gfp : flags to use for memory allocations
*/
-static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
- struct virtio_scsi_cmd *cmd,
- unsigned *out_num, unsigned *in_num,
- size_t req_size, size_t resp_size)
+static int virtscsi_add_cmd(struct virtqueue *vq,
+ struct virtio_scsi_cmd *cmd,
+ size_t req_size, size_t resp_size, gfp_t gfp)
{
struct scsi_cmnd *sc = cmd-sc;
- struct scatterlist *sg = tgt-sg;
- unsigned int idx = 0;
+ struct scatterlist sg;
+ unsigned int count, count_sg;
+ struct sg_table *out, *in;
+ struct virtqueue_buf buf;
+ int ret;
+
+ out = in = NULL;
+
+ if (sc sc-sc_data_direction != DMA_NONE) {
+ if (sc-sc_data_direction != DMA_FROM_DEVICE)
+ out = scsi_out(sc)-table;
+ if (sc-sc_data_direction != DMA_TO_DEVICE)
+ in = scsi_in(sc)-table;
+ }
+
+ count_sg = 2 + (out ? 1 : 0) + (in ? 1 : 0);
+ count= 2 + (out ? out-nents : 0) + (in ? in-nents : 0);
+ ret = virtqueue_start_buf(vq, buf, cmd, count, count_sg, gfp);
+ if (ret 0)
+ return ret;
/* Request header. */
- sg_set_buf(sg[idx++], cmd-req, req_size);
+ sg_init_one(sg, cmd-req, req_size);
+ virtqueue_add_sg(buf, sg, 1, DMA_TO_DEVICE);
/* Data-out buffer. */
- if (sc sc-sc_data_direction != DMA_FROM_DEVICE)
- virtscsi_map_sgl(sg, idx, scsi_out(sc));
-
- *out_num = idx;
+ if (out)
+ virtqueue_add_sg(buf, out-sgl, out-nents, DMA_TO_DEVICE);
/* Response header. */
- sg_set_buf(sg[idx++], cmd-resp, resp_size);
+ sg_init_one(sg, cmd-resp, resp_size);
+ virtqueue_add_sg(buf, sg, 1, DMA_FROM_DEVICE);
/* Data-in buffer */
- if (sc sc-sc_data_direction != DMA_TO_DEVICE)
- virtscsi_map_sgl(sg, idx, scsi_in(sc));
+ if (in)
+ virtqueue_add_sg(buf, in-sgl, in-nents, DMA_FROM_DEVICE);
- *in_num = idx - *out_num;
+ virtqueue_end_buf(buf);
+ return 0;
}
static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
@@ -409,25 +407,20 @@ static int virtscsi_kick_cmd(struct
virtio_scsi_target_state *tgt,
struct virtio_scsi_cmd *cmd,
size_t req_size, size_t resp_size, gfp_t gfp)
{
- unsigned int out_num, in_num;
unsigned long flags;
- int err;
+
Re: [PATCH v2 2/5] virtio-scsi: use functions for piecewise composition of buffers
Il 18/12/2012 14:37, Michael S. Tsirkin ha scritto:
On Tue, Dec 18, 2012 at 01:32:49PM +0100, Paolo Bonzini wrote:
Using the new virtio_scsi_add_sg function lets us simplify the queueing
path. In particular, all data protected by the tgt_lock is just gone
(multiqueue will find a new use for the lock).
vq access still needs some protection: virtio is not reentrant
by itself. with tgt_lock gone what protects vq against
concurrent add_buf calls?
vq_lock.
Paolo
The speedup is relatively small (2-4%) but it is worthwhile because of
the code simplification---both in this patches and in the next ones.
Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: new
drivers/scsi/virtio_scsi.c | 94
+++
1 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 74ab67a..2b93b6e 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -59,11 +59,8 @@ struct virtio_scsi_vq {
/* Per-target queue state */
struct virtio_scsi_target_state {
-/* Protects sg. Lock hierarchy is tgt_lock - vq_lock. */
+/* Never held at the same time as vq_lock. */
spinlock_t tgt_lock;
-
-/* For sglist construction when adding commands to the virtqueue. */
-struct scatterlist sg[];
};
/* Driver instance state */
@@ -351,57 +348,58 @@ static void virtscsi_event_done(struct virtqueue *vq)
spin_unlock_irqrestore(vscsi-event_vq.vq_lock, flags);
};
-static void virtscsi_map_sgl(struct scatterlist *sg, unsigned int *p_idx,
- struct scsi_data_buffer *sdb)
-{
-struct sg_table *table = sdb-table;
-struct scatterlist *sg_elem;
-unsigned int idx = *p_idx;
-int i;
-
-for_each_sg(table-sgl, sg_elem, table-nents, i)
-sg[idx++] = *sg_elem;
-
-*p_idx = idx;
-}
-
/**
- * virtscsi_map_cmd - map a scsi_cmd to a virtqueue scatterlist
+ * virtscsi_add_cmd - add a virtio_scsi_cmd to a virtqueue
* @vscsi : virtio_scsi state
* @cmd : command structure
- * @out_num : number of read-only elements
- * @in_num : number of write-only elements
* @req_size: size of the request buffer
* @resp_size : size of the response buffer
- *
- * Called with tgt_lock held.
+ * @gfp : flags to use for memory allocations
*/
-static void virtscsi_map_cmd(struct virtio_scsi_target_state *tgt,
- struct virtio_scsi_cmd *cmd,
- unsigned *out_num, unsigned *in_num,
- size_t req_size, size_t resp_size)
+static int virtscsi_add_cmd(struct virtqueue *vq,
+struct virtio_scsi_cmd *cmd,
+size_t req_size, size_t resp_size, gfp_t gfp)
{
struct scsi_cmnd *sc = cmd-sc;
-struct scatterlist *sg = tgt-sg;
-unsigned int idx = 0;
+struct scatterlist sg;
+unsigned int count, count_sg;
+struct sg_table *out, *in;
+struct virtqueue_buf buf;
+int ret;
+
+out = in = NULL;
+
+if (sc sc-sc_data_direction != DMA_NONE) {
+if (sc-sc_data_direction != DMA_FROM_DEVICE)
+out = scsi_out(sc)-table;
+if (sc-sc_data_direction != DMA_TO_DEVICE)
+in = scsi_in(sc)-table;
+}
+
+count_sg = 2 + (out ? 1 : 0) + (in ? 1 : 0);
+count= 2 + (out ? out-nents : 0) + (in ? in-nents : 0);
+ret = virtqueue_start_buf(vq, buf, cmd, count, count_sg, gfp);
+if (ret 0)
+return ret;
/* Request header. */
-sg_set_buf(sg[idx++], cmd-req, req_size);
+sg_init_one(sg, cmd-req, req_size);
+virtqueue_add_sg(buf, sg, 1, DMA_TO_DEVICE);
/* Data-out buffer. */
-if (sc sc-sc_data_direction != DMA_FROM_DEVICE)
-virtscsi_map_sgl(sg, idx, scsi_out(sc));
-
-*out_num = idx;
+if (out)
+virtqueue_add_sg(buf, out-sgl, out-nents, DMA_TO_DEVICE);
/* Response header. */
-sg_set_buf(sg[idx++], cmd-resp, resp_size);
+sg_init_one(sg, cmd-resp, resp_size);
+virtqueue_add_sg(buf, sg, 1, DMA_FROM_DEVICE);
/* Data-in buffer */
-if (sc sc-sc_data_direction != DMA_TO_DEVICE)
-virtscsi_map_sgl(sg, idx, scsi_in(sc));
+if (in)
+virtqueue_add_sg(buf, in-sgl, in-nents, DMA_FROM_DEVICE);
-*in_num = idx - *out_num;
+virtqueue_end_buf(buf);
+return 0;
}
static int virtscsi_kick_cmd(struct virtio_scsi_target_state *tgt,
@@ -409,25 +407,20 @@ static int virtscsi_kick_cmd(struct
virtio_scsi_target_state *tgt,
struct virtio_scsi_cmd *cmd,
size_t req_size, size_t resp_size, gfp_t gfp)
{
-unsigned int out_num, in_num;
unsigned long flags;
-int err;
Re: [PATCH] [SCSI] mpt2sas: fix for driver fails EEH recovery from injected pci bus error
On 12/18/2012 06:07 AM, Reddy, Sreekanth wrote:
Yes Thomas, we need to reset the non_operational_loop to zero after the
transient event.
OK, so let me repost a V2 of the whole patch.
Thanks,
Sreekanth.
-Original Message-
From: Tomas Henzl [mailto:the...@redhat.com]
Sent: Monday, December 17, 2012 6:43 PM
To: Reddy, Sreekanth
Cc: j...@kernel.org; Nandigama, Nagalakshmi; jbottom...@parallels.com;
linux-scsi@vger.kernel.org; Prakash, Sathya
Subject: Re: [PATCH] [SCSI] mpt2sas: fix for driver fails EEH recovery from
injected pci bus error
On 12/17/2012 10:58 PM, Sreekanth Reddy wrote:
This patch stops the driver to invoke kthread (which remove the dead
ioc) for some time while EEH recovery has started.
Thank you for posting this, the issue we have seen is resolved now.
Shouldn't be an additional initialization added?
So after a transient event the non_operational_loop is reset again?
Tomas
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
b/drivers/scsi/mpt2sas/mpt2sas_base.c
index fd3b3d7..480111c 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -208,6 +208,8 @@ _base_fault_reset_work(struct work_struct *work)
return; /* don't rearm timer */
}
+ ioc-non_operational_loop = 0;
+
if ((doorbell MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_FAULT) {
rc = mpt2sas_base_hard_reset_handler(ioc, CAN_SLEEP,
FORCE_BIG_HAMMER);
Signed-off-by: Sreekanth Reddy sreekanth.re...@lsi.com
---
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
b/drivers/scsi/mpt2sas/mpt2sas_base.c
index ffd85c5..2349531 100755
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -155,7 +155,7 @@ _base_fault_reset_work(struct work_struct *work)
struct task_struct *p;
spin_lock_irqsave(ioc-ioc_reset_in_progress_lock, flags);
-if (ioc-shost_recovery)
+if (ioc-shost_recovery || ioc-pci_error_recovery)
goto rearm_timer;
spin_unlock_irqrestore(ioc-ioc_reset_in_progress_lock, flags);
@@ -164,6 +164,20 @@ _base_fault_reset_work(struct work_struct *work)
printk(MPT2SAS_INFO_FMT %s : SAS host is non-operational
\n,
ioc-name, __func__);
+/* It may be possible that EEH recovery can resolve some of
+ * pci bus failure issues rather removing the dead ioc function
+ * by considering controller is in a non-operational state. So
+ * here priority is given to the EEH recovery. If it doesn't
+ * not resolve this issue, mpt2sas driver will consider this
+ * controller to non-operational state and remove the dead ioc
+ * function.
+ */
+if (ioc-non_operational_loop++ 5) {
+spin_lock_irqsave(ioc-ioc_reset_in_progress_lock,
+ flags);
+goto rearm_timer;
+}
+
/*
* Call _scsih_flush_pending_cmds callback so that we flush all
* pending commands back to OS. This call is required to aovid
@@
-4386,6 +4400,7 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc)
if (missing_delay[0] != -1 missing_delay[1] != -1)
_base_update_missing_delay(ioc, missing_delay[0],
missing_delay[1]);
+ioc-non_operational_loop = 0;
return 0;
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index 543d8d6..c6ee7aa 100755
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -835,6 +835,7 @@ struct MPT2SAS_ADAPTER {
u16 cpu_msix_table_sz;
u32 ioc_reset_count;
MPT2SAS_FLUSH_RUNNING_CMDS schedule_dead_ioc_flush_running_cmds;
+u32 non_operational_loop;
/* internal commands, callback index */
u8 scsi_io_cb_idx;
--
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
--
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
--
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 v2 0/5] Multiqueue virtio-scsi, and API for piecewise buffer submission
On Tue, Dec 18, 2012 at 01:32:47PM +0100, Paolo Bonzini wrote: Hi all, this series adds multiqueue support to the virtio-scsi driver, based on Jason Wang's work on virtio-net. It uses a simple queue steering algorithm that expects one queue per CPU. LUNs in the same target always use the same queue (so that commands are not reordered); queue switching occurs when the request being queued is the only one for the target. Also based on Jason's patches, the virtqueue affinity is set so that each CPU is associated to one virtqueue. I tested the patches with fio, using up to 32 virtio-scsi disks backed by tmpfs on the host. These numbers are with 1 LUN per target. FIO configuration - [global] rw=read bsrange=4k-64k ioengine=libaio direct=1 iodepth=4 loops=20 overall bandwidth (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 795 965 925 4 997 13761500 8 1136 21302060 161440 22692474 241408 21792436 321515 19782319 (These numbers for single-queue are with 4 VCPUs, but the impact of adding more VCPUs is very limited). avg bandwidth per LUN (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 397 482 462 4 249 344 375 8 142 266 257 16 90 141 154 24 5890 101 32 4761 72 Could you please try and measure host CPU utilization? Without this data it is possible that your host is undersubscribed and you are drinking up more host CPU. Another thing to note is that ATM you might need to test with idle=poll on host otherwise we have strange interaction with power management where reducing the overhead switches to lower power so gives you a worse IOPS. Patch 1 adds a new API to add functions for piecewise addition for buffers, which enables various simplifications in virtio-scsi (patches 2-3) and a small performance improvement of 2-6%. Patches 4 and 5 add multiqueuing. I'm mostly looking for comments on the new API of patch 1 for inclusion into the 3.9 kernel. Thanks to Wao Ganlong for help rebasing and benchmarking these patches. Paolo Bonzini (5): virtio: add functions for piecewise addition of buffers virtio-scsi: use functions for piecewise composition of buffers virtio-scsi: redo allocation of target data virtio-scsi: pass struct virtio_scsi to virtqueue completion function virtio-scsi: introduce multiqueue support drivers/scsi/virtio_scsi.c | 374 +- drivers/virtio/virtio_ring.c | 205 include/linux/virtio.h | 21 +++ 3 files changed, 485 insertions(+), 115 deletions(-) -- 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 v2 1/5] virtio: add functions for piecewise addition of buffers
Il 18/12/2012 14:36, Michael S. Tsirkin ha scritto:
Some comments without arguing about whether the performance
benefit is worth it.
On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index cf8adb1..39d56c4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -7,6 +7,7 @@
#include linux/spinlock.h
#include linux/device.h
#include linux/mod_devicetable.h
+#include linux/dma-direction.h
#include linux/gfp.h
/**
@@ -40,6 +41,26 @@ int virtqueue_add_buf(struct virtqueue *vq,
void *data,
gfp_t gfp);
+struct virtqueue_buf {
+struct virtqueue *vq;
+struct vring_desc *indirect, *tail;
This is wrong: virtio.h does not include virito_ring.h,
and it shouldn't by design depend on it.
+int head;
+};
+
Can't we track state internally to the virtqueue?
Exposing it seems to buy us nothing since you can't
call add_buf between start and end anyway.
I wanted to keep the state for these functions separate from the rest.
I don't think it makes much sense to move it to struct virtqueue unless
virtqueue_add_buf is converted to use the new API (doesn't make much
sense, could even be a tad slower).
On the other hand moving it there would eliminate the dependency on
virtio_ring.h. Rusty, what do you think?
+int virtqueue_start_buf(struct virtqueue *_vq,
+struct virtqueue_buf *buf,
+void *data,
+unsigned int count,
+unsigned int count_sg,
+gfp_t gfp);
+
+void virtqueue_add_sg(struct virtqueue_buf *buf,
+ struct scatterlist sgl[],
+ unsigned int count,
+ enum dma_data_direction dir);
+
And idea: in practice virtio scsi seems to always call sg_init_one, no?
So how about we pass in void* or something and avoid using sg and count?
This would make it useful for -net BTW.
It also passes the scatterlist from the LLD. It calls sg_init_one for
the request/response headers.
Paolo
+void virtqueue_end_buf(struct virtqueue_buf *buf);
+
void virtqueue_kick(struct virtqueue *vq);
bool virtqueue_kick_prepare(struct virtqueue *vq);
--
1.7.1
--
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
[PATCH v2] [SCSI] mpt2sas: fix for driver fails EEH, recovery from injected pci bus error
This patch stops the driver to invoke kthread (which remove the dead ioc)
for some time while EEH recovery has started.
V2 adds a 'non_operational_loop' reset.
Signed-off-by: Sreekanth Reddy sreekanth.re...@lsi.com
Signed-off-by: Tomas Henzl the...@redhat.com
---
drivers/scsi/mpt2sas/mpt2sas_base.c | 19 ++-
drivers/scsi/mpt2sas/mpt2sas_base.h | 1 +
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c
b/drivers/scsi/mpt2sas/mpt2sas_base.c
index 6102ef2..b5d316f 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.c
@@ -155,7 +155,7 @@ _base_fault_reset_work(struct work_struct *work)
struct task_struct *p;
spin_lock_irqsave(ioc-ioc_reset_in_progress_lock, flags);
- if (ioc-shost_recovery)
+ if (ioc-shost_recovery || ioc-pci_error_recovery)
goto rearm_timer;
spin_unlock_irqrestore(ioc-ioc_reset_in_progress_lock, flags);
@@ -164,6 +164,20 @@ _base_fault_reset_work(struct work_struct *work)
printk(MPT2SAS_INFO_FMT %s : SAS host is non-operational
\n,
ioc-name, __func__);
+ /* It may be possible that EEH recovery can resolve some of
+* pci bus failure issues rather removing the dead ioc function
+* by considering controller is in a non-operational state. So
+* here priority is given to the EEH recovery. If it doesn't
+* not resolve this issue, mpt2sas driver will consider this
+* controller to non-operational state and remove the dead ioc
+* function.
+*/
+ if (ioc-non_operational_loop++ 5) {
+ spin_lock_irqsave(ioc-ioc_reset_in_progress_lock,
+flags);
+ goto rearm_timer;
+ }
+
/*
* Call _scsih_flush_pending_cmds callback so that we flush all
* pending commands back to OS. This call is required to aovid
@@ -193,6 +207,8 @@ _base_fault_reset_work(struct work_struct *work)
return; /* don't rearm timer */
}
+ ioc-non_operational_loop = 0;
+
if ((doorbell MPI2_IOC_STATE_MASK) == MPI2_IOC_STATE_FAULT) {
rc = mpt2sas_base_hard_reset_handler(ioc, CAN_SLEEP,
FORCE_BIG_HAMMER);
@@ -4376,6 +4392,7 @@ mpt2sas_base_attach(struct MPT2SAS_ADAPTER *ioc)
if (missing_delay[0] != -1 missing_delay[1] != -1)
_base_update_missing_delay(ioc, missing_delay[0],
missing_delay[1]);
+ ioc-non_operational_loop = 0;
return 0;
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index b6dd3a5..f69bbe2 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -835,6 +835,7 @@ struct MPT2SAS_ADAPTER {
u16 cpu_msix_table_sz;
u32 ioc_reset_count;
MPT2SAS_FLUSH_RUNNING_CMDS schedule_dead_ioc_flush_running_cmds;
+ u32 non_operational_loop;
/* internal commands, callback index */
u8 scsi_io_cb_idx;
--
1.7.11.7
--
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 v2 5/5] virtio-scsi: introduce multiqueue support
On Tue, Dec 18, 2012 at 01:32:52PM +0100, Paolo Bonzini wrote:
This patch adds queue steering to virtio-scsi. When a target is sent
multiple requests, we always drive them to the same queue so that FIFO
processing order is kept. However, if a target was idle, we can choose
a queue arbitrarily. In this case the queue is chosen according to the
current VCPU, so the driver expects the number of request queues to be
equal to the number of VCPUs. This makes it easy and fast to select
the queue, and also lets the driver optimize the IRQ affinity for the
virtqueues (each virtqueue's affinity is set to the CPU that owns
the queue).
The speedup comes from improving cache locality and giving CPU affinity
to the virtqueues, which is why this scheme was selected. Assuming that
the thread that is sending requests to the device is I/O-bound, it is
likely to be sleeping at the time the ISR is executed, and thus executing
the ISR on the same processor that sent the requests is cheap.
However, the kernel will not execute the ISR on the best processor
unless you explicitly set the affinity. This is because in practice
you will have many such I/O-bound processes and thus many otherwise
idle processors. Then the kernel will execute the ISR on a random
processor, rather than the one that is sending requests to the device.
The alternative to per-CPU virtqueues is per-target virtqueues. To
achieve the same locality, we could dynamically choose the virtqueue's
affinity based on the CPU of the last task that sent a request. This
is less appealing because we do not set the affinity directly---we only
provide a hint to the irqbalanced running in userspace. Dynamically
changing the affinity only works if the userspace applies the hint
fast enough.
Signed-off-by: Paolo Bonzini pbonz...@redhat.com
---
v1-v2: improved comments and commit messages, added memory barriers
drivers/scsi/virtio_scsi.c | 234 +--
1 files changed, 201 insertions(+), 33 deletions(-)
diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 4f6c6a3..ca9d29d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -26,6 +26,7 @@
#define VIRTIO_SCSI_MEMPOOL_SZ 64
#define VIRTIO_SCSI_EVENT_LEN 8
+#define VIRTIO_SCSI_VQ_BASE 2
/* Command queue element */
struct virtio_scsi_cmd {
@@ -57,24 +58,57 @@ struct virtio_scsi_vq {
struct virtqueue *vq;
};
-/* Per-target queue state */
+/*
+ * Per-target queue state.
+ *
+ * This struct holds the data needed by the queue steering policy. When a
+ * target is sent multiple requests, we need to drive them to the same queue
so
+ * that FIFO processing order is kept. However, if a target was idle, we can
+ * choose a queue arbitrarily. In this case the queue is chosen according to
+ * the current VCPU, so the driver expects the number of request queues to be
+ * equal to the number of VCPUs. This makes it easy and fast to select the
+ * queue, and also lets the driver optimize the IRQ affinity for the
virtqueues
+ * (each virtqueue's affinity is set to the CPU that owns the queue).
+ *
+ * An interesting effect of this policy is that only writes to req_vq need to
+ * take the tgt_lock. Read can be done outside the lock because:
+ *
+ * - writes of req_vq only occur when atomic_inc_return(tgt-reqs) returns
1.
+ * In that case, no other CPU is reading req_vq: even if they were in
+ * virtscsi_queuecommand_multi, they would be spinning on tgt_lock.
+ *
+ * - reads of req_vq only occur when the target is not idle (reqs != 0).
+ * A CPU that enters virtscsi_queuecommand_multi will not modify req_vq.
+ *
+ * Similarly, decrements of reqs are never concurrent with writes of req_vq.
+ * Thus they can happen outside the tgt_lock, provided of course we make reqs
+ * an atomic_t.
+ */
struct virtio_scsi_target_state {
- /* Never held at the same time as vq_lock. */
+ /* This spinlock ever held at the same time as vq_lock. */
spinlock_t tgt_lock;
+
+ /* Count of outstanding requests. */
+ atomic_t reqs;
+
+ /* Currently active virtqueue for requests sent to this target. */
+ struct virtio_scsi_vq *req_vq;
};
/* Driver instance state */
struct virtio_scsi {
struct virtio_device *vdev;
- struct virtio_scsi_vq ctrl_vq;
- struct virtio_scsi_vq event_vq;
- struct virtio_scsi_vq req_vq;
-
/* Get some buffers ready for event vq */
struct virtio_scsi_event_node event_list[VIRTIO_SCSI_EVENT_LEN];
struct virtio_scsi_target_state *tgt;
+
+ u32 num_queues;
+
+ struct virtio_scsi_vq ctrl_vq;
+ struct virtio_scsi_vq event_vq;
+ struct virtio_scsi_vq req_vqs[];
};
static struct kmem_cache *virtscsi_cmd_cache;
@@ -109,6 +143,7 @@ static void virtscsi_complete_cmd(struct virtio_scsi
*vscsi, void *buf)
struct
Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers
On Tue, Dec 18, 2012 at 02:43:51PM +0100, Paolo Bonzini wrote:
Il 18/12/2012 14:36, Michael S. Tsirkin ha scritto:
Some comments without arguing about whether the performance
benefit is worth it.
On Tue, Dec 18, 2012 at 01:32:48PM +0100, Paolo Bonzini wrote:
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index cf8adb1..39d56c4 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -7,6 +7,7 @@
#include linux/spinlock.h
#include linux/device.h
#include linux/mod_devicetable.h
+#include linux/dma-direction.h
#include linux/gfp.h
/**
@@ -40,6 +41,26 @@ int virtqueue_add_buf(struct virtqueue *vq,
void *data,
gfp_t gfp);
+struct virtqueue_buf {
+ struct virtqueue *vq;
+ struct vring_desc *indirect, *tail;
This is wrong: virtio.h does not include virito_ring.h,
and it shouldn't by design depend on it.
+ int head;
+};
+
Can't we track state internally to the virtqueue?
Exposing it seems to buy us nothing since you can't
call add_buf between start and end anyway.
I wanted to keep the state for these functions separate from the rest.
I don't think it makes much sense to move it to struct virtqueue unless
virtqueue_add_buf is converted to use the new API (doesn't make much
sense, could even be a tad slower).
Why would it be slower?
On the other hand moving it there would eliminate the dependency on
virtio_ring.h. Rusty, what do you think?
+int virtqueue_start_buf(struct virtqueue *_vq,
+ struct virtqueue_buf *buf,
+ void *data,
+ unsigned int count,
+ unsigned int count_sg,
+ gfp_t gfp);
+
+void virtqueue_add_sg(struct virtqueue_buf *buf,
+struct scatterlist sgl[],
+unsigned int count,
+enum dma_data_direction dir);
+
And idea: in practice virtio scsi seems to always call sg_init_one, no?
So how about we pass in void* or something and avoid using sg and count?
This would make it useful for -net BTW.
It also passes the scatterlist from the LLD. It calls sg_init_one for
the request/response headers.
Paolo
Try adding a _single variant. You might see unrolling a loop
gives more of a benefit than this whole optimization.
+void virtqueue_end_buf(struct virtqueue_buf *buf);
+
void virtqueue_kick(struct virtqueue *vq);
bool virtqueue_kick_prepare(struct virtqueue *vq);
--
1.7.1
--
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 v2 5/5] virtio-scsi: introduce multiqueue support
Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
-static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
+static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
+ struct virtio_scsi_target_state *tgt,
+ struct scsi_cmnd *sc)
{
-struct virtio_scsi *vscsi = shost_priv(sh);
-struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
struct virtio_scsi_cmd *cmd;
+struct virtio_scsi_vq *req_vq;
int ret;
struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
@@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh,
struct scsi_cmnd *sc)
BUG_ON(sc-cmd_len VIRTIO_SCSI_CDB_SIZE);
memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
-if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
+req_vq = ACCESS_ONCE(tgt-req_vq);
This ACCESS_ONCE without a barrier looks strange to me.
Can req_vq change? Needs a comment.
Barriers are needed to order two things. Here I don't have the second thing
to order against, hence no barrier.
Accessing req_vq lockless is safe, and there's a comment about it, but you
still want ACCESS_ONCE to ensure the compiler doesn't play tricks. It
shouldn't be necessary, because the critical section of
virtscsi_queuecommand_multi will already include the appropriate
compiler barriers, but it is actually clearer this way to me. :)
+if (virtscsi_kick_cmd(tgt, req_vq, cmd,
sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
GFP_ATOMIC) == 0)
ret = 0;
@@ -472,6 +545,48 @@ out:
return ret;
}
+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+struct scsi_cmnd *sc)
+{
+struct virtio_scsi *vscsi = shost_priv(sh);
+struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
+
+atomic_inc(tgt-reqs);
And here we don't have barrier after atomic? Why? Needs a comment.
Because we don't write req_vq, so there's no two writes to order. Barrier
against what?
+return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
+static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+struct virtio_scsi *vscsi = shost_priv(sh);
+struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
+unsigned long flags;
+u32 queue_num;
+
+/*
+ * Using an atomic_t for tgt-reqs lets the virtqueue handler
+ * decrement it without taking the spinlock.
+ *
+ * We still need a critical section to prevent concurrent submissions
+ * from picking two different req_vqs.
+ */
+spin_lock_irqsave(tgt-tgt_lock, flags);
+if (atomic_inc_return(tgt-reqs) == 1) {
+queue_num = smp_processor_id();
+while (unlikely(queue_num = vscsi-num_queues))
+queue_num -= vscsi-num_queues;
+
+/*
+ * Write reqs before writing req_vq, matching the
+ * smp_read_barrier_depends() in virtscsi_req_done.
+ */
+smp_wmb();
+tgt-req_vq = vscsi-req_vqs[queue_num];
+}
+spin_unlock_irqrestore(tgt-tgt_lock, flags);
+return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd
*cmd)
{
DECLARE_COMPLETION_ONSTACK(comp);
@@ -541,12 +656,26 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
return virtscsi_tmf(vscsi, cmd);
}
-static struct scsi_host_template virtscsi_host_template = {
+static struct scsi_host_template virtscsi_host_template_single = {
.module = THIS_MODULE,
.name = Virtio SCSI HBA,
.proc_name = virtio_scsi,
-.queuecommand = virtscsi_queuecommand,
.this_id = -1,
+.queuecommand = virtscsi_queuecommand_single,
+.eh_abort_handler = virtscsi_abort,
+.eh_device_reset_handler = virtscsi_device_reset,
+
+.can_queue = 1024,
+.dma_boundary = UINT_MAX,
+.use_clustering = ENABLE_CLUSTERING,
+};
+
+static struct scsi_host_template virtscsi_host_template_multi = {
+.module = THIS_MODULE,
+.name = Virtio SCSI HBA,
+.proc_name = virtio_scsi,
+.this_id = -1,
+.queuecommand = virtscsi_queuecommand_multi,
.eh_abort_handler = virtscsi_abort,
.eh_device_reset_handler = virtscsi_device_reset,
@@ -572,16 +701,27 @@ static struct scsi_host_template
virtscsi_host_template = {
__val, sizeof(__val)); \
})
+
static void virtscsi_init_vq(struct virtio_scsi_vq *virtscsi_vq,
- struct virtqueue *vq)
+ struct virtqueue *vq, bool affinity)
{
spin_lock_init(virtscsi_vq-vq_lock);
virtscsi_vq-vq = vq;
+if (affinity)
+virtqueue_set_affinity(vq, vq-index - VIRTIO_SCSI_VQ_BASE);
I've been
Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers
Il 18/12/2012 14:59, Michael S. Tsirkin ha scritto: Can't we track state internally to the virtqueue? Exposing it seems to buy us nothing since you can't call add_buf between start and end anyway. I wanted to keep the state for these functions separate from the rest. I don't think it makes much sense to move it to struct virtqueue unless virtqueue_add_buf is converted to use the new API (doesn't make much sense, could even be a tad slower). Why would it be slower? virtqueue_add_buf could be slower if it used the new API. That's because of the overhead of writing and reading from struct virtqueue_buf, instead of using variables in registers. On the other hand moving it there would eliminate the dependency on virtio_ring.h. Rusty, what do you think? And idea: in practice virtio scsi seems to always call sg_init_one, no? So how about we pass in void* or something and avoid using sg and count? This would make it useful for -net BTW. It also passes the scatterlist from the LLD. It calls sg_init_one for the request/response headers. Try adding a _single variant. You might see unrolling a loop gives more of a benefit than this whole optimization. Makes sense, I'll try. However, note that I *do* need the infrastructure in this patch because virtio-scsi could never use a hypothetical virtqueue_add_buf_single; requests always have at least 2 buffers for the headers. However I could add virtqueue_add_sg_single and use it for those headers. The I/O buffer can keep using virtqueue_add_sg. Paolo -- 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] scsi: remove device when state is changed to deleted thru sysfs
Bart Van Assche wrote:
On 12/18/12 00:26, David Milburn wrote:
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index ce5224c..6d72abb 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -596,7 +596,7 @@ static ssize_t
store_state_field(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
-int i;
+int i, rc;
struct scsi_device *sdev = to_scsi_device(dev);
enum scsi_device_state state = 0;
@@ -611,7 +611,11 @@ store_state_field(struct device *dev, struct
device_attribute *attr,
if (!state)
return -EINVAL;
-if (scsi_device_set_state(sdev, state))
+if (state == SDEV_DEL) {
+rc = device_schedule_callback(dev, sdev_store_delete_callback);
+if (rc)
+count = rc;
+} else if (scsi_device_set_state(sdev, state))
return -EINVAL;
return count;
}
Hello David,
And what about the cancel state ? I guess this means that you have
missed patch http://marc.info/?l=linux-scsim=135480941801843 ?
Hi Bart,
Sorry, I did miss that patch.
Adding SDEV_CANCEL to the above check would result in __scsi_remove_device()
not completing since it wouldn't be able to set the device state from
SDEV_CANCEL
to SDEV_CANCEL. Your patch to not allow this state change makes sense.
Thanks,
David
Bart.
--
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
--
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 v2 5/5] virtio-scsi: introduce multiqueue support
On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
-static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd
*sc)
+static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
+ struct virtio_scsi_target_state *tgt,
+ struct scsi_cmnd *sc)
{
- struct virtio_scsi *vscsi = shost_priv(sh);
- struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
struct virtio_scsi_cmd *cmd;
+ struct virtio_scsi_vq *req_vq;
int ret;
struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
@@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh,
struct scsi_cmnd *sc)
BUG_ON(sc-cmd_len VIRTIO_SCSI_CDB_SIZE);
memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
- if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
+ req_vq = ACCESS_ONCE(tgt-req_vq);
This ACCESS_ONCE without a barrier looks strange to me.
Can req_vq change? Needs a comment.
Barriers are needed to order two things. Here I don't have the second thing
to order against, hence no barrier.
Accessing req_vq lockless is safe, and there's a comment about it, but you
still want ACCESS_ONCE to ensure the compiler doesn't play tricks.
That's just it.
Why don't you want compiler to play tricks?
ACCESS_ONCE is needed if the value can change
while you access it, this helps ensure
a consistent value is evalutated.
If it can you almost always need a barrier. If it doesn't
you don't need ACCESS_ONCE.
It
shouldn't be necessary, because the critical section of
virtscsi_queuecommand_multi will already include the appropriate
compiler barriers,
So if there's a barrier then pls add a comment saying where
it is.
but it is actually clearer this way to me. :)
No barriers are needed I think because
when you queue command req is incremented to req_vq
can not change. But this also means ACCESS_ONCE
is not needed either.
+ if (virtscsi_kick_cmd(tgt, req_vq, cmd,
sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
GFP_ATOMIC) == 0)
ret = 0;
@@ -472,6 +545,48 @@ out:
return ret;
}
+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+ struct virtio_scsi *vscsi = shost_priv(sh);
+ struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
+
+ atomic_inc(tgt-reqs);
And here we don't have barrier after atomic? Why? Needs a comment.
Because we don't write req_vq, so there's no two writes to order. Barrier
against what?
Between atomic update and command. Once you queue command it
can complete and decrement reqs, if this happens before
increment reqs can become negative even.
+ return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
+static int virtscsi_queuecommand_multi(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+ struct virtio_scsi *vscsi = shost_priv(sh);
+ struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
+ unsigned long flags;
+ u32 queue_num;
+
+ /*
+ * Using an atomic_t for tgt-reqs lets the virtqueue handler
+ * decrement it without taking the spinlock.
+ *
+ * We still need a critical section to prevent concurrent submissions
+ * from picking two different req_vqs.
+ */
+ spin_lock_irqsave(tgt-tgt_lock, flags);
+ if (atomic_inc_return(tgt-reqs) == 1) {
+ queue_num = smp_processor_id();
+ while (unlikely(queue_num = vscsi-num_queues))
+ queue_num -= vscsi-num_queues;
+
+ /*
+ * Write reqs before writing req_vq, matching the
+ * smp_read_barrier_depends() in virtscsi_req_done.
+ */
+ smp_wmb();
+ tgt-req_vq = vscsi-req_vqs[queue_num];
+ }
+ spin_unlock_irqrestore(tgt-tgt_lock, flags);
+ return virtscsi_queuecommand(vscsi, tgt, sc);
+}
+
static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd
*cmd)
{
DECLARE_COMPLETION_ONSTACK(comp);
@@ -541,12 +656,26 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
return virtscsi_tmf(vscsi, cmd);
}
-static struct scsi_host_template virtscsi_host_template = {
+static struct scsi_host_template virtscsi_host_template_single = {
.module = THIS_MODULE,
.name = Virtio SCSI HBA,
.proc_name = virtio_scsi,
- .queuecommand = virtscsi_queuecommand,
.this_id = -1,
+ .queuecommand = virtscsi_queuecommand_single,
+ .eh_abort_handler = virtscsi_abort,
+ .eh_device_reset_handler = virtscsi_device_reset,
+
+ .can_queue = 1024,
+ .dma_boundary = UINT_MAX,
+ .use_clustering = ENABLE_CLUSTERING,
+};
+
+static struct scsi_host_template virtscsi_host_template_multi = {
+ .module = THIS_MODULE,
+ .name = Virtio SCSI HBA,
+ .proc_name
Re: [PATCH v2 1/5] virtio: add functions for piecewise addition of buffers
On Tue, Dec 18, 2012 at 03:32:15PM +0100, Paolo Bonzini wrote: Il 18/12/2012 14:59, Michael S. Tsirkin ha scritto: Can't we track state internally to the virtqueue? Exposing it seems to buy us nothing since you can't call add_buf between start and end anyway. I wanted to keep the state for these functions separate from the rest. I don't think it makes much sense to move it to struct virtqueue unless virtqueue_add_buf is converted to use the new API (doesn't make much sense, could even be a tad slower). Why would it be slower? virtqueue_add_buf could be slower if it used the new API. That's because of the overhead of writing and reading from struct virtqueue_buf, instead of using variables in registers. Yes but we'll get rid of virtqueue_buf. On the other hand moving it there would eliminate the dependency on virtio_ring.h. Rusty, what do you think? And idea: in practice virtio scsi seems to always call sg_init_one, no? So how about we pass in void* or something and avoid using sg and count? This would make it useful for -net BTW. It also passes the scatterlist from the LLD. It calls sg_init_one for the request/response headers. Try adding a _single variant. You might see unrolling a loop gives more of a benefit than this whole optimization. Makes sense, I'll try. However, note that I *do* need the infrastructure in this patch because virtio-scsi could never use a hypothetical virtqueue_add_buf_single; requests always have at least 2 buffers for the headers. However I could add virtqueue_add_sg_single and use it for those headers. Right. The I/O buffer can keep using virtqueue_add_sg. Paolo -- 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 v2 5/5] virtio-scsi: introduce multiqueue support
Il 18/12/2012 16:03, Michael S. Tsirkin ha scritto:
On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
-static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd
*sc)
+static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
+ struct virtio_scsi_target_state *tgt,
+ struct scsi_cmnd *sc)
{
- struct virtio_scsi *vscsi = shost_priv(sh);
- struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
struct virtio_scsi_cmd *cmd;
+ struct virtio_scsi_vq *req_vq;
int ret;
struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
@@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host *sh,
struct scsi_cmnd *sc)
BUG_ON(sc-cmd_len VIRTIO_SCSI_CDB_SIZE);
memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
- if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
+ req_vq = ACCESS_ONCE(tgt-req_vq);
This ACCESS_ONCE without a barrier looks strange to me.
Can req_vq change? Needs a comment.
Barriers are needed to order two things. Here I don't have the second thing
to order against, hence no barrier.
Accessing req_vq lockless is safe, and there's a comment about it, but you
still want ACCESS_ONCE to ensure the compiler doesn't play tricks.
That's just it.
Why don't you want compiler to play tricks?
Because I want the lockless access to occur exactly when I write it.
Otherwise I have one more thing to think about, i.e. what a crazy
compiler writer could do with my code. And having been on the other
side of the trench, compiler writers can have *really* crazy ideas.
Anyhow, I'll reorganize the code to move the ACCESS_ONCE closer to the
write and make it clearer.
+ if (virtscsi_kick_cmd(tgt, req_vq, cmd,
sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
GFP_ATOMIC) == 0)
ret = 0;
@@ -472,6 +545,48 @@ out:
return ret;
}
+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+ struct scsi_cmnd *sc)
+{
+ struct virtio_scsi *vscsi = shost_priv(sh);
+ struct virtio_scsi_target_state *tgt = vscsi-tgt[sc-device-id];
+
+ atomic_inc(tgt-reqs);
And here we don't have barrier after atomic? Why? Needs a comment.
Because we don't write req_vq, so there's no two writes to order. Barrier
against what?
Between atomic update and command. Once you queue command it
can complete and decrement reqs, if this happens before
increment reqs can become negative even.
This is not a problem. Please read Documentation/memory-barrier.txt:
The following also do _not_ imply memory barriers, and so may
require explicit memory barriers under some circumstances
(smp_mb__before_atomic_dec() for instance):
atomic_add();
atomic_sub();
atomic_inc();
atomic_dec();
If they're used for statistics generation, then they probably don't
need memory barriers, unless there's a coupling between statistical
data.
This is the single-queue case, so it falls under this case.
/* Discover virtqueues and write information to configuration. */
- err = vdev-config-find_vqs(vdev, 3, vqs, callbacks, names);
+ err = vdev-config-find_vqs(vdev, num_vqs, vqs, callbacks, names);
if (err)
return err;
- virtscsi_init_vq(vscsi-ctrl_vq, vqs[0]);
- virtscsi_init_vq(vscsi-event_vq, vqs[1]);
- virtscsi_init_vq(vscsi-req_vq, vqs[2]);
+ virtscsi_init_vq(vscsi-ctrl_vq, vqs[0], false);
+ virtscsi_init_vq(vscsi-event_vq, vqs[1], false);
+ for (i = VIRTIO_SCSI_VQ_BASE; i num_vqs; i++)
+ virtscsi_init_vq(vscsi-req_vqs[i - VIRTIO_SCSI_VQ_BASE],
+ vqs[i], vscsi-num_queues 1);
So affinity is true if 1 vq? I am guessing this is not
going to do the right thing unless you have at least
as many vqs as CPUs.
Yes, and then you're not setting up the thing correctly.
Why not just check instead of doing the wrong thing?
The right thing could be to set the affinity with a stride, e.g. CPUs
0-4 for virtqueue 0 and so on until CPUs 3-7 for virtqueue 3.
Paolo
Isn't the same thing true for virtio-net mq?
Paolo
Last I looked it checked vi-max_queue_pairs == num_online_cpus().
This is even too aggressive I think, max_queue_pairs =
num_online_cpus() should be enough.
--
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 v2 5/5] virtio-scsi: introduce multiqueue support
On Tue, Dec 18, 2012 at 04:51:28PM +0100, Paolo Bonzini wrote:
Il 18/12/2012 16:03, Michael S. Tsirkin ha scritto:
On Tue, Dec 18, 2012 at 03:08:08PM +0100, Paolo Bonzini wrote:
Il 18/12/2012 14:57, Michael S. Tsirkin ha scritto:
-static int virtscsi_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd
*sc)
+static int virtscsi_queuecommand(struct virtio_scsi *vscsi,
+ struct virtio_scsi_target_state *tgt,
+ struct scsi_cmnd *sc)
{
-struct virtio_scsi *vscsi = shost_priv(sh);
-struct virtio_scsi_target_state *tgt =
vscsi-tgt[sc-device-id];
struct virtio_scsi_cmd *cmd;
+struct virtio_scsi_vq *req_vq;
int ret;
struct Scsi_Host *shost = virtio_scsi_host(vscsi-vdev);
@@ -461,7 +533,8 @@ static int virtscsi_queuecommand(struct Scsi_Host
*sh, struct scsi_cmnd *sc)
BUG_ON(sc-cmd_len VIRTIO_SCSI_CDB_SIZE);
memcpy(cmd-req.cmd.cdb, sc-cmnd, sc-cmd_len);
-if (virtscsi_kick_cmd(tgt, vscsi-req_vq, cmd,
+req_vq = ACCESS_ONCE(tgt-req_vq);
This ACCESS_ONCE without a barrier looks strange to me.
Can req_vq change? Needs a comment.
Barriers are needed to order two things. Here I don't have the second
thing
to order against, hence no barrier.
Accessing req_vq lockless is safe, and there's a comment about it, but you
still want ACCESS_ONCE to ensure the compiler doesn't play tricks.
That's just it.
Why don't you want compiler to play tricks?
Because I want the lockless access to occur exactly when I write it.
It doesn't occur when you write it. CPU can still move accesses
around. That's why you either need both ACCESS_ONCE and a barrier
or none.
Otherwise I have one more thing to think about, i.e. what a crazy
compiler writer could do with my code. And having been on the other
side of the trench, compiler writers can have *really* crazy ideas.
Anyhow, I'll reorganize the code to move the ACCESS_ONCE closer to the
write and make it clearer.
+if (virtscsi_kick_cmd(tgt, req_vq, cmd,
sizeof cmd-req.cmd, sizeof cmd-resp.cmd,
GFP_ATOMIC) == 0)
ret = 0;
@@ -472,6 +545,48 @@ out:
return ret;
}
+static int virtscsi_queuecommand_single(struct Scsi_Host *sh,
+struct scsi_cmnd *sc)
+{
+struct virtio_scsi *vscsi = shost_priv(sh);
+struct virtio_scsi_target_state *tgt =
vscsi-tgt[sc-device-id];
+
+atomic_inc(tgt-reqs);
And here we don't have barrier after atomic? Why? Needs a comment.
Because we don't write req_vq, so there's no two writes to order. Barrier
against what?
Between atomic update and command. Once you queue command it
can complete and decrement reqs, if this happens before
increment reqs can become negative even.
This is not a problem. Please read Documentation/memory-barrier.txt:
The following also do _not_ imply memory barriers, and so may
require explicit memory barriers under some circumstances
(smp_mb__before_atomic_dec() for instance):
atomic_add();
atomic_sub();
atomic_inc();
atomic_dec();
If they're used for statistics generation, then they probably don't
need memory barriers, unless there's a coupling between statistical
data.
This is the single-queue case, so it falls under this case.
Aha I missed it's single queue. Correct but please add a comment.
/* Discover virtqueues and write information to configuration.
*/
-err = vdev-config-find_vqs(vdev, 3, vqs, callbacks, names);
+err = vdev-config-find_vqs(vdev, num_vqs, vqs, callbacks,
names);
if (err)
return err;
-virtscsi_init_vq(vscsi-ctrl_vq, vqs[0]);
-virtscsi_init_vq(vscsi-event_vq, vqs[1]);
-virtscsi_init_vq(vscsi-req_vq, vqs[2]);
+virtscsi_init_vq(vscsi-ctrl_vq, vqs[0], false);
+virtscsi_init_vq(vscsi-event_vq, vqs[1], false);
+for (i = VIRTIO_SCSI_VQ_BASE; i num_vqs; i++)
+virtscsi_init_vq(vscsi-req_vqs[i -
VIRTIO_SCSI_VQ_BASE],
+ vqs[i], vscsi-num_queues 1);
So affinity is true if 1 vq? I am guessing this is not
going to do the right thing unless you have at least
as many vqs as CPUs.
Yes, and then you're not setting up the thing correctly.
Why not just check instead of doing the wrong thing?
The right thing could be to set the affinity with a stride, e.g. CPUs
0-4 for virtqueue 0 and so on until CPUs 3-7 for virtqueue 3.
Paolo
I think a simple #vqs == #cpus check would be kind of OK for
starters, otherwise let userspace set affinity.
Again need to think what happens with CPU hotplug.
Isn't the same thing true for
megaraid_sas: problem with specific hardware only with kernel 3.2.5 and above
megaraid_sas driver fails to initialize the storage controller on some Intel platforms. The issue concerns at least the following hardware: Intel Server System R2000IP, which includes: - Intel Server Board S2600IP4 - Intel Integrated RAID Module RMS25CB080 with RES2SV240 RAID Expander Card The RAID module is listed by lspci as: 01:00.0 RAID bus controller: LSI Logic / Symbios Logic MegaRAID SAS 2208 [Thunderbolt] (rev 03) By trying different kernels, we found out, that the problem occurs with 3.2.5 and newer versions. The problem doesn't occur with kernel 3.2.4 and older. On kernel version 3.2.5 the output is: megasas: 00.00.06.12-rc1 Wed. Oct. 5 17:00:00 PDT 2011 megasas: 0x1000:0x005b:0x8086:0x3513: bus 1:slot 0:func 0 megaraid_sas :01:00.0: PCI INT A - GSI 26 (level, low) - IRQ 26 megaraid_sas :01:00.0: setting latency timer to 64 megasas: Waiting for FW to come to ready state megasas: FW in FAULT state!! On kernel 3.2.4: megasas: 00.00.06.12-rc1 Wed. Oct. 5 17:00:00 PDT 2011 megasas: 0x1000:0x005b:0x8086:0x3513: bus 1:slot 0:func 0 megaraid_sas :01:00.0: PCI INT A - GSI 26 (level, low) - IRQ 26 megaraid_sas :01:00.0: setting latency timer to 64 megasas: FW now in Ready state The 3.2.5 introduces only one modification: PCI: Rework ASPM disable code (commit 3c076351c4027a56d5005a39a0b518a4ba393ce2) It looks like this change in kernel behavior for mainboards that don't report the support for ASPM has impact on the SAS controller itself, its' firmware or further communication between the driver and hardware. We tried the following firmware versions from Intel: - 23.9.0-0018 (dated 2012-11-17) - 23.7.0-0033 (the current one) I can collect more information about the hardware, boot logs etc. and test the provided patches according to your directions. Thanks in advance for any help with this issue. -- Michał Miszewski -- 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] megaraid: convert to work_struct
On Thu, Dec 13, 2012 at 04:53:18PM +0800, Xiaotian Feng wrote: There's no need to use delayed work, convert to use work_struct and cancel_work_sync(). Requested-by: Tejun Heo t...@kernel.org Signed-off-by: Xiaotian Feng dannyf...@tencent.com Cc: Neela Syam Kolli megaraidli...@lsi.com Cc: James E.J. Bottomley jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org Cc: linux-ker...@vger.kernel.org Acked-by: Tejun Heo t...@kernel.org 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 v2 0/5] Multiqueue virtio-scsi, and API for piecewise buffer submission
Paolo Bonzini wrote: Hi all, this series adds multiqueue support to the virtio-scsi driver, based on Jason Wang's work on virtio-net. It uses a simple queue steering algorithm that expects one queue per CPU. LUNs in the same target always use the same queue (so that commands are not reordered); queue switching occurs when the request being queued is the only one for the target. Also based on Jason's patches, the virtqueue affinity is set so that each CPU is associated to one virtqueue. I tested the patches with fio, using up to 32 virtio-scsi disks backed by tmpfs on the host. These numbers are with 1 LUN per target. FIO configuration - [global] rw=read bsrange=4k-64k ioengine=libaio direct=1 iodepth=4 loops=20 overall bandwidth (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 795 965 925 4 997 13761500 8 1136 21302060 161440 22692474 241408 21792436 321515 19782319 (These numbers for single-queue are with 4 VCPUs, but the impact of adding more VCPUs is very limited). avg bandwidth per LUN (MB/s) # of targetssingle-queuemulti-queue, 4 VCPUsmulti-queue, 8 VCPUs 1 540 626 599 2 397 482 462 4 249 344 375 8 142 266 257 16 90 141 154 24 5890 101 32 4761 72 Is there an explanation why 8x8 is slower then 4x8 in both cases? 8x1 and 8x2 being slower than 4x1 and 4x2 is more or less expected, but 8x8 loses against 4x8 while 8x4 wins against 4x4 and 8x16 against 4x16. Eike signature.asc Description: This is a digitally signed message part.
