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