From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jens Axboe Subject: Re: [PATCH, RFC] scsi: use host wide tags by default Date: Fri, 17 Apr 2015 15:47:50 -0600 Message-ID: <55317F86.7030509@kernel.dk> References: <1429301471-5666-1-git-send-email-hch@lst.de> <1429306960.1079.25.camel@HansenPartnership.com> <55317EAE.10201@kernel.dk> <1429307160.1079.27.camel@HansenPartnership.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f52.google.com ([209.85.220.52]:35230 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753752AbbDQVrw (ORCPT ); Fri, 17 Apr 2015 17:47:52 -0400 Received: by pabtp1 with SMTP id tp1so137809631pab.2 for ; Fri, 17 Apr 2015 14:47:52 -0700 (PDT) In-Reply-To: <1429307160.1079.27.camel@HansenPartnership.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: James Bottomley Cc: Christoph Hellwig , linux-scsi@vger.kernel.org On 04/17/2015 03:46 PM, James Bottomley wrote: > On Fri, 2015-04-17 at 15:44 -0600, Jens Axboe wrote: >> On 04/17/2015 03:42 PM, James Bottomley wrote: >>>> @@ -662,32 +662,14 @@ void scsi_finish_command(struct scsi_cmnd *cmd) >>>> */ >>>> int scsi_change_queue_depth(struct scsi_device *sdev, int depth) >>>> { >>>> - unsigned long flags; >>>> - >>>> - if (depth <= 0) >>>> - goto out; >>>> - >>>> - spin_lock_irqsave(sdev->request_queue->queue_lock, flags); >>>> + if (depth > 0) { >>>> + unsigned long flags; >>>> >>>> - /* >>>> - * Check to see if the queue is managed by the block layer. >>>> - * If it is, and we fail to adjust the depth, exit. >>>> - * >>>> - * Do not resize the tag map if it is a host wide share bqt, >>>> - * because the size should be the hosts's can_queue. If there >>>> - * is more IO than the LLD's can_queue (so there are not enuogh >>>> - * tags) request_fn's host queue ready check will handle it. >>>> - */ >>>> - if (!shost_use_blk_mq(sdev->host) && !sdev->host->bqt) { >>>> - if (blk_queue_tagged(sdev->request_queue) && >>>> - blk_queue_resize_tags(sdev->request_queue, depth) != 0) >>>> - goto out_unlock; >>>> + spin_lock_irqsave(sdev->request_queue->queue_lock, flags); >>>> + sdev->queue_depth = depth; >>>> + spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags); >>> >>> This lock/unlock is a nasty global sync point which can be eliminated: >>> we can rely on the architectural atomicity of 32 bit writes (might need >>> to make sdev->queue_depth a u32 because I seem to remember 16 bit writes >>> had to be done as two byte stores on some architectures). >> >> It's not in a hot path (by any stretch), so doesn't really matter... > > Sure, but it's good practise not to do this, otherwise the pattern > lock/u32 store/unlock gets duplicated into hot paths by people who are > confused about whether locking is required. It's a lot saner default to lock/unlock and have people copy that, than have them misguidedly think that no locking is require for whatever reason. The write itself might be atomic, but you still need to guarantee visibility. For something like this init style case, I would not try and do anything clever... -- Jens Axboe