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@xxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>
---
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)
