| Message ID | 20180323171959.7ac6klrqg4fdk3ey@linutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, 2018-03-23 at 18:19 +0100, bigeasy@linutronix.de wrote: > __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller > holds lun_tg_pt_gp_lock(). Both functions are static, the callers are > acquiring the lock before the invocation of the function so the check > looks superfluous. > Remove it. Does this check cause trouble to anyone or to a specific kernel configuration? In other words, do we really need to remove these checks? I think that these checks are useful as documentation to people who read the SCSI target code. The target code is already hard to follow so I think any documentation, especially documentation in the form of code that is checked at runtime, is welcome. Thanks, Bart.
On 2018-03-23 17:44:46 [+0000], Bart Van Assche wrote: > On Fri, 2018-03-23 at 18:19 +0100, bigeasy@linutronix.de wrote: > > __target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller > > holds lun_tg_pt_gp_lock(). Both functions are static, the callers are > > acquiring the lock before the invocation of the function so the check > > looks superfluous. > > Remove it. > > Does this check cause trouble to anyone or to a specific kernel configuration? Those two do not. > In other words, do we really need to remove these checks? I think that these > checks are useful as documentation to people who read the SCSI target code. > The target code is already hard to follow so I think any documentation, > especially documentation in the form of code that is checked at runtime, is > welcome. so if I remove those two and add a kernel doc comment instead, saying that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held then we would remove the obvious runtime check and add a piece of documentation. Would that work? > Thanks, > > Bart. Sebastian
On Fri, 2018-03-23 at 18:50 +0100, bigeasy@linutronix.de wrote: > On 2018-03-23 17:44:46 [+0000], Bart Van Assche wrote: > > In other words, do we really need to remove these checks? I think that these > > checks are useful as documentation to people who read the SCSI target code. > > The target code is already hard to follow so I think any documentation, > > especially documentation in the form of code that is checked at runtime, is > > welcome. > > so if I remove those two and add a kernel doc comment instead, saying > that the caller needs to ensure that "lun->lun_tg_pt_gp_lock" is held > then we would remove the obvious runtime check and add a piece of > documentation. Would that work? Comments are not verified at runtime and hence can become outdated if the code is modified. assert_spin_locked() and lockdep_assert_held() assertions however are verified at runtime with the proper kernel configuration options enabled. Hence my preference for assert_spin_locked()/lockdep_assert_held() over source code comments. Thanks, Bart.
On Fri, 23 Mar 2018 17:55:54 +0000 Bart Van Assche <Bart.VanAssche@wdc.com> wrote: > Comments are not verified at runtime and hence can become outdated if the code > is modified. assert_spin_locked() and lockdep_assert_held() assertions however > are verified at runtime with the proper kernel configuration options enabled. > Hence my preference for assert_spin_locked()/lockdep_assert_held() over source > code comments. Asserts are fine, but when the code is static and used close to the caller, asserts are overkill. A comment at the beginning of a function should suffice. We do that all over the kernel for functions like that. The asserts are there when the code can be called from other files, or in multiple places. -- Steve
diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c index e46ca968009c..e5bda674bdbd 100644 --- a/drivers/target/target_core_alua.c +++ b/drivers/target/target_core_alua.c @@ -1843,8 +1843,6 @@ static void __target_attach_tg_pt_gp(struct se_lun *lun, { struct se_dev_entry *se_deve; - assert_spin_locked(&lun->lun_tg_pt_gp_lock); - spin_lock(&tg_pt_gp->tg_pt_gp_lock); lun->lun_tg_pt_gp = tg_pt_gp; list_add_tail(&lun->lun_tg_pt_gp_link, &tg_pt_gp->tg_pt_gp_lun_list); @@ -1868,8 +1866,6 @@ void target_attach_tg_pt_gp(struct se_lun *lun, static void __target_detach_tg_pt_gp(struct se_lun *lun, struct t10_alua_tg_pt_gp *tg_pt_gp) { - assert_spin_locked(&lun->lun_tg_pt_gp_lock); - spin_lock(&tg_pt_gp->tg_pt_gp_lock); list_del_init(&lun->lun_tg_pt_gp_link); tg_pt_gp->tg_pt_gp_members--;
__target_attach_tg_pt_gp() and __target_detach_tg_pt_gp() check if the caller holds lun_tg_pt_gp_lock(). Both functions are static, the callers are acquiring the lock before the invocation of the function so the check looks superfluous. Remove it. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/target/target_core_alua.c | 4 ---- 1 file changed, 4 deletions(-)