| Message ID | 1654879602-33497-4-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | pm8001 driver improvements | expand |
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. Cheers, Hannes
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
> > After the patch, spinlock guarantees atomicity for both alloc and free. > > Not sure there is any gain from this. +1
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
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); } /** @@ -76,9 +80,9 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag) */ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) { - unsigned int tag; void *bitmap = pm8001_ha->tags; unsigned long flags; + unsigned int tag; spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags); tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num); @@ -86,7 +90,7 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out) spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags); return -SAS_QUEUE_FULL; } - set_bit(tag, bitmap); + __set_bit(tag, bitmap); spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags); *tag_out = tag; return 0;
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(-)