[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

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

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

---

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4558f2e1fe1b..318453e7adfd 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -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;

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

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

On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
>         assert_spin_locked(&cmd->t_state_lock);
> -       WARN_ON_ONCE(!irqs_disabled());
> +       WARN_ON_ONCE_NONRT(!irqs_disabled());

Ugh.

Can't we just replace both of those with a lockdep annotation?

Does "lockdep_assert_held()" already verify the irq contextr, or do we
need lockdep_assert_irqs_disabled() too?

Honestly, the old-fashioned way of doing verification of state by hand
is understandable, but it's legacy and kind of pointless when we have
much better tools these days.

I'm perfectly willing to leave old assertions in place, but if they
need fixing anyway, I'd damn well want to fix them *right* instead of
starting to just add more piles of hacks on top of the old model.

Because when the details of the locking rules depend on RT vs non-RT,
I want the checks to make sense.  And presumably lockdep is the thing
that really knows what the status of a lock is, no?

                  Linus

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Christoph Hellwig]

On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
>  	assert_spin_locked(&cmd->t_state_lock);
> -	WARN_ON_ONCE(!irqs_disabled());
> +	WARN_ON_ONCE_NONRT(!irqs_disabled());

I can't find where WARN_ON_ONCE_NONRT is defined.

That being said I think we can just kill these asserts.  If we have irqs
disabled spin_unlock_irq a few lines below should already warn.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Steven Rostedt]

On Wed, 21 Mar 2018 11:50:01 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, Mar 21, 2018 at 12:38:54PM -0300, Arnaldo Carvalho de Melo wrote:
> >  	assert_spin_locked(&cmd->t_state_lock);
> > -	WARN_ON_ONCE(!irqs_disabled());
> > +	WARN_ON_ONCE_NONRT(!irqs_disabled());  
> 
> I can't find where WARN_ON_ONCE_NONRT is defined.

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

> 
> That being said I think we can just kill these asserts.  If we have irqs
> disabled spin_unlock_irq a few lines below should already warn.

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

-- Steve

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Wed, 21 Mar 2018, Linus Torvalds wrote:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?
> 
> Does "lockdep_assert_held()" already verify the irq contextr, or do we
> need lockdep_assert_irqs_disabled() too?
> 
> Honestly, the old-fashioned way of doing verification of state by hand
> is understandable, but it's legacy and kind of pointless when we have
> much better tools these days.
> 
> I'm perfectly willing to leave old assertions in place, but if they
> need fixing anyway, I'd damn well want to fix them *right* instead of
> starting to just add more piles of hacks on top of the old model.
> 
> Because when the details of the locking rules depend on RT vs non-RT,
> I want the checks to make sense.  And presumably lockdep is the thing
> that really knows what the status of a lock is, no?

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

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> [ Adding PeterZ to participants due to query about lockdep_assert() ]
> 
> On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> >         assert_spin_locked(&cmd->t_state_lock);
> > -       WARN_ON_ONCE(!irqs_disabled());
> > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> 
> Ugh.
> 
> Can't we just replace both of those with a lockdep annotation?

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

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Thu, Mar 22, 2018 at 06:37:45AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> 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.

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

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Thomas Gleixner]

On Thu, 22 Mar 2018, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> 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.

That's the evil plan :)

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-x015-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers//target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers//target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT'; did you mean 'WARN_ON_ONCE'? [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
     WARN_ON_ONCE
   cc1: some warnings being treated as errors

vim +3207 drivers//target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24987 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

Hi Arnaldo,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Arnaldo-Carvalho-de-Melo/target-Use-WARNON_NON_RT-irqs_disabled/20180322-174549
config: i386-randconfig-s1-03221113 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/target/target_core_transport.c: In function '__transport_check_aborted_status':
>> drivers/target/target_core_transport.c:3207:2: error: implicit declaration of function 'WARN_ON_ONCE_NONRT' [-Werror=implicit-function-declaration]
     WARN_ON_ONCE_NONRT(!irqs_disabled());
     ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/WARN_ON_ONCE_NONRT +3207 drivers/target/target_core_transport.c

  3199	
  3200	static int __transport_check_aborted_status(struct se_cmd *cmd, int send_status)
  3201		__releases(&cmd->t_state_lock)
  3202		__acquires(&cmd->t_state_lock)
  3203	{
  3204		int ret;
  3205	
  3206		assert_spin_locked(&cmd->t_state_lock);
> 3207		WARN_ON_ONCE_NONRT(!irqs_disabled());
  3208	
  3209		if (!(cmd->transport_state & CMD_T_ABORTED))
  3210			return 0;
  3211		/*
  3212		 * If cmd has been aborted but either no status is to be sent or it has
  3213		 * already been sent, just return
  3214		 */
  3215		if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
  3216			if (send_status)
  3217				cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
  3218			return 1;
  3219		}
  3220	
  3221		pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
  3222			" 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
  3223	
  3224		cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
  3225		cmd->scsi_status = SAM_STAT_TASK_ABORTED;
  3226		trace_target_cmd_complete(cmd);
  3227	
  3228		spin_unlock_irq(&cmd->t_state_lock);
  3229		ret = cmd->se_tfo->queue_status(cmd);
  3230		if (ret)
  3231			transport_handle_queue_full(cmd, cmd->se_dev, ret, false);
  3232		spin_lock_irq(&cmd->t_state_lock);
  3233	
  3234		return 1;
  3235	}
  3236	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28546 bytes --]

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Sebastian Andrzej Siewior]

