From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932898AbbLVOsb (ORCPT ); Tue, 22 Dec 2015 09:48:31 -0500 Received: from mx2.suse.de ([195.135.220.15]:54939 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932243AbbLVOs1 (ORCPT ); Tue, 22 Dec 2015 09:48:27 -0500 Subject: Re: [PATCH v3 20/77] ncr5380: Introduce unbound workqueue To: Finn Thain References: <20151222011737.980475848@telegraphics.com.au> <20151222011743.455500361@telegraphics.com.au> <5678F776.8080305@suse.de> Cc: James Bottomley , Michael Schmitz , linux-m68k@vger.kernel.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, "Martin K. Petersen" , Russell King , linux-arm-kernel@lists.infradead.org From: Hannes Reinecke Message-ID: <567962B8.9080101@suse.de> Date: Tue, 22 Dec 2015 15:48:24 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/22/2015 01:44 PM, Finn Thain wrote: > > On Tue, 22 Dec 2015, Hannes Reinecke wrote: > >> On 12/22/2015 02:17 AM, Finn Thain wrote: >>> Allocate a work queue that will permit busy waiting and sleeping. This >>> means NCR5380_init() can potentially fail, so add this error path. >>> >>> Signed-off-by: Finn Thain >>> >>> --- >>> >>> In subsequent patches, the work function adopts this work queue so it >>> can sleep while polling, which allows the removal of some flawed and >>> complicated code in NCR5380_select() in NCR5380.c. >>> >>> Changed since v1: >>> - Dropped WQ_CPU_INTENSIVE flag because Documentation/workqueue.txt says it >>> "is meaningless for unbound wq". >>> >>> --- >>> drivers/scsi/NCR5380.c | 15 +++++++++++---- >>> drivers/scsi/NCR5380.h | 1 + >>> drivers/scsi/arm/cumana_1.c | 8 ++++++-- >>> drivers/scsi/arm/oak.c | 8 ++++++-- >>> drivers/scsi/atari_NCR5380.c | 8 +++++++- >>> drivers/scsi/atari_scsi.c | 5 ++++- >>> drivers/scsi/dmx3191d.c | 17 +++++++++++------ >>> drivers/scsi/dtc.c | 11 +++++++++-- >>> drivers/scsi/g_NCR5380.c | 31 +++++++++++++++---------------- >>> drivers/scsi/mac_scsi.c | 5 ++++- >>> drivers/scsi/pas16.c | 10 ++++++++-- >>> drivers/scsi/sun3_scsi.c | 5 ++++- >>> drivers/scsi/t128.c | 13 ++++++++++--- >>> 13 files changed, 96 insertions(+), 41 deletions(-) >>> >>> Index: linux/drivers/scsi/NCR5380.c >>> =================================================================== >>> --- linux.orig/drivers/scsi/NCR5380.c 2015-12-22 12:15:52.000000000 +1100 >>> +++ linux/drivers/scsi/NCR5380.c 2015-12-22 12:15:56.000000000 +1100 >>> @@ -514,7 +514,7 @@ static int should_disconnect(unsigned ch >>> static void NCR5380_set_timer(struct NCR5380_hostdata *hostdata, unsigned >>> long timeout) >>> { >>> hostdata->time_expires = jiffies + timeout; >>> - schedule_delayed_work(&hostdata->coroutine, timeout); >>> + queue_delayed_work(hostdata->work_q, &hostdata->coroutine, timeout); >>> } >>> >>> >>> @@ -791,7 +791,12 @@ static int NCR5380_init(struct Scsi_Host >>> hostdata->disconnected_queue = NULL; >>> >>> INIT_DELAYED_WORK(&hostdata->coroutine, NCR5380_main); >>> - >>> + hostdata->work_q = alloc_workqueue("ncr5380_%d", >>> + WQ_UNBOUND | WQ_MEM_RECLAIM, >>> + 1, instance->host_no); >>> + if (!hostdata->work_q) >>> + return -ENOMEM; >>> + >>> /* The CHECK code seems to break the 53C400. Will check it later maybe */ >>> if (flags & FLAG_NCR53C400) >>> hostdata->flags = FLAG_HAS_LAST_BYTE_SENT | flags; >> >> Wouldn't it be better to use a normal (ie bound) workqueue here? > > The polling algorithm I've used requires that the workqueue item is free > to busy-wait and sleep. Perhaps a kthread_worker would be better? > >> SCSI-2 is pretty much single-threaded, so shifting things onto arbitrary >> CPUs don't sound very appealing. > > Most of these drivers only run on UP systems. For the x86 drivers, I > suspect that the cache miss penalty would be insignificant compared to > some of the other overheads. The 5380 chip requires that the CPU is > involved in SCSI bus signalling and merely accessing a chip register > takes over a microsecond. > I know. But using a bound workqueue would mean you could use 'create_workqueue()' instead of open-coding it :-) But in the end it's up to you. If the thing works I'm not that concerned. Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)