target: Use WARNON_NON_RT(!irqs_disabled())

Hi,

We got a report where this WARN_ON got triggered:

[ 7552.799997] ------------[ cut here ]------------
[ 7552.800016] WARNING: CPU: 7 PID: 1090 at drivers/target/target_core_transport.c:3009 __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800037] Modules linked in: target_core_user uio target_core_pscsi target_core_file target_core_iblock ib_srpt ib_srp scsi_transport_srp scsi_tgt xt_CHECKSUM iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 tun ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bridge stp llc ib_isert iscsi_target_mod target_core_mod ib_ucm rpcrdma mlx5_ib sunrpc rdma_ucm ib_uverbs ib_iser rdma_cm iw_cm libiscsi ib_umad ib_ipoib scsi_transport_iscsi ib_cm sb_edac intel_powerclamp coretemp intel_rapl iosf_mbi crc32_pclmul ghash_clmulni_intel aesni_intel lrw gf128mul glue_helper ablk_helper cryptd iTCO_wdt iTCO_vendor_support hfi1 ipmi_ssif sg rdmavt ib_core hpilo hpwdt
  pcspkr ipmi_si
[ 7552.800055]  ipmi_devintf ipmi_msghandler wmi acpi_power_meter ioatdma dca shpchp pcc_cpufreq lpc_ich ip_tables xfs libcrc32c mgag200 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm sd_mod crc_t10dif crct10dif_generic crct10dif_pclmul crct10dif_common crc32c_intel serio_raw ata_generic pata_acpi mlx5_core ata_piix tg3 drm devlink libata i2c_core ptp hpsa scsi_transport_sas pps_core dm_mirror dm_region_hash dm_log dm_mod [last unloaded: ib_srpt]
[ 7552.800058] CPU: 7 PID: 1090 Comm: kworker/7:1H Not tainted 3.10.0-768.rt56.699.el7.x86_64 #1
[ 7552.800058] Hardware name: HP ProLiant DL380p Gen8, BIOS P70 11/14/2013
[ 7552.800066] Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
[ 7552.800067] Call Trace:
[ 7552.800075]  [<ffffffffb76cd055>] dump_stack+0x19/0x1b
[ 7552.800078]  [<ffffffffb70807bb>] __warn+0xfb/0x120
[ 7552.800080]  [<ffffffffb70808fd>] warn_slowpath_null+0x1d/0x20
[ 7552.800085]  [<ffffffffc0ab3983>] __transport_check_aborted_status+0x153/0x190 [target_core_mod]
[ 7552.800091]  [<ffffffffc0ab5c04>] target_execute_cmd+0x34/0x2e0 [target_core_mod]
[ 7552.800096]  [<ffffffffc0ab5fc2>] transport_generic_new_cmd+0x112/0x240 [target_core_mod]
[ 7552.800100]  [<ffffffffc0ab6132>] transport_handle_cdb_direct+0x42/0x90 [target_core_mod]
[ 7552.800105]  [<ffffffffc0ab62cd>] target_submit_cmd_map_sgls+0x14d/0x210 [target_core_mod]
[ 7552.800107]  [<ffffffffc09c15b4>] srpt_handle_new_iu+0x254/0x660 [ib_srpt]
[ 7552.800109]  [<ffffffffc09c1bc8>] srpt_recv_done+0x38/0x60 [ib_srpt]
[ 7552.800113]  [<ffffffffc07a5fb5>] __ib_process_cq+0x65/0xe0 [ib_core]
[ 7552.800118]  [<ffffffffc07a60a0>] ib_cq_poll_work+0x20/0x60 [ib_core]
[ 7552.800120]  [<ffffffffb70a4336>] process_one_work+0x176/0x4a0
[ 7552.800121]  [<ffffffffb70a50ec>] worker_thread+0x16c/0x3f0
[ 7552.800123]  [<ffffffffb70a4f80>] ? manage_workers.isra.36+0x2b0/0x2b0
[ 7552.800125]  [<ffffffffb70ac62f>] kthread+0xcf/0xe0
[ 7552.800139]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800151]  [<ffffffffb76dd1d8>] ret_from_fork+0x58/0x90
[ 7552.800153]  [<ffffffffb70ac560>] ? kthread_worker_fn+0x170/0x170
[ 7552.800154] ---[ end trace 0000000000000002 ]---
[ 7554.164964] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.231254] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.294860] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.360810] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.421867] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.485931] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.546909] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.607820] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.671883] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.
[ 7554.730826] srpt/0xf4521403000e4aa0f4521403000e4ad0: Unsupported SCSI Opcode 0xa3, sending CHECK_CONDITION.

