* [PATCH 0/2] Fix sparse warnings in scsi_debug @ 2022-02-25 8:45 Damien Le Moal 2022-02-25 8:45 ` [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings Damien Le Moal 2022-02-25 8:45 ` [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() Damien Le Moal 0 siblings, 2 replies; 10+ messages in thread From: Damien Le Moal @ 2022-02-25 8:45 UTC (permalink / raw) To: linux-scsi, Martin K . Petersen; +Cc: Douglas Gilbert A couple of patches to suppress sparse warnings in scsi_debug. No functional changes. Damien Le Moal (2): scsi: scsi_debug: silence sparse unexpected unlock warnings scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() drivers/scsi/scsi_debug.c | 87 ++++++++++++++++++++++++--------------- 1 file changed, 53 insertions(+), 34 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings 2022-02-25 8:45 [PATCH 0/2] Fix sparse warnings in scsi_debug Damien Le Moal @ 2022-02-25 8:45 ` Damien Le Moal 2022-02-28 1:39 ` Douglas Gilbert 2022-02-25 8:45 ` [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() Damien Le Moal 1 sibling, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2022-02-25 8:45 UTC (permalink / raw) To: linux-scsi, Martin K . Petersen; +Cc: Douglas Gilbert The return statement inside the sdeb_read_lock(), sdeb_read_unlock(), sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to many warnings about unexpected unlocks in the resp_xxx() functions. Modify the lock/unlock functions using the __acquire() and __release() inline annotations for the sdebug_no_rwlock == true case to avoid these warnings. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 0d25b30922ef..f4e97f2224b2 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec, static inline void sdeb_read_lock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - read_lock(&sip->macc_lck); - else - read_lock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __acquire(&sip->macc_lck); + else + __acquire(&sdeb_fake_rw_lck); + } else { + if (sip) + read_lock(&sip->macc_lck); + else + read_lock(&sdeb_fake_rw_lck); + } } static inline void sdeb_read_unlock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - read_unlock(&sip->macc_lck); - else - read_unlock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __release(&sip->macc_lck); + else + __release(&sdeb_fake_rw_lck); + } else { + if (sip) + read_unlock(&sip->macc_lck); + else + read_unlock(&sdeb_fake_rw_lck); + } } static inline void sdeb_write_lock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - write_lock(&sip->macc_lck); - else - write_lock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __acquire(&sip->macc_lck); + else + __acquire(&sdeb_fake_rw_lck); + } else { + if (sip) + write_lock(&sip->macc_lck); + else + write_lock(&sdeb_fake_rw_lck); + } } static inline void sdeb_write_unlock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - write_unlock(&sip->macc_lck); - else - write_unlock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __release(&sip->macc_lck); + else + __release(&sdeb_fake_rw_lck); + } else { + if (sip) + write_unlock(&sip->macc_lck); + else + write_unlock(&sdeb_fake_rw_lck); + } } static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) -- 2.35.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings 2022-02-25 8:45 ` [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings Damien Le Moal @ 2022-02-28 1:39 ` Douglas Gilbert 2022-02-28 6:58 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: Douglas Gilbert @ 2022-02-28 1:39 UTC (permalink / raw) To: Damien Le Moal, linux-scsi, Martin K . Petersen On 2022-02-25 03:45, Damien Le Moal wrote: > The return statement inside the sdeb_read_lock(), sdeb_read_unlock(), > sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to > many warnings about unexpected unlocks in the resp_xxx() functions. > > Modify the lock/unlock functions using the __acquire() and __release() > inline annotations for the sdebug_no_rwlock == true case to avoid these > warnings. I'm confused. The idea with sdebug_no_rwlock was that the application may know that the protection afforded by the driver's rwlock is not needed because locking is performed at a higher level (e.g. in the user space). Hence there is no need to use a read-write lock (or a full lock) in this driver to protect a read (say) against a co-incident write to the same memory region. So this was a performance enhancement. The proposed patch seems to be replacing a read-write lock with a full lock. That would be going in the opposite direction to what I intended. If this is the only solution, a better idea might be to drop the patch (in staging I think) that introduced the sdebug_no_rwlock option. The sdebug_no_rwlock option has been pretty thoroughly tested (for over a year) with memory to memory transfers (together with sgl to sgl additions to lib/scatterlist.h). Haven't seen any unexplained crashes that I could trace to this lack of locking. OTOH I haven't measured any improvement of the copy speed either, that may be because my tests are approaching the copy bandwidth of the ram. Does sparse understand guard variables (e.g. like 'bool lock_taken')? From what I've seen with sg3_utils Coverity doesn't, leading to many false reports. Doug Gilbert > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++-------------- > 1 file changed, 44 insertions(+), 24 deletions(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index 0d25b30922ef..f4e97f2224b2 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec, > static inline void > sdeb_read_lock(struct sdeb_store_info *sip) > { > - if (sdebug_no_rwlock) > - return; > - if (sip) > - read_lock(&sip->macc_lck); > - else > - read_lock(&sdeb_fake_rw_lck); > + if (sdebug_no_rwlock) { > + if (sip) > + __acquire(&sip->macc_lck); > + else > + __acquire(&sdeb_fake_rw_lck); > + } else { > + if (sip) > + read_lock(&sip->macc_lck); > + else > + read_lock(&sdeb_fake_rw_lck); > + } > } > > static inline void > sdeb_read_unlock(struct sdeb_store_info *sip) > { > - if (sdebug_no_rwlock) > - return; > - if (sip) > - read_unlock(&sip->macc_lck); > - else > - read_unlock(&sdeb_fake_rw_lck); > + if (sdebug_no_rwlock) { > + if (sip) > + __release(&sip->macc_lck); > + else > + __release(&sdeb_fake_rw_lck); > + } else { > + if (sip) > + read_unlock(&sip->macc_lck); > + else > + read_unlock(&sdeb_fake_rw_lck); > + } > } > > static inline void > sdeb_write_lock(struct sdeb_store_info *sip) > { > - if (sdebug_no_rwlock) > - return; > - if (sip) > - write_lock(&sip->macc_lck); > - else > - write_lock(&sdeb_fake_rw_lck); > + if (sdebug_no_rwlock) { > + if (sip) > + __acquire(&sip->macc_lck); > + else > + __acquire(&sdeb_fake_rw_lck); > + } else { > + if (sip) > + write_lock(&sip->macc_lck); > + else > + write_lock(&sdeb_fake_rw_lck); > + } > } > > static inline void > sdeb_write_unlock(struct sdeb_store_info *sip) > { > - if (sdebug_no_rwlock) > - return; > - if (sip) > - write_unlock(&sip->macc_lck); > - else > - write_unlock(&sdeb_fake_rw_lck); > + if (sdebug_no_rwlock) { > + if (sip) > + __release(&sip->macc_lck); > + else > + __release(&sdeb_fake_rw_lck); > + } else { > + if (sip) > + write_unlock(&sip->macc_lck); > + else > + write_unlock(&sdeb_fake_rw_lck); > + } > } > > static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings 2022-02-28 1:39 ` Douglas Gilbert @ 2022-02-28 6:58 ` Damien Le Moal 2022-02-28 22:45 ` Douglas Gilbert 0 siblings, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2022-02-28 6:58 UTC (permalink / raw) To: dgilbert, linux-scsi, Martin K . Petersen On 2022/02/28 3:39, Douglas Gilbert wrote: > On 2022-02-25 03:45, Damien Le Moal wrote: >> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(), >> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to >> many warnings about unexpected unlocks in the resp_xxx() functions. >> >> Modify the lock/unlock functions using the __acquire() and __release() >> inline annotations for the sdebug_no_rwlock == true case to avoid these >> warnings. > > I'm confused. The idea with sdebug_no_rwlock was that the application > may know that the protection afforded by the driver's rwlock is not > needed because locking is performed at a higher level (e.g. in the > user space). Hence there is no need to use a read-write lock (or a > full lock) in this driver to protect a read (say) against a co-incident > write to the same memory region. So this was a performance enhancement. > > The proposed patch seems to be replacing a read-write lock with a full > lock. That would be going in the opposite direction to what I intended. Not at all. The __acquire() and __release() calls are not locking functions. They are annotations for sparse so that we get a correct +/-1 counting of the lock/unlock calls. So there is no functional change here and no overhead added when compiling without C=1 since these macros disappear without sparse. > > If this is the only solution, a better idea might be to drop the > patch (in staging I think) that introduced the sdebug_no_rwlock option. > > The sdebug_no_rwlock option has been pretty thoroughly tested (for over > a year) with memory to memory transfers (together with sgl to sgl > additions to lib/scatterlist.h). Haven't seen any unexplained crashes > that I could trace to this lack of locking. OTOH I haven't measured > any improvement of the copy speed either, that may be because my tests > are approaching the copy bandwidth of the ram. > > > Does sparse understand guard variables (e.g. like 'bool lock_taken')? > From what I've seen with sg3_utils Coverity doesn't, leading to many false > reports. > > Doug Gilbert > >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++-------------- >> 1 file changed, 44 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >> index 0d25b30922ef..f4e97f2224b2 100644 >> --- a/drivers/scsi/scsi_debug.c >> +++ b/drivers/scsi/scsi_debug.c >> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec, >> static inline void >> sdeb_read_lock(struct sdeb_store_info *sip) >> { >> - if (sdebug_no_rwlock) >> - return; >> - if (sip) >> - read_lock(&sip->macc_lck); >> - else >> - read_lock(&sdeb_fake_rw_lck); >> + if (sdebug_no_rwlock) { >> + if (sip) >> + __acquire(&sip->macc_lck); >> + else >> + __acquire(&sdeb_fake_rw_lck); >> + } else { >> + if (sip) >> + read_lock(&sip->macc_lck); >> + else >> + read_lock(&sdeb_fake_rw_lck); >> + } >> } >> >> static inline void >> sdeb_read_unlock(struct sdeb_store_info *sip) >> { >> - if (sdebug_no_rwlock) >> - return; >> - if (sip) >> - read_unlock(&sip->macc_lck); >> - else >> - read_unlock(&sdeb_fake_rw_lck); >> + if (sdebug_no_rwlock) { >> + if (sip) >> + __release(&sip->macc_lck); >> + else >> + __release(&sdeb_fake_rw_lck); >> + } else { >> + if (sip) >> + read_unlock(&sip->macc_lck); >> + else >> + read_unlock(&sdeb_fake_rw_lck); >> + } >> } >> >> static inline void >> sdeb_write_lock(struct sdeb_store_info *sip) >> { >> - if (sdebug_no_rwlock) >> - return; >> - if (sip) >> - write_lock(&sip->macc_lck); >> - else >> - write_lock(&sdeb_fake_rw_lck); >> + if (sdebug_no_rwlock) { >> + if (sip) >> + __acquire(&sip->macc_lck); >> + else >> + __acquire(&sdeb_fake_rw_lck); >> + } else { >> + if (sip) >> + write_lock(&sip->macc_lck); >> + else >> + write_lock(&sdeb_fake_rw_lck); >> + } >> } >> >> static inline void >> sdeb_write_unlock(struct sdeb_store_info *sip) >> { >> - if (sdebug_no_rwlock) >> - return; >> - if (sip) >> - write_unlock(&sip->macc_lck); >> - else >> - write_unlock(&sdeb_fake_rw_lck); >> + if (sdebug_no_rwlock) { >> + if (sip) >> + __release(&sip->macc_lck); >> + else >> + __release(&sdeb_fake_rw_lck); >> + } else { >> + if (sip) >> + write_unlock(&sip->macc_lck); >> + else >> + write_unlock(&sdeb_fake_rw_lck); >> + } >> } >> >> static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings 2022-02-28 6:58 ` Damien Le Moal @ 2022-02-28 22:45 ` Douglas Gilbert 0 siblings, 0 replies; 10+ messages in thread From: Douglas Gilbert @ 2022-02-28 22:45 UTC (permalink / raw) To: Damien Le Moal, linux-scsi, Martin K . Petersen On 2022-02-28 01:58, Damien Le Moal wrote: > On 2022/02/28 3:39, Douglas Gilbert wrote: >> On 2022-02-25 03:45, Damien Le Moal wrote: >>> The return statement inside the sdeb_read_lock(), sdeb_read_unlock(), >>> sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to >>> many warnings about unexpected unlocks in the resp_xxx() functions. >>> >>> Modify the lock/unlock functions using the __acquire() and __release() >>> inline annotations for the sdebug_no_rwlock == true case to avoid these >>> warnings. >> >> I'm confused. The idea with sdebug_no_rwlock was that the application >> may know that the protection afforded by the driver's rwlock is not >> needed because locking is performed at a higher level (e.g. in the >> user space). Hence there is no need to use a read-write lock (or a >> full lock) in this driver to protect a read (say) against a co-incident >> write to the same memory region. So this was a performance enhancement. >> >> The proposed patch seems to be replacing a read-write lock with a full >> lock. That would be going in the opposite direction to what I intended. > > Not at all. The __acquire() and __release() calls are not locking functions. > They are annotations for sparse so that we get a correct +/-1 counting of the > lock/unlock calls. So there is no functional change here and no overhead added > when compiling without C=1 since these macros disappear without sparse. Grrr. If those functions are dummies then I think it would be reasonable if their names had a word like "fake" or "dummy" in them. That being the case: Acked-by: Douglas Gilbert <dgilbert@interlog.com> Note: these patches should probably be against Martin's 5.18/scsi-staging tree as he has taken 5 or 6 of my scsi_debug patches in this cycle. > >> >> If this is the only solution, a better idea might be to drop the >> patch (in staging I think) that introduced the sdebug_no_rwlock option. >> >> The sdebug_no_rwlock option has been pretty thoroughly tested (for over >> a year) with memory to memory transfers (together with sgl to sgl >> additions to lib/scatterlist.h). Haven't seen any unexplained crashes >> that I could trace to this lack of locking. OTOH I haven't measured >> any improvement of the copy speed either, that may be because my tests >> are approaching the copy bandwidth of the ram. >> >> >> Does sparse understand guard variables (e.g. like 'bool lock_taken')? >> From what I've seen with sg3_utils Coverity doesn't, leading to many false >> reports. >> >> Doug Gilbert >> >>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>> --- >>> drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++-------------- >>> 1 file changed, 44 insertions(+), 24 deletions(-) >>> >>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >>> index 0d25b30922ef..f4e97f2224b2 100644 >>> --- a/drivers/scsi/scsi_debug.c >>> +++ b/drivers/scsi/scsi_debug.c >>> @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec, >>> static inline void >>> sdeb_read_lock(struct sdeb_store_info *sip) >>> { >>> - if (sdebug_no_rwlock) >>> - return; >>> - if (sip) >>> - read_lock(&sip->macc_lck); >>> - else >>> - read_lock(&sdeb_fake_rw_lck); >>> + if (sdebug_no_rwlock) { >>> + if (sip) >>> + __acquire(&sip->macc_lck); >>> + else >>> + __acquire(&sdeb_fake_rw_lck); >>> + } else { >>> + if (sip) >>> + read_lock(&sip->macc_lck); >>> + else >>> + read_lock(&sdeb_fake_rw_lck); >>> + } >>> } >>> >>> static inline void >>> sdeb_read_unlock(struct sdeb_store_info *sip) >>> { >>> - if (sdebug_no_rwlock) >>> - return; >>> - if (sip) >>> - read_unlock(&sip->macc_lck); >>> - else >>> - read_unlock(&sdeb_fake_rw_lck); >>> + if (sdebug_no_rwlock) { >>> + if (sip) >>> + __release(&sip->macc_lck); >>> + else >>> + __release(&sdeb_fake_rw_lck); >>> + } else { >>> + if (sip) >>> + read_unlock(&sip->macc_lck); >>> + else >>> + read_unlock(&sdeb_fake_rw_lck); >>> + } >>> } >>> >>> static inline void >>> sdeb_write_lock(struct sdeb_store_info *sip) >>> { >>> - if (sdebug_no_rwlock) >>> - return; >>> - if (sip) >>> - write_lock(&sip->macc_lck); >>> - else >>> - write_lock(&sdeb_fake_rw_lck); >>> + if (sdebug_no_rwlock) { >>> + if (sip) >>> + __acquire(&sip->macc_lck); >>> + else >>> + __acquire(&sdeb_fake_rw_lck); >>> + } else { >>> + if (sip) >>> + write_lock(&sip->macc_lck); >>> + else >>> + write_lock(&sdeb_fake_rw_lck); >>> + } >>> } >>> >>> static inline void >>> sdeb_write_unlock(struct sdeb_store_info *sip) >>> { >>> - if (sdebug_no_rwlock) >>> - return; >>> - if (sip) >>> - write_unlock(&sip->macc_lck); >>> - else >>> - write_unlock(&sdeb_fake_rw_lck); >>> + if (sdebug_no_rwlock) { >>> + if (sip) >>> + __release(&sip->macc_lck); >>> + else >>> + __release(&sdeb_fake_rw_lck); >>> + } else { >>> + if (sip) >>> + write_unlock(&sip->macc_lck); >>> + else >>> + write_unlock(&sdeb_fake_rw_lck); >>> + } >>> } >>> >>> static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip) >> > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() 2022-02-25 8:45 [PATCH 0/2] Fix sparse warnings in scsi_debug Damien Le Moal 2022-02-25 8:45 ` [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings Damien Le Moal @ 2022-02-25 8:45 ` Damien Le Moal 2022-02-28 2:05 ` Douglas Gilbert 1 sibling, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2022-02-25 8:45 UTC (permalink / raw) To: linux-scsi, Martin K . Petersen; +Cc: Douglas Gilbert The use of the locked boolean variable to control locking and unlocking of the qc_lock of struct sdebug_queue confuses sparse, leading to a warning about an unexpected unlock. Simplify the qc_lock lock/unlock handling code of this function to avoid this warning by removing the locked boolean variable. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/scsi_debug.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index f4e97f2224b2..acb32f3e38eb 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) { bool first; bool retiring = false; - bool locked = false; int num_entries = 0; unsigned int qc_idx = 0; unsigned long iflags; @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) if (qc_idx >= sdebug_max_queue) return 0; + spin_lock_irqsave(&sqp->qc_lock, iflags); + for (first = true; first || qc_idx + 1 < sdebug_max_queue; ) { - if (!locked) { - spin_lock_irqsave(&sqp->qc_lock, iflags); - locked = true; - } if (first) { first = false; if (!test_bit(qc_idx, sqp->in_use_bm)) continue; - } else { - qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1); } + + qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, + qc_idx + 1); if (qc_idx >= sdebug_max_queue) break; @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) } WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE); spin_unlock_irqrestore(&sqp->qc_lock, iflags); - locked = false; scsi_done(scp); /* callback to mid level */ num_entries++; + spin_lock_irqsave(&sqp->qc_lock, iflags); if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue) break; /* if no more then exit without retaking spinlock */ } - if (locked) - spin_unlock_irqrestore(&sqp->qc_lock, iflags); + + spin_unlock_irqrestore(&sqp->qc_lock, iflags); + if (num_entries > 0) atomic_add(num_entries, &sdeb_mq_poll_count); return num_entries; -- 2.35.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() 2022-02-25 8:45 ` [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() Damien Le Moal @ 2022-02-28 2:05 ` Douglas Gilbert 2022-02-28 7:07 ` Damien Le Moal 2022-02-28 13:46 ` Damien Le Moal 0 siblings, 2 replies; 10+ messages in thread From: Douglas Gilbert @ 2022-02-28 2:05 UTC (permalink / raw) To: Damien Le Moal, linux-scsi, Martin K . Petersen On 2022-02-25 03:45, Damien Le Moal wrote: > The use of the locked boolean variable to control locking and unlocking > of the qc_lock of struct sdebug_queue confuses sparse, leading to a > warning about an unexpected unlock. Simplify the qc_lock lock/unlock > handling code of this function to avoid this warning by removing the > locked boolean variable. See below. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/scsi/scsi_debug.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c > index f4e97f2224b2..acb32f3e38eb 100644 > --- a/drivers/scsi/scsi_debug.c > +++ b/drivers/scsi/scsi_debug.c > @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) > { > bool first; > bool retiring = false; > - bool locked = false; > int num_entries = 0; > unsigned int qc_idx = 0; > unsigned long iflags; > @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) > if (qc_idx >= sdebug_max_queue) > return 0; > > + spin_lock_irqsave(&sqp->qc_lock, iflags); > + > for (first = true; first || qc_idx + 1 < sdebug_max_queue; ) { > - if (!locked) { > - spin_lock_irqsave(&sqp->qc_lock, iflags); > - locked = true; > - } > if (first) { > first = false; > if (!test_bit(qc_idx, sqp->in_use_bm)) > continue; > - } else { > - qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1); > } > + > + qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, > + qc_idx + 1); The original logic is wrong or the above line is wrong. find_next_bit() is not called on the first iteration in the original, but it is with this patch. > if (qc_idx >= sdebug_max_queue) > break; > > @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) > } > WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE); > spin_unlock_irqrestore(&sqp->qc_lock, iflags); > - locked = false; > scsi_done(scp); /* callback to mid level */ > num_entries++; > + spin_lock_irqsave(&sqp->qc_lock, iflags); > if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue) > break; /* if no more then exit without retaking spinlock */ See that comment on the line above? That is the reason for the guard variable. Defying that comment, the modified code does a superfluous spinlock irqsave and irqrestore. Sparse could be taken as a comment on the amount of grey matter that tool has. > } > - if (locked) > - spin_unlock_irqrestore(&sqp->qc_lock, iflags); > + > + spin_unlock_irqrestore(&sqp->qc_lock, iflags); > + > if (num_entries > 0) > atomic_add(num_entries, &sdeb_mq_poll_count); > return num_entries; Locking issues are extremely difficult to analyze via a unified diff of the function. A copy of the original function is required to make any sense of it. Doug Gilbert ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() 2022-02-28 2:05 ` Douglas Gilbert @ 2022-02-28 7:07 ` Damien Le Moal 2022-02-28 13:46 ` Damien Le Moal 1 sibling, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2022-02-28 7:07 UTC (permalink / raw) To: dgilbert, linux-scsi, Martin K . Petersen On 2022/02/28 4:05, Douglas Gilbert wrote: > On 2022-02-25 03:45, Damien Le Moal wrote: >> The use of the locked boolean variable to control locking and unlocking >> of the qc_lock of struct sdebug_queue confuses sparse, leading to a >> warning about an unexpected unlock. Simplify the qc_lock lock/unlock >> handling code of this function to avoid this warning by removing the >> locked boolean variable. > > See below. > >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/scsi/scsi_debug.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >> index f4e97f2224b2..acb32f3e38eb 100644 >> --- a/drivers/scsi/scsi_debug.c >> +++ b/drivers/scsi/scsi_debug.c >> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> { >> bool first; >> bool retiring = false; >> - bool locked = false; >> int num_entries = 0; >> unsigned int qc_idx = 0; >> unsigned long iflags; >> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> if (qc_idx >= sdebug_max_queue) >> return 0; >> >> + spin_lock_irqsave(&sqp->qc_lock, iflags); >> + >> for (first = true; first || qc_idx + 1 < sdebug_max_queue; ) { >> - if (!locked) { >> - spin_lock_irqsave(&sqp->qc_lock, iflags); >> - locked = true; >> - } >> if (first) { >> first = false; >> if (!test_bit(qc_idx, sqp->in_use_bm)) >> continue; >> - } else { >> - qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1); >> } >> + >> + qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, >> + qc_idx + 1); > > The original logic is wrong or the above line is wrong. find_next_bit() is not > called on the first iteration in the original, but it is with this patch. Good catch. Indeed I should not have touched that. Will fix this in V2. > >> if (qc_idx >= sdebug_max_queue) >> break; >> >> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> } >> WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE); >> spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> - locked = false; >> scsi_done(scp); /* callback to mid level */ >> num_entries++; >> + spin_lock_irqsave(&sqp->qc_lock, iflags); >> if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue) >> break; /* if no more then exit without retaking spinlock */ > > See that comment on the line above? That is the reason for the guard variable. > Defying that comment, the modified code does a superfluous spinlock irqsave > and irqrestore. It does, and I should have removed the comment. Without this extra lock/unlock, I could not find a way to shut-up sparse. Do you think this is too much of an overhead ? > Sparse could be taken as a comment on the amount of grey matter that tool has. When it comes to conditional locking, most of the time, sparse does not get it and generates warnings. In this case, I tried the acquire/release annotations, but the loop+locked variable are too much for sparse. I really would like to suppress *all* sparse warnings in scsi. There are way too many right now, which makes this tool nearly useless as new warnings get drowned in the sea of old ones... > > >> } >> - if (locked) >> - spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> + >> + spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> + >> if (num_entries > 0) >> atomic_add(num_entries, &sdeb_mq_poll_count); >> return num_entries; > > Locking issues are extremely difficult to analyze via a unified diff of > the function. A copy of the original function is required to make any > sense of it. Sorry, but I do not understand what you are trying to say here... > > Doug Gilbert > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() 2022-02-28 2:05 ` Douglas Gilbert 2022-02-28 7:07 ` Damien Le Moal @ 2022-02-28 13:46 ` Damien Le Moal 2022-03-01 1:48 ` Douglas Gilbert 1 sibling, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2022-02-28 13:46 UTC (permalink / raw) To: dgilbert, linux-scsi, Martin K . Petersen On 2022/02/28 4:05, Douglas Gilbert wrote: > On 2022-02-25 03:45, Damien Le Moal wrote: >> The use of the locked boolean variable to control locking and unlocking >> of the qc_lock of struct sdebug_queue confuses sparse, leading to a >> warning about an unexpected unlock. Simplify the qc_lock lock/unlock >> handling code of this function to avoid this warning by removing the >> locked boolean variable. > > See below. > >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/scsi/scsi_debug.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >> index f4e97f2224b2..acb32f3e38eb 100644 >> --- a/drivers/scsi/scsi_debug.c >> +++ b/drivers/scsi/scsi_debug.c >> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> { >> bool first; >> bool retiring = false; >> - bool locked = false; >> int num_entries = 0; >> unsigned int qc_idx = 0; >> unsigned long iflags; >> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> if (qc_idx >= sdebug_max_queue) >> return 0; >> >> + spin_lock_irqsave(&sqp->qc_lock, iflags); >> + >> for (first = true; first || qc_idx + 1 < sdebug_max_queue; ) { >> - if (!locked) { >> - spin_lock_irqsave(&sqp->qc_lock, iflags); >> - locked = true; >> - } >> if (first) { >> first = false; >> if (!test_bit(qc_idx, sqp->in_use_bm)) >> continue; >> - } else { >> - qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1); >> } >> + >> + qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, >> + qc_idx + 1); > > The original logic is wrong or the above line is wrong. find_next_bit() is not > called on the first iteration in the original, but it is with this patch. > >> if (qc_idx >= sdebug_max_queue) >> break; >> >> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >> } >> WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE); >> spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> - locked = false; >> scsi_done(scp); /* callback to mid level */ >> num_entries++; >> + spin_lock_irqsave(&sqp->qc_lock, iflags); >> if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue) >> break; /* if no more then exit without retaking spinlock */ > > See that comment on the line above? That is the reason for the guard variable. > Defying that comment, the modified code does a superfluous spinlock irqsave > and irqrestore. Rechecking this, there is one point that is bothering me: is it OK to have the find_first_bit() outside of the sqp lock ? If not, then this is a bug and the extra lock/unlock that my patch add is a fix... > > Sparse could be taken as a comment on the amount of grey matter that tool has. > > >> } >> - if (locked) >> - spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> + >> + spin_unlock_irqrestore(&sqp->qc_lock, iflags); >> + >> if (num_entries > 0) >> atomic_add(num_entries, &sdeb_mq_poll_count); >> return num_entries; > > Locking issues are extremely difficult to analyze via a unified diff of > the function. A copy of the original function is required to make any > sense of it. > > Doug Gilbert > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() 2022-02-28 13:46 ` Damien Le Moal @ 2022-03-01 1:48 ` Douglas Gilbert 0 siblings, 0 replies; 10+ messages in thread From: Douglas Gilbert @ 2022-03-01 1:48 UTC (permalink / raw) To: Damien Le Moal, linux-scsi, Martin K . Petersen See below: On 2022-02-28 08:46, Damien Le Moal wrote: > On 2022/02/28 4:05, Douglas Gilbert wrote: >> On 2022-02-25 03:45, Damien Le Moal wrote: >>> The use of the locked boolean variable to control locking and unlocking >>> of the qc_lock of struct sdebug_queue confuses sparse, leading to a >>> warning about an unexpected unlock. Simplify the qc_lock lock/unlock >>> handling code of this function to avoid this warning by removing the >>> locked boolean variable. >> >> See below. >> >>> >>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >>> --- >>> drivers/scsi/scsi_debug.c | 19 +++++++++---------- >>> 1 file changed, 9 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c >>> index f4e97f2224b2..acb32f3e38eb 100644 >>> --- a/drivers/scsi/scsi_debug.c >>> +++ b/drivers/scsi/scsi_debug.c >>> @@ -7509,7 +7509,6 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >>> { >>> bool first; >>> bool retiring = false; >>> - bool locked = false; >>> int num_entries = 0; >>> unsigned int qc_idx = 0; >>> unsigned long iflags; >>> @@ -7525,18 +7524,17 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >>> if (qc_idx >= sdebug_max_queue) >>> return 0; >>> >>> + spin_lock_irqsave(&sqp->qc_lock, iflags); >>> + >>> for (first = true; first || qc_idx + 1 < sdebug_max_queue; ) { >>> - if (!locked) { >>> - spin_lock_irqsave(&sqp->qc_lock, iflags); >>> - locked = true; >>> - } >>> if (first) { >>> first = false; >>> if (!test_bit(qc_idx, sqp->in_use_bm)) >>> continue; >>> - } else { >>> - qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, qc_idx + 1); >>> } >>> + >>> + qc_idx = find_next_bit(sqp->in_use_bm, sdebug_max_queue, >>> + qc_idx + 1); >> >> The original logic is wrong or the above line is wrong. find_next_bit() is not >> called on the first iteration in the original, but it is with this patch. >> >>> if (qc_idx >= sdebug_max_queue) >>> break; >>> >>> @@ -7586,14 +7584,15 @@ static int sdebug_blk_mq_poll(struct Scsi_Host *shost, unsigned int queue_num) >>> } >>> WRITE_ONCE(sd_dp->defer_t, SDEB_DEFER_NONE); >>> spin_unlock_irqrestore(&sqp->qc_lock, iflags); >>> - locked = false; >>> scsi_done(scp); /* callback to mid level */ >>> num_entries++; >>> + spin_lock_irqsave(&sqp->qc_lock, iflags); >>> if (find_first_bit(sqp->in_use_bm, sdebug_max_queue) >= sdebug_max_queue) >>> break; /* if no more then exit without retaking spinlock */ >> >> See that comment on the line above? That is the reason for the guard variable. >> Defying that comment, the modified code does a superfluous spinlock irqsave >> and irqrestore. > > Rechecking this, there is one point that is bothering me: is it OK to have the > find_first_bit() outside of the sqp lock ? If not, then this is a bug and the > extra lock/unlock that my patch add is a fix... I think you are correct, please fix it. You will notice that when the spinlock_irq is dropped to call scsi_done(), that the iteration is restarted. >> >> Sparse could be taken as a comment on the amount of grey matter that tool has. >> >> >>> } >>> - if (locked) >>> - spin_unlock_irqrestore(&sqp->qc_lock, iflags); >>> + >>> + spin_unlock_irqrestore(&sqp->qc_lock, iflags); >>> + >>> if (num_entries > 0) >>> atomic_add(num_entries, &sdeb_mq_poll_count); >>> return num_entries; >> >> Locking issues are extremely difficult to analyze via a unified diff of >> the function. A copy of the original function is required to make any >> sense of it. I was trying to say: it is difficult to understand what diff style output of a change as shown in a [PATCH] post like this, especially to a function's locking, will do, without see the __whole__ function. It is not a criticism of this patchset, but the process in general which loses important context of the function being patched. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-03-01 1:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-25 8:45 [PATCH 0/2] Fix sparse warnings in scsi_debug Damien Le Moal 2022-02-25 8:45 ` [PATCH 1/2] scsi: scsi_debug: silence sparse unexpected unlock warnings Damien Le Moal 2022-02-28 1:39 ` Douglas Gilbert 2022-02-28 6:58 ` Damien Le Moal 2022-02-28 22:45 ` Douglas Gilbert 2022-02-25 8:45 ` [PATCH 2/2] scsi: scsi_debug: fix sparse lock warnings in sdebug_blk_mq_poll() Damien Le Moal 2022-02-28 2:05 ` Douglas Gilbert 2022-02-28 7:07 ` Damien Le Moal 2022-02-28 13:46 ` Damien Le Moal 2022-03-01 1:48 ` Douglas Gilbert
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.