linux-kernel - Re: [PATCH v3 20/77] ncr5380: Introduce unbound workqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 22 Dec 2015 23:44:21 +1100 (AEDT)
From:	Finn Thain <fthain@...egraphics.com.au>
To:	Hannes Reinecke <hare@...e.de>
cc:	"James E.J. Bottomley" <JBottomley@...n.com>,
	Michael Schmitz <schmitzmic@...il.com>,
	linux-m68k@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Russell King <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 20/77] ncr5380: Introduce unbound workqueue


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 <fthain@...egraphics.com.au>
> >
> > ---
> >
> > 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.

-- 

> 
> Cheers,
> 
> Hannes
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/