static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)

        __releases(&cmd->t_state_lock)
        __acquires(&cmd->t_state_lock)
{
        int ret;

        assert_spin_locked(&cmd->t_state_lock);
        WARN_ON_ONCE_NONRT(!irqs_disabled());

<SNIP>

And it is called just from these two places:

int transport_check_aborted_status(struct se_cmd *cmd, int send_status)

{
        int ret;

        spin_lock_irq(&cmd->t_state_lock);
        ret = __transport_check_aborted_status(cmd, send_status);
        spin_unlock_irq(&cmd->t_state_lock);

        return ret;
}

EXPORT_SYMBOL(transport_check_aborted_status);

And:

void target_execute_cmd(struct se_cmd *cmd)

{
        /*
         * Determine if frontend context caller is requesting the stopping of
         * this command for frontend exceptions.
         *
         * If the received CDB has aleady been aborted stop processing it here.
         */
        spin_lock_irq(&cmd->t_state_lock);
        if (__transport_check_aborted_status(cmd, 1)) {
                spin_unlock_irq(&cmd->t_state_lock);
                return;
        }

<SNIP>

Since cmd->t_state_lock becomes a sleeping spin lock on RT, that thing triggers, turn it into a NONRT WARN_ON, a kernel built with this patch passes the test case that lead to a BZ being filled for the kernel-rt package:

https://bugzilla.redhat.com/show_bug.cgi?id=1512875

---

index 4558f2e1fe1b..318453e7adfd 100644
@@ -3204,7 +3204,7 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 	int ret;
 
 	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
+	WARN_ON_ONCE_NONRT(!irqs_disabled());
 
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;

[ Adding PeterZ to participants due to query about lockdep_assert() ]

We are working on replacing the _NONRT _RT variants with proper lockdep mechnisms which are aware of the RT vs. non-RT magic under the hood. Just not there yet.

Thanks,

tglx

Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:

Huh, even better, when that feature gets finished (tglx said it was being developed, not there yet tho) it'll allow further reduction of the PREEMPT_RT_FULL patchkit.

- Arnaldo

Em Thu, Mar 22, 2018 at 06:37:45AM -0300, Arnaldo Carvalho de Melo escreveu:

We'd get rid of these:

[acme@jouet patches-4.11.12-rt15]$ grep "^+[[:space:]]\+.*NONRT" *.patch
dm-make-rt-aware.patch:+ BUG_ON_NONRT(!irqs_disabled());
fs-block-rt-support.patch:+ WARN_ON_NONRT(!irqs_disabled());
i915-bogus-warning-from-i915-when-running-on-PREEMPT.patch:+ WARN_ON_NONRT(!in_interrupt());
iommu-amd--Use-WARN_ON_NORT.patch:+ WARN_ON_NONRT(!irqs_disabled());
iommu-amd--Use-WARN_ON_NORT.patch:+ WARN_ON_NONRT(!irqs_disabled());
irqwork-push_most_work_into_softirq_context.patch:+ BUG_ON_NONRT(!irqs_disabled());
net-wireless-warn-nort.patch:+ WARN_ON_ONCE_NONRT(softirq_count() == 0);
posix-timers-thread-posix-cpu-timers-on-rt.patch:+ WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+ WARN_ON_ONCE_NONRT(!irqs_disabled());
posix-timers-thread-posix-cpu-timers-on-rt.patch:+ WARN_ON_ONCE_NONRT(!irqs_disabled());
workqueue-use-locallock.patch:+ WARN_ON_ONCE_NONRT(!irqs_disabled());
[acme@jouet patches-4.11.12-rt15]$

- Arnaldo

That's the evil plan :)

I am going take this into -RT tree for now until we have different solution. I will try to be kind and do the same change in __transport_wait_for_tasks().
Arnaldo, please do "[PATCH RT]" while sending patches. Then the bots don't complain if it applies but does not compile on !RT kernel (or so I've been told).

Technically speaking the code wants to ensure that the lock is held and the interrupts are disabled because the lock is always taken with disabled interrupts. This kind of check could be done with

	lockdep_assert_held(&cmd->t_state_lock);

but would require lockdep to be switched on. Nicholas, would you mind such a change?

Sebastian

Have you considered to delete the WARN_ON_ONCE(!irqs_disabled()) statement? I think that check duplicates functionality that already exists in lockdep since lockdep is already able to detect spinlock use inconsistencies.

Bart.

correct. That is why I suggested to use lockdep_assert_held() instead of this IRQ-check + the spin_lock_assert().
The only downside is that this code (as of now) works with lockdep disabled. On the other hand, lockdep_assert_held() gives you a splat instead of a BUG() statement like spin_lock_assert() does so I clearly promote lockdep here :)

