| Message ID | 20250225215146.4937-2-kartilak@cisco.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] scsi: fnic: Replace fnic->lock_flags with local flags | expand |
On Tue, Feb 25, 2025 at 01:51:46PM -0800, Karan Tilak Kumar wrote: > diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c > index 8843d9486dbb..6530298733f0 100644 > --- a/drivers/scsi/fnic/fdls_disc.c > +++ b/drivers/scsi/fnic/fdls_disc.c > @@ -311,36 +311,30 @@ void fdls_schedule_oxid_free_retry_work(struct work_struct *work) > unsigned long flags; > int idx; > > - spin_lock_irqsave(&fnic->fnic_lock, flags); > - > for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) { > > FNIC_FCS_DBG(KERN_INFO, fnic->host, fnic->fnic_num, > "Schedule oxid free. oxid idx: %d\n", idx); > > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL); > - spin_lock_irqsave(&fnic->fnic_lock, flags); > - > if (!reclaim_entry) { > schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry, > msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME)); > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > return; > } > > if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) { We discussed this earlier and I really should have brought it up then, but what is this check about? We "know" (scare quotes) that it is true because we're inside a for_each_set_bit() loop. I had assumed it was to test for race conditions so that's why I put it under the lock. If the locking doesn't matter then we could just do a clear_bit() without doing a second test. regards, dan carpenter > reclaim_entry->oxid_idx = idx; > reclaim_entry->expires = round_jiffies(jiffies + delay_j); > + spin_lock_irqsave(&fnic->fnic_lock, flags); > list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list); > + spin_unlock_irqrestore(&fnic->fnic_lock, flags); > schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j); > } else { > /* unlikely scenario, free the allocated memory and continue */ > kfree(reclaim_entry); > } > } > - > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > } > > static bool fdls_is_oxid_fabric_req(uint16_t oxid) > -- > 2.47.1
On Tuesday, February 25, 2025 11:32 PM, Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Tue, Feb 25, 2025 at 01:51:46PM -0800, Karan Tilak Kumar wrote: > > diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c > > index 8843d9486dbb..6530298733f0 100644 > > --- a/drivers/scsi/fnic/fdls_disc.c > > +++ b/drivers/scsi/fnic/fdls_disc.c > > @@ -311,36 +311,30 @@ void fdls_schedule_oxid_free_retry_work(struct work_struct *work) > > unsigned long flags; > > int idx; > > > > - spin_lock_irqsave(&fnic->fnic_lock, flags); > > - > > for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) { > > > > FNIC_FCS_DBG(KERN_INFO, fnic->host, fnic->fnic_num, > > "Schedule oxid free. oxid idx: %d\n", idx); > > > > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > > reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL); > > - spin_lock_irqsave(&fnic->fnic_lock, flags); > > - > > if (!reclaim_entry) { > > schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry, > > msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME)); > > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > > return; > > } > > > > if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) { > > We discussed this earlier and I really should have brought it up then, > but what is this check about? We "know" (scare quotes) that it is true > because we're inside a for_each_set_bit() loop. I had assumed it was to > test for race conditions so that's why I put it under the lock. If the > locking doesn't matter then we could just do a clear_bit() without doing > a second test. > > regards, > dan carpenter > > > reclaim_entry->oxid_idx = idx; > > reclaim_entry->expires = round_jiffies(jiffies + delay_j); > > + spin_lock_irqsave(&fnic->fnic_lock, flags); > > list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list); > > + spin_unlock_irqrestore(&fnic->fnic_lock, flags); > > schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j); > > } else { > > /* unlikely scenario, free the allocated memory and continue */ > > kfree(reclaim_entry); > > } > > } > > > - > > - spin_unlock_irqrestore(&fnic->fnic_lock, flags); > > } > > > > static bool fdls_is_oxid_fabric_req(uint16_t oxid) > > -- > > 2.47.1 > Thanks for your review and comments, Dan. You are right; We reviewed it, and locking is only needed to add to the reclaim list. We can use clear_bit. I'll send out a revised patch for this. Regards, Karan (on behalf of the Cisco team)
diff --git a/drivers/scsi/fnic/fdls_disc.c b/drivers/scsi/fnic/fdls_disc.c index 8843d9486dbb..6530298733f0 100644 --- a/drivers/scsi/fnic/fdls_disc.c +++ b/drivers/scsi/fnic/fdls_disc.c @@ -311,36 +311,30 @@ void fdls_schedule_oxid_free_retry_work(struct work_struct *work) unsigned long flags; int idx; - spin_lock_irqsave(&fnic->fnic_lock, flags); - for_each_set_bit(idx, oxid_pool->pending_schedule_free, FNIC_OXID_POOL_SZ) { FNIC_FCS_DBG(KERN_INFO, fnic->host, fnic->fnic_num, "Schedule oxid free. oxid idx: %d\n", idx); - spin_unlock_irqrestore(&fnic->fnic_lock, flags); reclaim_entry = kzalloc(sizeof(*reclaim_entry), GFP_KERNEL); - spin_lock_irqsave(&fnic->fnic_lock, flags); - if (!reclaim_entry) { schedule_delayed_work(&oxid_pool->schedule_oxid_free_retry, msecs_to_jiffies(SCHEDULE_OXID_FREE_RETRY_TIME)); - spin_unlock_irqrestore(&fnic->fnic_lock, flags); return; } if (test_and_clear_bit(idx, oxid_pool->pending_schedule_free)) { reclaim_entry->oxid_idx = idx; reclaim_entry->expires = round_jiffies(jiffies + delay_j); + spin_lock_irqsave(&fnic->fnic_lock, flags); list_add_tail(&reclaim_entry->links, &oxid_pool->oxid_reclaim_list); + spin_unlock_irqrestore(&fnic->fnic_lock, flags); schedule_delayed_work(&oxid_pool->oxid_reclaim_work, delay_j); } else { /* unlikely scenario, free the allocated memory and continue */ kfree(reclaim_entry); } } - - spin_unlock_irqrestore(&fnic->fnic_lock, flags); } static bool fdls_is_oxid_fabric_req(uint16_t oxid)
Remove unnecessary locking and unlocking of spinlock in fdls_schedule_oxid_free_retry_work. This will shorten the time in the critical section. Suggested-by: Dan Carpenter <dan.carpenter@linaro.org> Fixes: a63e78eb2b0f ("scsi: fnic: Add support for fabric based solicited requests and responses") Reviewed-by: Sesidhar Baddela <sebaddel@cisco.com> Reviewed-by: Arulprabhu Ponnusamy <arulponn@cisco.com> Reviewed-by: Gian Carlo Boffa <gcboffa@cisco.com> Reviewed-by: Arun Easi <aeasi@cisco.com> Tested-by: Karan Tilak Kumar <kartilak@cisco.com> Signed-off-by: Karan Tilak Kumar <kartilak@cisco.com> --- drivers/scsi/fnic/fdls_disc.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)