From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomas Henzl Subject: Re: [PATCH 1/3] hpsa: remove unneeded loop Date: Thu, 01 Aug 2013 16:05:20 +0200 Message-ID: <51FA6B20.2090005@redhat.com> References: <51FA5E7A.9050000@redhat.com> <20130801133951.GX24664@beardog.cce.hp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:57746 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756485Ab3HAOFv (ORCPT ); Thu, 1 Aug 2013 10:05:51 -0400 In-Reply-To: <20130801133951.GX24664@beardog.cce.hp.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: scameron@beardog.cce.hp.com Cc: "'linux-scsi@vger.kernel.org'" , stephenmcameron@gmail.com, mikem@beardog.cce.hp.com On 08/01/2013 03:39 PM, scameron@beardog.cce.hp.com wrote: > On Thu, Aug 01, 2013 at 03:11:22PM +0200, Tomas Henzl wrote: >> From: Tomas Henzl >> >> The cmd_pool_bits is protected everywhere with a spinlock, >> we don't need the test_and_set_bit, set_bit is enough and the loop >> can be removed too. >> >> Signed-off-by: Tomas Henzl >> --- >> drivers/scsi/hpsa.c | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c >> index 796482b..d7df01e 100644 >> --- a/drivers/scsi/hpsa.c >> +++ b/drivers/scsi/hpsa.c >> @@ -2662,15 +2662,12 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h) >> unsigned long flags; >> >> spin_lock_irqsave(&h->lock, flags); >> - do { >> - i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds); >> - if (i == h->nr_cmds) { >> - spin_unlock_irqrestore(&h->lock, flags); >> - return NULL; >> - } >> - } while (test_and_set_bit >> - (i & (BITS_PER_LONG - 1), >> - h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0); >> + i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds); >> + if (i == h->nr_cmds) { >> + spin_unlock_irqrestore(&h->lock, flags); >> + return NULL; >> + } >> + set_bit(i & (BITS_PER_LONG - 1), h->cmd_pool_bits + (i / BITS_PER_LONG)); >> h->nr_allocs++; >> spin_unlock_irqrestore(&h->lock, flags); >> >> -- >> 1.8.3.1 >> > Would it be better instead to just not use the spinlock for protecting > cmd_pool_bits? I have thought about doing this for awhile, but haven't > gotten around to it. > > I think the while loop is safe without the spin lock. And then it is > not needed in cmd_free either. I was evaluating the same idea for a while too, a loop and inside just the test_and_set_bit, maybe even a stored value to start with a likely empty bit from last time to tune it a bit. But I know almost nothing about the use pattern, so I decided for the least invasive change to the existing code, to not make it worse. > > -- steve > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html