Sebastian

Please just delete both lines.

There is exactly two callers of that static function, and both of them do

        spin_lock_irq(&cmd->t_state_lock);

right above the call.

It's not like this is some function that is exported to random users, and we should check that the calling convention is right.

So honestly, even lockdep annotations look like you don't need them.

This looks like "it may have been useful during coding to document things, but it's not useful long-term".

Sure, the annotation is not wrong, but even if you go "verification is good", you should ask yourself whether there are maybe better places that would catch more relevant problems, than putting verification into some static function with two trivially correct callers wrt this verification?

Linus

There are a few functions which check for if the lock is held (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).

and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

---
 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

index 9c7bc1ca341a..3d35dad1de2c 100644
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,

index 74b646f165d4..2c3ddb3a4124 100644
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 

@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*

2.16.2

__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.

---
 drivers/target/target_core_alua.c | 4 ----
 1 file changed, 4 deletions(-)

index e46ca968009c..e5bda674bdbd 100644
@@ -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--;

2.16.2

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.

Those two do not.

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?

Sebastian

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.

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

There are a few functions which check for if the lock is held (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).

and does spin_lock_irq|safe(). As Linus puts it:

|It's not like this is some function that is exported to random users,
|and we should check that the calling convention is right.
|
|This looks like "it may have been useful during coding to document
|things, but it's not useful long-term".

Remove those checks.

---
v1…2: Add credits.

 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

index 9c7bc1ca341a..3d35dad1de2c 100644
@@ -114,8 +114,6 @@ static bool __target_check_io_state(struct se_cmd *se_cmd,
 {
 	struct se_session *sess = se_cmd->se_sess;
 
-	assert_spin_locked(&sess->sess_cmd_lock);
-	WARN_ON_ONCE(!irqs_disabled());
 	/*
 	 * If command already reached CMD_T_COMPLETE state within
 	 * target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,

index 74b646f165d4..2c3ddb3a4124 100644
@@ -2954,9 +2954,6 @@ __transport_wait_for_tasks(struct se_cmd *cmd, bool fabric_stop,
 	__acquires(&cmd->t_state_lock)
 {
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (fabric_stop)
 		cmd->transport_state |= CMD_T_FABRIC_STOP;
 

@@ -3241,9 +3238,6 @@ static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
 {
 	int ret;
 
-	assert_spin_locked(&cmd->t_state_lock);
-	WARN_ON_ONCE(!irqs_disabled());
-
 	if (!(cmd->transport_state & CMD_T_ABORTED))
 		return 0;
 	/*

2.16.2

I could. I haven't heard from Bart / Nicholas about their opinion. I know, we do this other places but Bart was against this kind of annotation in 2/2 of this thread.
So I am waiting to hear from them :)

Sebastian

The names of the two functions touched by patch 1/2 start with a double underscore. That by itself is already a hint that these should be called with a lock held (I know that this is not a universal convention in the Linux kernel). I'm fine either way - either with patch 1/2 as posted or patch 1/2 with the above comment added.

Bart.

Okay. In that case let me update 1/2.
But 2/2 with the comment as Steven suggested is still a no no for you?

Sebastian

Although I'm not enthusiast about patch 2/2, if others agree with that patch I'm fine with that patch being sent upstream.

Thanks,

Bart.

I've been waiting for something to happen but nobody replied. Bart, you were fine with 1/2 as posted but not too happy about 2/2. Assuming we keep 1/2 as is and I remove just the "WARN_ON_ONCE(!irqs_disabled());" from 2/2 (keeping the assert_spin_locked()), would that improve your mood? Lockdep would still perform full validation, yell if __transport_check_aborted_status() was invoked without locking and also yell abut missing IRQ-save at locking time of ->t_state_lock).

Sebastian

Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel?

Thanks,

Bart.

no, because the interrupts are not disabled at this point on RT. Lockdep works fine on RT.

Sebastian

Em Fri, Mar 23, 2018 at 04:55:13PM +0100, Sebastian Andrzej Siewior escreveu:

Ok, I'll try to remember that for future patches.

- Arnaldo

It's only in the RT patch set. But that may be changing soon.

I agree with Linus. This should just be some kind of lockdep_assert_held_irqs_disabeld() or something like that.

-- Steve