Re: [PATCH 3/4] scsi: pm8001: Use non-atomic bitmap ops for tag alloc + free

[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@xxxxxxxxxx>
> > > ---
> > >    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