Messages in this thread | | | Date | Mon, 20 Jun 2022 15:07:06 +0900 | Subject | Re: [PATCH 3/4] scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free | From | Damien Le Moal <> |
| |
On 6/20/22 15:00, Hannes Reinecke wrote: > On 6/10/22 18:46, John Garry wrote: >> In pm8001_tag_alloc() we don't require atomic set_bit() as we are already >> in atomic context. In pm8001_tag_free() we should use the same host >> spinlock to protect clearing the tag (and then don't require the atomic >> clear_bit()). >> >> Signed-off-by: John Garry <john.garry@huawei.com> >> --- >> drivers/scsi/pm8001/pm8001_sas.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c >> index 3a863d776724..8e3f2f9ddaac 100644 >> --- a/drivers/scsi/pm8001/pm8001_sas.c >> +++ b/drivers/scsi/pm8001/pm8001_sas.c >> @@ -66,7 +66,11 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag) >> void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag) >> { >> void *bitmap = pm8001_ha->tags; >> - clear_bit(tag, bitmap); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags); >> + __clear_bit(tag, bitmap); >> + spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags); >> } >> > This spin lock is pretty much pointless; clear_bit() is always atomic.
But __clear_bit() is not atomic. I think it was the point of this patch, to not use atomics and use the spinlock instead to protect bitmap.
Before the patch, pm8001_tag_alloc() takes the spinlock *and* use the atomic set_bit(), which is an overkill. pm8001_tag_free() only clears the bit using the the atomic clear_bit().
After the patch, spinlock guarantees atomicity for both alloc and free.
Not sure there is any gain from this.
> > Cheers, > > Hannes
-- Damien Le Moal Western Digital Research
| |