target: Use WARNON_NON_RT(!irqs_disabled())
diff mbox

Message ID 20180321153854.GB24312@kernel.org
State New, archived
Headers show

Commit Message

Arnaldo Carvalho de Melo March 21, 2018, 3:38 p.m. UTC
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>

---

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Torvalds March 21, 2018, 6:43 p.m. UTC | #1
[ 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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig March 21, 2018, 6:50 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Steven Rostedt March 21, 2018, 7:01 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner March 22, 2018, 9:13 a.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnaldo Carvalho de Melo March 22, 2018, 9:37 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnaldo Carvalho de Melo March 22, 2018, 9:40 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner March 22, 2018, 9:47 a.m. UTC | #7
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 :)
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kbuild test robot March 22, 2018, 10:25 a.m. UTC | #8
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
kbuild test robot March 22, 2018, 10:45 a.m. UTC | #9
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
Sebastian Andrzej Siewior March 23, 2018, 3:55 p.m. UTC | #10
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche March 23, 2018, 4:25 p.m. UTC | #11
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.
Sebastian Andrzej Siewior March 23, 2018, 4:33 p.m. UTC | #12
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds March 23, 2018, 4:37 p.m. UTC | #13
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnaldo Carvalho de Melo March 26, 2018, 2:21 p.m. UTC | #14
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
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;