Messages in this thread | | | Date | Mon, 20 Jun 2022 08:40:43 +0100 | Subject | Re: [PATCH 3/4] scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free | From | John Garry <> |
| |
On 20/06/2022 07:07, Damien Le Moal wrote: > 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().
Right, so I could change to use __set_bit() in pm8001_find_tag(), but rather use spinlock always.
> > After the patch, spinlock guarantees atomicity for both alloc and free. > > Not sure there is any gain from this.
A few more points to note: - On architectures which do not support atomic operations natively, they have to use global spinlocks to create atomic context before doing non-atomic bit clearing - see atomic64.c . As such, it's better to use the already available pm8001_ha->bitmap_lock. - spinlock does more than create atomic context, but also has barrier semantics, so proper to use consistently for protecting the same region.
Thanks, John
| |