| Message ID | 20220225084527.523038-2-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix sparse warnings in scsi_debug | expand |
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)
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) >
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) >> > >
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)
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(-)