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@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)
>
--
Damien Le Moal
Western Digital Research
