2011-02-01 23:44:55

by Mike Christie

[permalink] [raw]
Subject: Re: [PATCH 16/32] scsi/be2iscsi,qla2xxx: convert to alloc_workqueue()

On 01/03/2011 07:49 AM, Tejun Heo wrote:
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 75a85aa..4339196 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -4276,7 +4276,7 @@ static int __devinit beiscsi_dev_probe(struct pci_dev *pcidev,
>
> snprintf(phba->wq_name, sizeof(phba->wq_name), "beiscsi_q_irq%u",
> phba->shost->host_no);
> - phba->wq = create_workqueue(phba->wq_name);
> + phba->wq = alloc_workqueue(phba->wq_name, WQ_MEM_RECLAIM, 1);
> if (!phba->wq) {
> shost_printk(KERN_ERR, phba->shost, "beiscsi_dev_probe-"
> "Failed to allocate work queue\n");
> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 2c0876c..2cd0a77 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -356,7 +356,7 @@ static int qla25xx_setup_mode(struct scsi_qla_host *vha)
> "Can't create request queue\n");
> goto fail;
> }
> - ha->wq = create_workqueue("qla2xxx_wq");
> + ha->wq = alloc_workqueue("qla2xxx_wq", WQ_MEM_RECLAIM, 1);
> vha->req = ha->req_q_map[req];
> options |= BIT_1;
> for (ques = 1; ques< ha->max_rsp_queues; ques++) {

I think these are used in the main IO path, so would you also want
WQ_HIGHPRI | WQ_CPU_INTENSIVE to be set? I think qla2xxx was using this
in a path they expected to have high throughput/low latency, and from
the interrupt handler they would try to queue the work on the same cpu
the isr was answered on. With the new workqueue code could you possibly
get queued onto a workqueue that is doing other work for other drivers?

Should qla2xxx be using the blkiopoll framework instead of a workqueue
or will workqueues still provide the same performance when setting
WQ_HIGHPRI | WQ_CPU_INTENSIVE?


2011-02-02 10:25:29

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH 16/32] scsi/be2iscsi,qla2xxx: convert to alloc_workqueue()

Hello,

On Tue, Feb 01, 2011 at 05:45:13PM -0600, Mike Christie wrote:
> I think these are used in the main IO path, so would you also want
> WQ_HIGHPRI | WQ_CPU_INTENSIVE to be set?

Maybe HIGHPRI but definitely not CPU_INTENSIVE, but even HIGHPRI isn't
usually necessary. The queue of pending works tend to be short and
consumed pretty fast.

> I think qla2xxx was using this in a path they expected to have high
> throughput/low latency, and from the interrupt handler they would
> try to queue the work on the same cpu the isr was answered on. With
> the new workqueue code could you possibly get queued onto a
> workqueue that is doing other work for other drivers?

There are no separate workqueues. There's one per cpu which is used
by all workqueue users. HIGHPRI boosts the queueing order in that
queue.

> Should qla2xxx be using the blkiopoll framework instead of a
> workqueue or will workqueues still provide the same performance when
> setting WQ_HIGHPRI | WQ_CPU_INTENSIVE?

CPU_INTENSIVE isn't applicable at all unless you want to run crypto or
zlib from the driver.

blk_iopoll runs off softirq and will always have lower latency than
workqueue which requires scheduling, so it depends on the
requirements. I guess the best thing would be playing with things a
bit and make decisions on test results. It shouldn't be too difficult
to switch among them.

Thanks.

--
tejun

2011-02-02 20:41:05

by Mike Christie

[permalink] [raw]
Subject: Re: [PATCH 16/32] scsi/be2iscsi,qla2xxx: convert to alloc_workqueue()

On 02/02/2011 04:25 AM, Tejun Heo wrote:
> usually necessary. The queue of pending works tend to be short and
> consumed pretty fast.
>

What if we want to do something that could take a while, like doing
recovery of a device/transport (so you have to send resets and
logouts/logins and wait for the results but they could take a while if
they timeout), should we be using something other than a workqueue so it
does not interfere with other users?

2011-02-03 09:31:27

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH 16/32] scsi/be2iscsi,qla2xxx: convert to alloc_workqueue()

Hello, Mike.

On Wed, Feb 02, 2011 at 02:41:22PM -0600, Mike Christie wrote:
> On 02/02/2011 04:25 AM, Tejun Heo wrote:
> >usually necessary. The queue of pending works tend to be short and
> >consumed pretty fast.
>
> What if we want to do something that could take a while, like doing
> recovery of a device/transport (so you have to send resets and
> logouts/logins and wait for the results but they could take a while
> if they timeout), should we be using something other than a
> workqueue so it does not interfere with other users?

As long as you don't use ordered workqueue, it wouldn't be a problem.
The max concurrency is determined by @max_active parameter to
alloc_workqueue() and workqueue will try to provide concurrency upto
the limit on demand as long as resources are available.

Thanks.

--
tejun