On 2018-03-22 06:37:45 [-0300], Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 21, 2018 at 11:43:58AM -0700, Linus Torvalds escreveu:
> > [ Adding PeterZ to participants due to query about lockdep_assert() ]
> > 
> > On Wed, Mar 21, 2018 at 8:38 AM, Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > >         assert_spin_locked(&cmd->t_state_lock);
> > > -       WARN_ON_ONCE(!irqs_disabled());
> > > +       WARN_ON_ONCE_NONRT(!irqs_disabled());
> > 
> > Ugh.
> > 
> > Can't we just replace both of those with a lockdep annotation?
> 
> 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.

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?

> - Arnaldo

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Bart Van Assche]

On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote:
> I am going take this into -RT tree for now until we have different
> solution.

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.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[bigeasy]

On 2018-03-23 16:25:25 [+0000], Bart Van Assche wrote:
> On Fri, 2018-03-23 at 16:55 +0100, Sebastian Andrzej Siewior wrote:
> > I am going take this into -RT tree for now until we have different
> > solution.
> 
> 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.

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 :)

> Bart.

Sebastian

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Linus Torvalds]

On Fri, Mar 23, 2018 at 9:25 AM, Bart Van Assche <Bart.VanAssche@wdc.com> wrote:
>
> 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.

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

[PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
>From looking at the code, each function is static, the caller is near by
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.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/target/target_core_tmr.c       | 2 --
 drivers/target/target_core_transport.c | 6 ------
 2 files changed, 8 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -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,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -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

[PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

__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(-)

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--;
-- 
2.16.2

[PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Sebastian Andrzej Siewior]

There are a few functions which check for if the lock is held
(spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
>From looking at the code, each function is static, the caller is near by
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.

Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…2: Add credits.

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

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 9c7bc1ca341a..3d35dad1de2c 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -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,
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 74b646f165d4..2c3ddb3a4124 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -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

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

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.

Re: [PATCH 1/2 v2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Fri, 2018-03-23 at 18:36 +0100, Sebastian Andrzej Siewior wrote:
> There are a few functions which check for if the lock is held
> (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
> > From looking at the code, each function is static, the caller is near by
> 
> 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.

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[bigeasy]

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

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Bart Van Assche]

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.

Re: [PATCH] target: Use WARNON_NON_RT(!irqs_disabled())

[Arnaldo Carvalho de Melo]

Em Fri, Mar 23, 2018 at 04:55:13PM +0100, Sebastian Andrzej Siewior escreveu:
> 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).

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

- Arnaldo

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Steven Rostedt]

On Fri, 23 Mar 2018 18:17:36 +0100
"bigeasy@linutronix.de" <bigeasy@linutronix.de> wrote:

> There are a few functions which check for if the lock is held
> (spin_lock_assert()) and the interrupts are disabled (irqs_disabled()).
> >From looking at the code, each function is static, the caller is near by  
> 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.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/target/target_core_tmr.c       | 2 --
>  drivers/target/target_core_transport.c | 6 ------
>  2 files changed, 8 deletions(-)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 9c7bc1ca341a..3d35dad1de2c 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c

Can you add a comment above the functions though?

/* Expects to have se_cmd->se_sess->sess_cmd_lock held */

> @@ -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,
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 74b646f165d4..2c3ddb3a4124 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c

/* Expects to have cmd->t_state_lock held */

> @@ -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;
>  

/* Expects to have cmd->t_state_lock held */

> @@ -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;
>  	/*

-- Steve

Re: [PATCH 2/2] target: remove spin_lock_assert() in __target_(attach|detach)_tg_pt_gp()

[Steven Rostedt]

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

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote:
> > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > index 9c7bc1ca341a..3d35dad1de2c 100644
> > --- a/drivers/target/target_core_tmr.c
> > +++ b/drivers/target/target_core_tmr.c
> 
> Can you add a comment above the functions though?
> 
> /* Expects to have se_cmd->se_sess->sess_cmd_lock held */

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 :)

> 
> -- Steve

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Wed, 2018-03-28 at 12:15 +0200, bigeasy@linutronix.de wrote:
> On 2018-03-26 11:13:59 [-0400], Steven Rostedt wrote:
> > > diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> > > index 9c7bc1ca341a..3d35dad1de2c 100644
> > > --- a/drivers/target/target_core_tmr.c
> > > +++ b/drivers/target/target_core_tmr.c
> > 
> > Can you add a comment above the functions though?
> > 
> > /* Expects to have se_cmd->se_sess->sess_cmd_lock held */
> 
> 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 :)

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.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> 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.

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?

> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
>> 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.
> 
> 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?

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.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote:
> On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> > > 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.
> > 
> > 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?
> 
> Although I'm not enthusiast about patch 2/2, if others agree with that patch
> I'm fine with that patch being sent upstream.

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

> Thanks,
> 
> Bart.

Sebastian

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[Bart Van Assche]

On Mon, 2018-05-28 at 16:35 +0200, bigeasy@linutronix.de wrote:
> On 2018-03-28 08:31:38 [-0700], Bart Van Assche wrote:
> > On 03/28/18 08:14, bigeasy@linutronix.de wrote:
> > > On 2018-03-28 15:05:41 [+0000], Bart Van Assche wrote:
> > > > 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.
> > > 
> > > 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?
> > 
> > Although I'm not enthusiast about patch 2/2, if others agree with that patch
> > I'm fine with that patch being sent upstream.
> 
> 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).

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

Thanks,

Bart.

Re: [PATCH 1/2] target: drop spin_lock_assert() + irqs_disabled() combo checks

[bigeasy]

On 2018-06-04 07:02:15 [+0000], Bart Van Assche wrote:
> 
> Would adding WARN_ON_ONCE(!irqs_disabled()) work fine with an RT kernel?

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

> Thanks,
> 
> Bart.

Sebastian