[GIT PULL 0/2] infiniband hfi1 PREEMPT_RT_FULL changes

[GIT PULL 0/2] infiniband hfi1 PREEMPT_RT_FULL changes

[Arnaldo Carvalho de Melo]

Hi,

	Please consider pulling into the linux-4.11.y-rt branch,

	It was tested with a simple netperf script found below (that I
intercepted from PeterZ :-)), systems with default kernel tunables, with
the following results:

# Both machines:
[root@opa0-qa-06 ~]# getconf _NPROCESSORS_ONLN
40
[root@opa0-qa-06 ~]# ./netpeterz 
                  3.10.0-716         3.10.0-716.rt56.644.test   
TCP_SENDFILE-1 :  Avg: 18014.7       Avg: 16443.6
TCP_SENDFILE-10 : Avg:  1691.76      Avg:  2070.77
TCP_SENDFILE-20 : Avg:   833.356     Avg:  1090.62
TCP_SENDFILE-40 : Avg:   409.206     Avg:   560.65
TCP_SENDFILE-80 : Avg:   197.961     Avg:   216.654
TCP_STREAM-1 :    Avg: 21648.3       Avg: 18678.7  
TCP_STREAM-10 :   Avg:  1804.94      Avg:  1683.76
TCP_STREAM-20 :   Avg:   839.943     Avg:   857.156
TCP_STREAM-40 :   Avg:   399.508     Avg:   428.413
TCP_STREAM-80 :   Avg:   191.331     Avg:   203.931
TCP_MAERTS-1 :    Avg: 21363         Avg: 21082.8
TCP_MAERTS-10 :   Avg:  1626.78      Avg:  2145.71
TCP_MAERTS-20 :   Avg:   699.969     Avg:   960.48
TCP_MAERTS-40 :   Avg:   378.719     Avg:   496.143
TCP_MAERTS-80 :   Avg:   183.864     Avg:   251.155
TCP_RR-1 :        Avg: 37258.7       Avg: 27904.7
TCP_RR-10 :       Avg: 14880.6       Avg: 12176.9
TCP_RR-20 :       Avg:  7482.94      Avg:  5964.42
TCP_RR-40 :       Avg:  3762.57      Avg:  2937.66
TCP_RR-80 :       Avg:  1909.16      Avg:  1468.5
UDP_RR-1 :        Avg: 40755.8       Avg: 30205
UDP_RR-10 :       Avg: 15197.6       Avg: 13460.7
UDP_RR-20 :       Avg:  7552.46      Avg:  6553.47
UDP_RR-40 :       Avg:  3798.78      Avg:  3134.96
UDP_RR-80 :       Avg:  1922.08      Avg:  1533.69
UDP_STREAM-1 :    Avg: 18385.7       Avg: 17827.5
UDP_STREAM-10 :   Avg: 27855.9       Avg:  1641.97
UDP_STREAM-20 :   Avg: 11650.8       Avg:  1309.35
UDP_STREAM-40 :   Avg:  3824.01      Avg:   669.287
UDP_STREAM-80 :   Avg:  3465.8       Avg:   441.299
[root@opa0-qa-06 ~]#

[root@opa0-qa-06 ~]# cat netpeterz
#!/bin/bash

addrs="-L 172.31.20.6 -H 172.31.20.5"

for test in TCP_SENDFILE TCP_STREAM TCP_MAERTS ; do
	for i in 1 10 20 40 80 ; do
		echo -n $test-$i ": "
		(
		  for ((j=0; j<i; j++)) ; do
			netperf $addrs -t $test -4 -c -C -l 60 -P0 | head -1 &
		  done
		  wait
		) | awk '{ n++; v+=$5; } END { print "Avg: " v/n }'
	done
done

for test in TCP_RR UDP_RR UDP_STREAM ; do
	for i in 1 10 20 40 80 ; do
		echo -n $test-$i ": "
		(
		  for ((j=0; j<i; j++)) ; do
			netperf $addrs -t $test -4 -c -C -l 60 -P0 | head -1 &
		  done
		  wait
		) | awk '{ n++; v+=$6; } END { print "Avg: " v/n }'
	done
done
[root@opa0-qa-06 ~]#

The following changes since commit 327e86929bbc08ddb4b9bd6d5d222ef052eaa4a8:

  v4.11.12-rt14 (2017-09-22 11:12:56 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git infiniband-4.11.y-rt

for you to fetch changes up to 3f59a2ba5bdbf28f4dfe43fe0b713c7574d2baaa:

  IB/hfi1: Handle packets in the theaded handler only (2017-10-03 12:34:39 -0300)

----------------------------------------------------------------
Arnaldo Carvalho de Melo (2):
      IB/hfi1: Use preempt_{dis,en}able_nort()
      IB/hfi1: Handle packets in the theaded handler only

 drivers/infiniband/hw/hfi1/chip.c     | 10 +++++++---
 drivers/infiniband/hw/hfi1/pio.c      |  2 +-
 drivers/infiniband/hw/hfi1/pio_copy.c |  4 ++--
 3 files changed, 10 insertions(+), 6 deletions(-)

[PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

sc_buffer_alloc() disables preemption that will be reenabled by either
pio_copy() or seg_pio_copy_end(). But before disabling preemption it
grabs a spin lock that will be dropped after it disables preemption,
which ends up triggering a warning in migrate_disable() later on.

    spin_lock_irqsave(&sc->alloc_lock)
      migrate_disable() ++p->migrate_disable -> 2
    preempt_disable()
    spin_unlock_irqrestore(&sc->alloc_lock)
      migrate_enable() in_atomic(), so just returns, migrate_disable stays at 2
    spin_lock_irqsave(some other lock) -> b00m

And the WARN_ON code ends up tripping over this over and over in
log_store().

Sequence captured via ftrace_dump_on_oops + crash utility 'dmesg'
command.

[512258.613862] sm-3297 16 .....11 359465349134644: sc_buffer_alloc <-hfi1_verbs_send_pio
[512258.613876] sm-3297 16 .....11 359465349134719: migrate_disable <-sc_buffer_alloc
[512258.613890] sm-3297 16 .....12 359465349134798: rt_spin_lock <-sc_buffer_alloc
[512258.613903] sm-3297 16 ....112 359465349135481: rt_spin_unlock <-sc_buffer_alloc
[512258.613916] sm-3297 16 ....112 359465349135556: migrate_enable <-sc_buffer_alloc
[512258.613935] sm-3297 16 ....112 359465349135788: seg_pio_copy_start <-hfi1_verbs_send_pio
[512258.613954] sm-3297 16 ....112 359465349136273: update_sge <-hfi1_verbs_send_pio
[512258.613981] sm-3297 16 ....112 359465349136373: seg_pio_copy_mid <-hfi1_verbs_send_pio
[512258.613999] sm-3297 16 ....112 359465349136873: update_sge <-hfi1_verbs_send_pio
[512258.614017] sm-3297 16 ....112 359465349136956: seg_pio_copy_mid <-hfi1_verbs_send_pio
[512258.614035] sm-3297 16 ....112 359465349137221: seg_pio_copy_end <-hfi1_verbs_send_pio
[512258.614048] sm-3297 16 .....12 359465349137360: migrate_disable <-hfi1_verbs_send_pio
[512258.614065] sm-3297 16 .....12 359465349137476: warn_slowpath_null <-migrate_disable
[512258.614081] sm-3297 16 .....12 359465349137564: __warn <-warn_slowpath_null
[512258.614088] sm-3297 16 .....12 359465349137958: printk <-__warn
[512258.614096] sm-3297 16 .....12 359465349138055: vprintk_default <-printk
[512258.614104] sm-3297 16 .....12 359465349138144: vprintk_emit <-vprintk_default
[512258.614111] sm-3297 16 d....12 359465349138312: _raw_spin_lock <-vprintk_emit
[512258.614119] sm-3297 16 d...112 359465349138789: log_store <-vprintk_emit
[512258.614127] sm-3297 16 .....12 359465349139068: migrate_disable <-vprintk_emit

According to a discussion (see Link: below) on the linux-rt-users
mailing list, this locking is done for performance reasons, not for
correctness, so use the _nort() variants to avoid the above problem.

Suggested-by: Julia Cartwright <julia@ni.com>
Cc: Clark Williams <williams@redhat.com>
Cc: Dean Luick <dean.luick@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Kaike Wan <kaike.wan@intel.com>
Cc: Leon Romanovsky <leonro@mellanox.com>
Cc: linux-rdma@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <sebastian.siewior@linutronix.de>
Cc: Sebastian Sanchez <sebastian.sanchez@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20170926210045.GO29872@jcartwri.amer.corp.natinst.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 drivers/infiniband/hw/hfi1/pio.c      | 2 +-
 drivers/infiniband/hw/hfi1/pio_copy.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
index 615be68e40b3..3a30bde9a07b 100644
--- a/drivers/infiniband/hw/hfi1/pio.c
+++ b/drivers/infiniband/hw/hfi1/pio.c
@@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
 
 	/* there is enough room */
 
-	preempt_disable();
+	preempt_disable_nort();
 	this_cpu_inc(*sc->buffers_allocated);
 
 	/* read this once */
diff --git a/drivers/infiniband/hw/hfi1/pio_copy.c b/drivers/infiniband/hw/hfi1/pio_copy.c
index 03024cec78dd..c3f48f705c97 100644
--- a/drivers/infiniband/hw/hfi1/pio_copy.c
+++ b/drivers/infiniband/hw/hfi1/pio_copy.c
@@ -162,7 +162,7 @@ void pio_copy(struct hfi1_devdata *dd, struct pio_buf *pbuf, u64 pbc,
 
 	/* finished with this buffer */
 	this_cpu_dec(*pbuf->sc->buffers_allocated);
-	preempt_enable();
+	preempt_enable_nort();
 }
 
 /*
@@ -753,5 +753,5 @@ void seg_pio_copy_end(struct pio_buf *pbuf)
 
 	/* finished with this buffer */
 	this_cpu_dec(*pbuf->sc->buffers_allocated);
-	preempt_enable();
+	preempt_enable_nort();
 }
-- 
2.13.6

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Julia Cartwright]

On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> sc_buffer_alloc() disables preemption that will be reenabled by either
> pio_copy() or seg_pio_copy_end(). But before disabling preemption it
> grabs a spin lock that will be dropped after it disables preemption,
> which ends up triggering a warning in migrate_disable() later on.
> 
>     spin_lock_irqsave(&sc->alloc_lock)
>       migrate_disable() ++p->migrate_disable -> 2
>     preempt_disable()
>     spin_unlock_irqrestore(&sc->alloc_lock)
>       migrate_enable() in_atomic(), so just returns, migrate_disable stays at 2
>     spin_lock_irqsave(some other lock) -> b00m
> 
> And the WARN_ON code ends up tripping over this over and over in
> log_store().
> 
> Sequence captured via ftrace_dump_on_oops + crash utility 'dmesg'
> command.
> 
> [512258.613862] sm-3297 16 .....11 359465349134644: sc_buffer_alloc <-hfi1_verbs_send_pio
> [512258.613876] sm-3297 16 .....11 359465349134719: migrate_disable <-sc_buffer_alloc
> [512258.613890] sm-3297 16 .....12 359465349134798: rt_spin_lock <-sc_buffer_alloc
> [512258.613903] sm-3297 16 ....112 359465349135481: rt_spin_unlock <-sc_buffer_alloc
> [512258.613916] sm-3297 16 ....112 359465349135556: migrate_enable <-sc_buffer_alloc
> [512258.613935] sm-3297 16 ....112 359465349135788: seg_pio_copy_start <-hfi1_verbs_send_pio
> [512258.613954] sm-3297 16 ....112 359465349136273: update_sge <-hfi1_verbs_send_pio
> [512258.613981] sm-3297 16 ....112 359465349136373: seg_pio_copy_mid <-hfi1_verbs_send_pio
> [512258.613999] sm-3297 16 ....112 359465349136873: update_sge <-hfi1_verbs_send_pio
> [512258.614017] sm-3297 16 ....112 359465349136956: seg_pio_copy_mid <-hfi1_verbs_send_pio
> [512258.614035] sm-3297 16 ....112 359465349137221: seg_pio_copy_end <-hfi1_verbs_send_pio
> [512258.614048] sm-3297 16 .....12 359465349137360: migrate_disable <-hfi1_verbs_send_pio
> [512258.614065] sm-3297 16 .....12 359465349137476: warn_slowpath_null <-migrate_disable
> [512258.614081] sm-3297 16 .....12 359465349137564: __warn <-warn_slowpath_null
> [512258.614088] sm-3297 16 .....12 359465349137958: printk <-__warn
> [512258.614096] sm-3297 16 .....12 359465349138055: vprintk_default <-printk
> [512258.614104] sm-3297 16 .....12 359465349138144: vprintk_emit <-vprintk_default
> [512258.614111] sm-3297 16 d....12 359465349138312: _raw_spin_lock <-vprintk_emit
> [512258.614119] sm-3297 16 d...112 359465349138789: log_store <-vprintk_emit
> [512258.614127] sm-3297 16 .....12 359465349139068: migrate_disable <-vprintk_emit
> 
> According to a discussion (see Link: below) on the linux-rt-users
> mailing list, this locking is done for performance reasons, not for
> correctness, so use the _nort() variants to avoid the above problem.
> 
> Suggested-by: Julia Cartwright <julia@ni.com>
> Cc: Clark Williams <williams@redhat.com>
> Cc: Dean Luick <dean.luick@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Kaike Wan <kaike.wan@intel.com>
> Cc: Leon Romanovsky <leonro@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sebastian Andrzej Siewior <sebastian.siewior@linutronix.de>
> Cc: Sebastian Sanchez <sebastian.sanchez@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: https://urldefense.proofpoint.com/v2/url?u=http-3A__lkml.kernel.org_r_20170926210045.GO29872-40jcartwri.amer.corp.natinst.com&d=DwIBaQ&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=cAXq_8W9Othb2h8ZcWv8Vw&m=zzKtWJ595HB0jyuiFic0ZEkpmmjvGRXJHkGF27oyvCI&s=J4_Al0cbvQ9PCM3VbqzJ6apmpSZI9Xx7eq6Gcfucp24&e= 
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  drivers/infiniband/hw/hfi1/pio.c      | 2 +-
>  drivers/infiniband/hw/hfi1/pio_copy.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
> index 615be68e40b3..3a30bde9a07b 100644
> --- a/drivers/infiniband/hw/hfi1/pio.c
> +++ b/drivers/infiniband/hw/hfi1/pio.c
> @@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
>  
>  	/* there is enough room */
>  
> -	preempt_disable();
> +	preempt_disable_nort();
>  	this_cpu_inc(*sc->buffers_allocated);

Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
this_cpu_* operations perform a preemption check, which we'd trip.

You may also have to change these to the non-preempt checked variants.

   Julia

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Thomas Gleixner]

On Thu, 5 Oct 2017, Julia Cartwright wrote:
> On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:
> > -	preempt_disable();
> > +	preempt_disable_nort();
> >  	this_cpu_inc(*sc->buffers_allocated);
> 
> Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> this_cpu_* operations perform a preemption check, which we'd trip.

Good point. Changing this to migrate_disable() would do the trick.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Julia Cartwright]

On Thu, Oct 05, 2017 at 05:27:30PM +0200, Thomas Gleixner wrote:
> On Thu, 5 Oct 2017, Julia Cartwright wrote:
> > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:
> > > -	preempt_disable();
> > > +	preempt_disable_nort();
> > >  	this_cpu_inc(*sc->buffers_allocated);
> > 
> > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> > this_cpu_* operations perform a preemption check, which we'd trip.
> 
> Good point. Changing this to migrate_disable() would do the trick.

Wouldn't we still trip the preempt check even with migration disabled?
In another thread I asked the same question: should the preemption
checks here be converted to migration-checks in RT?

   Julia
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Steven Rostedt]

On Thu, 5 Oct 2017 10:37:59 -0500
Julia Cartwright <julia-acOepvfBmUk@public.gmane.org> wrote:

> On Thu, Oct 05, 2017 at 05:27:30PM +0200, Thomas Gleixner wrote:
> > On Thu, 5 Oct 2017, Julia Cartwright wrote:  
> > > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:  
> > > > -	preempt_disable();
> > > > +	preempt_disable_nort();
> > > >  	this_cpu_inc(*sc->buffers_allocated);  
> > > 
> > > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> > > this_cpu_* operations perform a preemption check, which we'd trip.  
> > 
> > Good point. Changing this to migrate_disable() would do the trick.  
> 
> Wouldn't we still trip the preempt check even with migration disabled?
> In another thread I asked the same question: should the preemption
> checks here be converted to migration-checks in RT?

Is it a "preemption check"? Getting a cpu # should only care about
migration. This isn't the same as a rcu_sched check is it? That does
care about preemption.

-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Julia Cartwright]

On Thu, Oct 05, 2017 at 11:55:39AM -0400, Steven Rostedt wrote:
> On Thu, 5 Oct 2017 10:37:59 -0500
> Julia Cartwright <julia@ni.com> wrote:
> 
> > On Thu, Oct 05, 2017 at 05:27:30PM +0200, Thomas Gleixner wrote:
> > > On Thu, 5 Oct 2017, Julia Cartwright wrote:  
> > > > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:  
> > > > > -	preempt_disable();
> > > > > +	preempt_disable_nort();
> > > > >  	this_cpu_inc(*sc->buffers_allocated);  
> > > > 
> > > > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> > > > this_cpu_* operations perform a preemption check, which we'd trip.  
> > > 
> > > Good point. Changing this to migrate_disable() would do the trick.  
> > 
> > Wouldn't we still trip the preempt check even with migration disabled?
> > In another thread I asked the same question: should the preemption
> > checks here be converted to migration-checks in RT?
> 
> Is it a "preemption check"?

Sorry if I was unclear, more precisely: the this_cpu_* family of
accessors, w/ CONFIG_DEBUG_PREEMPT currently spits out a warning when
the caller is invoked in a context where preemption is enabled.

The check is shared w/ the smp_processor_id() check, as implemented in
lib/smp_processor_id.c.  It effectively boils down to a check of
preempt_count() and irqs_disabled().

> Getting a cpu # should only care about migration.

I think we're agreeing? :)

> This isn't the same as a rcu_sched check is it? That does care about
> preemption.

This is something totally different, I think.

   Julia

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Thomas Gleixner]

On Thu, 5 Oct 2017, Julia Cartwright wrote:

> On Thu, Oct 05, 2017 at 11:55:39AM -0400, Steven Rostedt wrote:
> > On Thu, 5 Oct 2017 10:37:59 -0500
> > Julia Cartwright <julia@ni.com> wrote:
> > 
> > > On Thu, Oct 05, 2017 at 05:27:30PM +0200, Thomas Gleixner wrote:
> > > > On Thu, 5 Oct 2017, Julia Cartwright wrote:  
> > > > > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:  
> > > > > > -	preempt_disable();
> > > > > > +	preempt_disable_nort();
> > > > > >  	this_cpu_inc(*sc->buffers_allocated);  
> > > > > 
> > > > > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> > > > > this_cpu_* operations perform a preemption check, which we'd trip.  
> > > > 
> > > > Good point. Changing this to migrate_disable() would do the trick.  
> > > 
> > > Wouldn't we still trip the preempt check even with migration disabled?
> > > In another thread I asked the same question: should the preemption
> > > checks here be converted to migration-checks in RT?
> > 
> > Is it a "preemption check"?
> 
> Sorry if I was unclear, more precisely: the this_cpu_* family of
> accessors, w/ CONFIG_DEBUG_PREEMPT currently spits out a warning when
> the caller is invoked in a context where preemption is enabled.
> 
> The check is shared w/ the smp_processor_id() check, as implemented in
> lib/smp_processor_id.c.  It effectively boils down to a check of
> preempt_count() and irqs_disabled().

Except that on RT that check cares about the migrate disable state. You can
invoke this_cpu_* and smp_processor_id() in preemptible/interruptible
context because of:

	if (cpumask_equal(current->cpus_ptr, cpumask_of(this_cpu)))
		goto out;

That's true even on mainline.

But you are right that this check needs some update because
migrate_disable() does not immediately update the allowed cpumask IIRC.

Thanks,

	tglx

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Julia Cartwright]

On Thu, Oct 05, 2017 at 06:16:23PM +0200, Thomas Gleixner wrote:
> On Thu, 5 Oct 2017, Julia Cartwright wrote:
> 
> > On Thu, Oct 05, 2017 at 11:55:39AM -0400, Steven Rostedt wrote:
> > > On Thu, 5 Oct 2017 10:37:59 -0500
> > > Julia Cartwright <julia@ni.com> wrote:
> > > 
> > > > On Thu, Oct 05, 2017 at 05:27:30PM +0200, Thomas Gleixner wrote:
> > > > > On Thu, 5 Oct 2017, Julia Cartwright wrote:  
> > > > > > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:  
> > > > > > > -	preempt_disable();
> > > > > > > +	preempt_disable_nort();
> > > > > > >  	this_cpu_inc(*sc->buffers_allocated);  
> > > > > > 
> > > > > > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?  I believe that the
> > > > > > this_cpu_* operations perform a preemption check, which we'd trip.  
> > > > > 
> > > > > Good point. Changing this to migrate_disable() would do the trick.  
> > > > 
> > > > Wouldn't we still trip the preempt check even with migration disabled?
> > > > In another thread I asked the same question: should the preemption
> > > > checks here be converted to migration-checks in RT?
> > > 
> > > Is it a "preemption check"?
> > 
> > Sorry if I was unclear, more precisely: the this_cpu_* family of
> > accessors, w/ CONFIG_DEBUG_PREEMPT currently spits out a warning when
> > the caller is invoked in a context where preemption is enabled.
> > 
> > The check is shared w/ the smp_processor_id() check, as implemented in
> > lib/smp_processor_id.c.  It effectively boils down to a check of
> > preempt_count() and irqs_disabled().
> 
> Except that on RT that check cares about the migrate disable state. You can
> invoke this_cpu_* and smp_processor_id() in preemptible/interruptible
> context because of:
> 
> 	if (cpumask_equal(current->cpus_ptr, cpumask_of(this_cpu)))
> 		goto out;
> 
> That's true even on mainline.
> 
> But you are right that this check needs some update because
> migrate_disable() does not immediately update the allowed cpumask IIRC.

Actually, I think it does:

   migrate_disable() ->
      p = current;
      ...
      migrate_disable_update_cpus_allowed(p) ->
          p->cpus_ptr = cpumask_of(smp_processor_id())
      ...

Perhaps it's worth a simple comment update, below.

   Julia

-- 8< --
Subject: [PATCH] kernel: sched: document smp_processor_id/this_cpu safety in
 migration-disabled context

On RT, users of smp_processor_id() and this_cpu_*() per-cpu accessors
are considered safe if the caller executes with migration disabled.  On
!RT, preempt_disable() is sufficient to make this guarantee, however on
RT, the lesser migrate_disable() is sufficient.

It is not entirely obvious which check in check_preemption_disabled()
makes this work, so document it.

Signed-off-by: Julia Cartwright <julia@ni.com>
---
 lib/smp_processor_id.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index de3b2d925473..c8091d9eb1b4 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -20,7 +20,11 @@ notrace static unsigned int check_preemption_disabled(const char *what1,
 
 	/*
 	 * Kernel threads bound to a single CPU can safely use
-	 * smp_processor_id():
+	 * smp_processor_id().
+	 *
+	 * In addition, threads which are currently executing within
+	 * a migration disabled region can safely use smp_processor_id() and
+	 * this_cpu accessors.
 	 */
 	if (cpumask_equal(current->cpus_ptr, cpumask_of(this_cpu)))
 		goto out;
-- 
2.14.1

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Arnaldo Carvalho de Melo]

Em Thu, Oct 05, 2017 at 09:17:44AM -0500, Julia Cartwright escreveu:
> On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:
> > +++ b/drivers/infiniband/hw/hfi1/pio.c
> > @@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,

> >  	/* there is enough room */

> > -	preempt_disable();
> > +	preempt_disable_nort();
> >  	this_cpu_inc(*sc->buffers_allocated);

> Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?

No

> I believe that the this_cpu_* operations perform a preemption check, which we'd trip.

Humm, looking at include/linux/percpu-defs.h on v4.11.12-rt14 I see
(trimmed to what we're discussing here):

#ifdef CONFIG_DEBUG_PREEMPT
extern void __this_cpu_preempt_check(const char *op);
#else
static inline void __this_cpu_preempt_check(const char *op) { }
#endif

#define __this_cpu_add(pcp, val)					\
({									\
	__this_cpu_preempt_check("add");				\
	raw_cpu_add(pcp, val);						\
})
#define __this_cpu_inc(pcp)		__this_cpu_add(pcp, 1)

/*
 * Operations with implied preemption/interrupt protection.  These
 * operations can be used without worrying about preemption or interrupt.
 */
#define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
#define this_cpu_inc(pcp)               this_cpu_add(pcp, 1)
 
> You may also have to change these to the non-preempt checked variants.

So __this_cpu_inc() checks preemption but this_cpu_inc() doesn't and
thus we're ok here? Or am I getting lost in this maze of defines? :-)

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Julia Cartwright]

On Thu, Oct 05, 2017 at 01:53:05PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 05, 2017 at 09:17:44AM -0500, Julia Cartwright escreveu:
> > On Tue, Oct 03, 2017 at 12:49:19PM -0300, Arnaldo Carvalho de Melo wrote:
> > > +++ b/drivers/infiniband/hw/hfi1/pio.c
> > > @@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
> 
> > >  	/* there is enough room */
> 
> > > -	preempt_disable();
> > > +	preempt_disable_nort();
> > >  	this_cpu_inc(*sc->buffers_allocated);
> 
> > Have you tried this on RT w/ CONFIG_DEBUG_PREEMPT?
> 
> No
> 
> > I believe that the this_cpu_* operations perform a preemption check, which we'd trip.
>
> Humm, looking at include/linux/percpu-defs.h on v4.11.12-rt14 I see
> (trimmed to what we're discussing here):
>
> #ifdef CONFIG_DEBUG_PREEMPT
> extern void __this_cpu_preempt_check(const char *op);
> #else
> static inline void __this_cpu_preempt_check(const char *op) { }
> #endif
>
> #define __this_cpu_add(pcp, val)					\
> ({									\
> 	__this_cpu_preempt_check("add");				\
> 	raw_cpu_add(pcp, val);						\
> })
> #define __this_cpu_inc(pcp)		__this_cpu_add(pcp, 1)
>
> /*
>  * Operations with implied preemption/interrupt protection.  These
>  * operations can be used without worrying about preemption or interrupt.
>  */
> #define this_cpu_add(pcp, val)          __pcpu_size_call(this_cpu_add_, pcp, val)
> #define this_cpu_inc(pcp)               this_cpu_add(pcp, 1)
>
> > You may also have to change these to the non-preempt checked variants.
>
> So __this_cpu_inc() checks preemption but this_cpu_inc() doesn't and
> thus we're ok here? Or am I getting lost in this maze of defines? :-)

I think I was the one lost in the maze.  You are correct.  Sorry for the
confusion.

My mind expected that the __ prefixed versions would be the "raw"
versions that are free of checks, with the checks made in the non
prefixed versions, but it is the other way around.

I'm happy to accept that the bug is within my mind.

   Julia

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Arnaldo Carvalho de Melo]

Em Thu, Oct 05, 2017 at 01:29:00PM -0500, Julia Cartwright escreveu:
> On Thu, Oct 05, 2017 at 01:53:05PM -0300, Arnaldo Carvalho de Melo wrote:
> > So __this_cpu_inc() checks preemption but this_cpu_inc() doesn't and
> > thus we're ok here? Or am I getting lost in this maze of defines? :-)
 
> I think I was the one lost in the maze.  You are correct.  Sorry for the
> confusion.
 
> My mind expected that the __ prefixed versions would be the "raw"
> versions that are free of checks, with the checks made in the non
> prefixed versions, but it is the other way around.
 
> I'm happy to accept that the bug is within my mind.

:-) 

Ok, now I'm taking the opportunity to read more about local locks, as
Sebastian thinks are needed in this case, which I'm not entirely
convinced from the discussion that took place here, and as you took part
in that discussion and suggested using the nort variants of
preempt_disable, do you still think this is the way to go?

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Steven Rostedt]

On Thu, 5 Oct 2017 15:53:30 -0300
Arnaldo Carvalho de Melo <acme@kernel.org> wrote:

> Em Thu, Oct 05, 2017 at 01:29:00PM -0500, Julia Cartwright escreveu:
> > On Thu, Oct 05, 2017 at 01:53:05PM -0300, Arnaldo Carvalho de Melo wrote:  
> > > So __this_cpu_inc() checks preemption but this_cpu_inc() doesn't and
> > > thus we're ok here? Or am I getting lost in this maze of defines? :-)  
>  
> > I think I was the one lost in the maze.  You are correct.  Sorry for the
> > confusion.  
>  
> > My mind expected that the __ prefixed versions would be the "raw"
> > versions that are free of checks, with the checks made in the non
> > prefixed versions, but it is the other way around.  
>  
> > I'm happy to accept that the bug is within my mind.  
> 
> :-) 
> 
> Ok, now I'm taking the opportunity to read more about local locks, as
> Sebastian thinks are needed in this case, which I'm not entirely
> convinced from the discussion that took place here, and as you took part
> in that discussion and suggested using the nort variants of
> preempt_disable, do you still think this is the way to go?
>

Looking at
http://lkml.kernel.org/r/32E1700B9017364D9B60AED9960492BC3441B3BD@fmsmsx120.amr.corp.intel.com

I'm thinking it is fine to convert the preempt_disable() just to
preempt_disable_nort() (where it's a nop when PREEMPT_RT is enabled).
In the mentioned thread, the counters do not seem to be important to
the current CPU, just that they get incremented by "some" CPU.

Adding back the patch for Mike:

diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
index 615be68e40b3..3a30bde9a07b 100644
--- a/drivers/infiniband/hw/hfi1/pio.c
+++ b/drivers/infiniband/hw/hfi1/pio.c
@@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
 
 	/* there is enough room */
 
-	preempt_disable();
+	preempt_disable_nort();
 	this_cpu_inc(*sc->buffers_allocated);
 
 	/* read this once */
diff --git a/drivers/infiniband/hw/hfi1/pio_copy.c b/drivers/infiniband/hw/hfi1/pio_copy.c
index 03024cec78dd..c3f48f705c97 100644
--- a/drivers/infiniband/hw/hfi1/pio_copy.c
+++ b/drivers/infiniband/hw/hfi1/pio_copy.c
@@ -162,7 +162,7 @@ void pio_copy(struct hfi1_devdata *dd, struct pio_buf *pbuf, u64 pbc,
 
 	/* finished with this buffer */
 	this_cpu_dec(*pbuf->sc->buffers_allocated);
-	preempt_enable();
+	preempt_enable_nort();
 }
 
 /*
@@ -753,5 +753,5 @@ void seg_pio_copy_end(struct pio_buf *pbuf)
 
 	/* finished with this buffer */
 	this_cpu_dec(*pbuf->sc->buffers_allocated);
-	preempt_enable();
+	preempt_enable_nort();
 }


The above will only disable preemption when PREEMPT_RT is not enabled.
Honestly, if it's just an optimization, and the counters are not that
important, do we care on non PREEMPT_RT to disable preemption, and can
we just remove the preempt_disable() totally? Depending if it triggers
warnings using this_cpu_inc/dec() from preempt enabled regions.

-- Steve

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Sebastian Andrzej Siewior]

On 2017-10-03 12:49:19 [-0300], Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
> index 615be68e40b3..3a30bde9a07b 100644
> --- a/drivers/infiniband/hw/hfi1/pio.c
> +++ b/drivers/infiniband/hw/hfi1/pio.c
> @@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
>  
>  	/* there is enough room */
>  
> -	preempt_disable();
> +	preempt_disable_nort();
>  	this_cpu_inc(*sc->buffers_allocated);
>  
>  	/* read this once */

please replace the preempt_disable() / enable with local_lock() /
unlock. The section does not look like it could cope with multiple users
dereferencing / using the same per-CPU variables.

Sebastian

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Sebastian Andrzej Siewior]

On 2017-10-05 18:30:19 [+0200], To Arnaldo Carvalho de Melo wrote:
> On 2017-10-03 12:49:19 [-0300], Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
> > index 615be68e40b3..3a30bde9a07b 100644
> > --- a/drivers/infiniband/hw/hfi1/pio.c
> > +++ b/drivers/infiniband/hw/hfi1/pio.c
> > @@ -1421,7 +1421,7 @@ struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len,
> >  
> >  	/* there is enough room */
> >  
> > -	preempt_disable();
> > +	preempt_disable_nort();
> >  	this_cpu_inc(*sc->buffers_allocated);
> >  
> >  	/* read this once */
> 
> please replace the preempt_disable() / enable with local_lock() /
> unlock. The section does not look like it could cope with multiple users
> dereferencing / using the same per-CPU variables.

ignore me, there is a spin_lock to protect that…

Sebastian

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Dennis Dalessandro]

On 10/3/2017 11:49 AM, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> sc_buffer_alloc() disables preemption that will be reenabled by either
> pio_copy() or seg_pio_copy_end(). But before disabling preemption it
> grabs a spin lock that will be dropped after it disables preemption,
> which ends up triggering a warning in migrate_disable() later on.
> 
>      spin_lock_irqsave(&sc->alloc_lock)
>        migrate_disable() ++p->migrate_disable -> 2
>      preempt_disable()
>      spin_unlock_irqrestore(&sc->alloc_lock)
>        migrate_enable() in_atomic(), so just returns, migrate_disable stays at 2
>      spin_lock_irqsave(some other lock) -> b00m
> 
> And the WARN_ON code ends up tripping over this over and over in
> log_store().
> 
> Sequence captured via ftrace_dump_on_oops + crash utility 'dmesg'
> command.
> 
> [512258.613862] sm-3297 16 .....11 359465349134644: sc_buffer_alloc <-hfi1_verbs_send_pio
> [512258.613876] sm-3297 16 .....11 359465349134719: migrate_disable <-sc_buffer_alloc
> [512258.613890] sm-3297 16 .....12 359465349134798: rt_spin_lock <-sc_buffer_alloc
> [512258.613903] sm-3297 16 ....112 359465349135481: rt_spin_unlock <-sc_buffer_alloc
> [512258.613916] sm-3297 16 ....112 359465349135556: migrate_enable <-sc_buffer_alloc
> [512258.613935] sm-3297 16 ....112 359465349135788: seg_pio_copy_start <-hfi1_verbs_send_pio
> [512258.613954] sm-3297 16 ....112 359465349136273: update_sge <-hfi1_verbs_send_pio
> [512258.613981] sm-3297 16 ....112 359465349136373: seg_pio_copy_mid <-hfi1_verbs_send_pio
> [512258.613999] sm-3297 16 ....112 359465349136873: update_sge <-hfi1_verbs_send_pio
> [512258.614017] sm-3297 16 ....112 359465349136956: seg_pio_copy_mid <-hfi1_verbs_send_pio
> [512258.614035] sm-3297 16 ....112 359465349137221: seg_pio_copy_end <-hfi1_verbs_send_pio
> [512258.614048] sm-3297 16 .....12 359465349137360: migrate_disable <-hfi1_verbs_send_pio
> [512258.614065] sm-3297 16 .....12 359465349137476: warn_slowpath_null <-migrate_disable
> [512258.614081] sm-3297 16 .....12 359465349137564: __warn <-warn_slowpath_null
> [512258.614088] sm-3297 16 .....12 359465349137958: printk <-__warn
> [512258.614096] sm-3297 16 .....12 359465349138055: vprintk_default <-printk
> [512258.614104] sm-3297 16 .....12 359465349138144: vprintk_emit <-vprintk_default
> [512258.614111] sm-3297 16 d....12 359465349138312: _raw_spin_lock <-vprintk_emit
> [512258.614119] sm-3297 16 d...112 359465349138789: log_store <-vprintk_emit
> [512258.614127] sm-3297 16 .....12 359465349139068: migrate_disable <-vprintk_emit
> 
> According to a discussion (see Link: below) on the linux-rt-users
> mailing list, this locking is done for performance reasons, not for
> correctness, so use the _nort() variants to avoid the above problem.
> 
> Suggested-by: Julia Cartwright <julia-acOepvfBmUk@public.gmane.org>
> Cc: Clark Williams <williams-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Dean Luick <dean.luick-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Sebastian Andrzej Siewior <sebastian.siewior-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Sebastian Sanchez <sebastian.sanchez-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Link: http://lkml.kernel.org/r/20170926210045.GO29872-ew3lsbMjNqt5wtABiV/Xjqyly8cj88Ttqxv4g6HH51o@public.gmane.org
> Signed-off-by: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

I assume you are not asking for Doug to pick this up for linux-rdma and 
that this more of an RFC sort of deal and the intended destination is 
the -rt tree? Anyway, for this patch:

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Arnaldo Carvalho de Melo]

Em Tue, Oct 10, 2017 at 02:59:18PM -0400, Dennis Dalessandro escreveu:
> On 10/3/2017 11:49 AM, Arnaldo Carvalho de Melo wrote:
> > From: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> > sc_buffer_alloc() disables preemption that will be reenabled by either
> > pio_copy() or seg_pio_copy_end(). But before disabling preemption it
> > grabs a spin lock that will be dropped after it disables preemption,
> > which ends up triggering a warning in migrate_disable() later on.
> > 
> >      spin_lock_irqsave(&sc->alloc_lock)
> >        migrate_disable() ++p->migrate_disable -> 2
> >      preempt_disable()
> >      spin_unlock_irqrestore(&sc->alloc_lock)
> >        migrate_enable() in_atomic(), so just returns, migrate_disable stays at 2
> >      spin_lock_irqsave(some other lock) -> b00m
> > 
> > And the WARN_ON code ends up tripping over this over and over in
> > log_store().
> > 
> > Sequence captured via ftrace_dump_on_oops + crash utility 'dmesg'
> > command.
> > 
> > [512258.613862] sm-3297 16 .....11 359465349134644: sc_buffer_alloc <-hfi1_verbs_send_pio
> > [512258.613876] sm-3297 16 .....11 359465349134719: migrate_disable <-sc_buffer_alloc
> > [512258.613890] sm-3297 16 .....12 359465349134798: rt_spin_lock <-sc_buffer_alloc
> > [512258.613903] sm-3297 16 ....112 359465349135481: rt_spin_unlock <-sc_buffer_alloc
> > [512258.613916] sm-3297 16 ....112 359465349135556: migrate_enable <-sc_buffer_alloc
> > [512258.613935] sm-3297 16 ....112 359465349135788: seg_pio_copy_start <-hfi1_verbs_send_pio
> > [512258.613954] sm-3297 16 ....112 359465349136273: update_sge <-hfi1_verbs_send_pio
> > [512258.613981] sm-3297 16 ....112 359465349136373: seg_pio_copy_mid <-hfi1_verbs_send_pio
> > [512258.613999] sm-3297 16 ....112 359465349136873: update_sge <-hfi1_verbs_send_pio
> > [512258.614017] sm-3297 16 ....112 359465349136956: seg_pio_copy_mid <-hfi1_verbs_send_pio
> > [512258.614035] sm-3297 16 ....112 359465349137221: seg_pio_copy_end <-hfi1_verbs_send_pio
> > [512258.614048] sm-3297 16 .....12 359465349137360: migrate_disable <-hfi1_verbs_send_pio
> > [512258.614065] sm-3297 16 .....12 359465349137476: warn_slowpath_null <-migrate_disable
> > [512258.614081] sm-3297 16 .....12 359465349137564: __warn <-warn_slowpath_null
> > [512258.614088] sm-3297 16 .....12 359465349137958: printk <-__warn
> > [512258.614096] sm-3297 16 .....12 359465349138055: vprintk_default <-printk
> > [512258.614104] sm-3297 16 .....12 359465349138144: vprintk_emit <-vprintk_default
> > [512258.614111] sm-3297 16 d....12 359465349138312: _raw_spin_lock <-vprintk_emit
> > [512258.614119] sm-3297 16 d...112 359465349138789: log_store <-vprintk_emit
> > [512258.614127] sm-3297 16 .....12 359465349139068: migrate_disable <-vprintk_emit
> > 
> > According to a discussion (see Link: below) on the linux-rt-users
> > mailing list, this locking is done for performance reasons, not for
> > correctness, so use the _nort() variants to avoid the above problem.
> > 
> > Suggested-by: Julia Cartwright <julia-acOepvfBmUk@public.gmane.org>
> > Cc: Clark Williams <williams-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Dean Luick <dean.luick-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Kaike Wan <kaike.wan-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> > Cc: Sebastian Andrzej Siewior <sebastian.siewior-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > Cc: Sebastian Sanchez <sebastian.sanchez-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Steven Rostedt <rostedt-nx8X9YLhiw1AfugRpC6u6w@public.gmane.org>
> > Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> > Link: http://lkml.kernel.org/r/20170926210045.GO29872-ew3lsbMjNqt5wtABiV/Xjqyly8cj88Ttqxv4g6HH51o@public.gmane.org
> > Signed-off-by: Arnaldo Carvalho de Melo <acme-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> I assume you are not asking for Doug to pick this up for linux-rdma and that
> this more of an RFC sort of deal and the intended destination is the -rt

Right, and so far there were no strong objection for this one to be
merged on the -rt tree, Sebastian, can you do it please? Adding Dennis'
reviewed-by, one of maintainers for this driver, ok?

> tree? Anyway, for this patch:
> 
> Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Sebastian Andrzej Siewior]

On 2017-10-10 16:02:18 [-0300], Arnaldo Carvalho de Melo wrote:
> 
> Right, and so far there were no strong objection for this one to be
> merged on the -rt tree, Sebastian, can you do it please? Adding Dennis'
> reviewed-by, one of maintainers for this driver, ok?

I am still curious about the performance improvement that is with this
preempt disable section compared to without it compared to !PREEMPT
kernel..
If that is important then migrate_disable() would do that on RT.
I guess that there were no splat with CONFIG_DEBUG_PREEMPT ?
If that is all okay, please resend the patch with the explanation why
this preempt_disable() does not matter and I pick it up.

> > tree? Anyway, for this patch:
> > 
> > Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] IB/hfi1: Use preempt_{dis,en}able_nort()

[Arnaldo Carvalho de Melo]

Em Wed, Oct 11, 2017 at 01:03:55PM +0200, Sebastian Andrzej Siewior escreveu:
> On 2017-10-10 16:02:18 [-0300], Arnaldo Carvalho de Melo wrote:
> > 
> > Right, and so far there were no strong objection for this one to be
> > merged on the -rt tree, Sebastian, can you do it please? Adding Dennis'
> > reviewed-by, one of maintainers for this driver, ok?
> 
> I am still curious about the performance improvement that is with this
> preempt disable section compared to without it compared to !PREEMPT
> kernel..
> If that is important then migrate_disable() would do that on RT.

I can try that

> I guess that there were no splat with CONFIG_DEBUG_PREEMPT ?

I haven't tried that

> If that is all okay, please resend the patch with the explanation why
> this preempt_disable() does not matter and I pick it up.

I can just pick the explanation given by the authors and stash it there,
I guess.
 
> > > tree? Anyway, for this patch:
> > > 
> > > Reviewed-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> 
> Sebastian

[PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The hfi1 driver calls request_threaded_irq with two parameters:

      handler = receive_context_interrupt;
      thread = receive_context_thread;
      request_threaded_irq(me->msix.vector, handler, thread, 0, me->name, arg);

And tries to process packets on the hard irq one, receive_context_interrupt(),
only waking up the thread (returning IRQ_WAKE_THREAD) when some threshold is
crossed in the number of packets available in the NIC, trying to balance
latency and bandwidth.

But in a CONFIG_PREEMPT_RT_FULL kernel it ends up calling spin locks from the
hard irq handler (receive_context_interrupt) which causes BUGs like this:

  [ 1002.740581] hfi1 0000:21:00.0: hfi1_0: set_link_state: current ARMED, new ACTIVE
  [ 1002.740583] hfi1 0000:21:00.0: hfi1_0: logical state changed to PORT_ACTIVE (0x4)
  [ 1002.740599] hfi1 0000:21:00.0: hfi1_0: send_idle_message: sending idle message 0x203
  [ 1002.741873] hfi1 0000:21:00.0: hfi1_0: read_idle_message: read idle message 0x203
  [ 1002.741874] hfi1 0000:21:00.0: hfi1_0: handle_sma_message: SMA message 0x2
  [ 1002.741923] hfi1 0000:21:00.0: hfi1_0: Switching to NO_DMA_RTAIL
  [ 1004.744192] IPv6: ADDRCONF(NETDEV_CHANGE): hfi1_opa0: link becomes ready
  [ 1167.907754] ------------[ cut here ]------------
  [ 1167.907756] kernel BUG at kernel/rtmutex.c:902!
  [ 1167.907758] invalid opcode: 0000 [#1] PREEMPT SMP
  <SNIP>
  [ 1167.907805] CPU: 10 PID: 1505 Comm: hfi1_cq0 Not tainted 3.10.0-708.rt56.635.test.el7.x86_64 #1
  <SNIP>
  [ 1167.907823] Call Trace:
  [ 1167.907826]  <IRQ>
  [ 1167.907850]  [<ffffffffc06e4981>] ? hfi1_rvt_get_rwqe+0x141/0x400 [hfi1]
  [ 1167.907852]  [<ffffffff816b7625>] rt_spin_lock+0x25/0x30
  [ 1167.907856]  [<ffffffff810aa774>] queue_kthread_work+0x24/0x60
  [ 1167.907861]  [<ffffffffc068845b>] rvt_cq_enter+0x17b/0x250 [rdmavt]
  [ 1167.907869]  [<ffffffffc06e391a>] hfi1_rc_rcv+0x67a/0x1260 [hfi1]
  [ 1167.907878]  [<ffffffffc06fefc8>] hfi1_ib_rcv+0x2c8/0x400 [hfi1]
  [ 1167.907886]  [<ffffffffc06c381c>] process_receive_ib+0x6c/0x150 [hfi1]
  [ 1167.907888]  [<ffffffff810cee9d>] ? enqueue_pushable_task+0x6d/0x90
  [ 1167.907895]  [<ffffffffc06c1f31>] handle_receive_interrupt_nodma_rtail+0x161/0x310 [hfi1]
  [ 1167.907914]  [<ffffffffc06b49d3>] receive_context_interrupt+0x53/0x390 [hfi1]
  [ 1167.907917]  [<ffffffff8112fb26>] __handle_irq_event_percpu+0x56/0x240
  [ 1167.907919]  [<ffffffff816b7616>] ? rt_spin_lock+0x16/0x30
  [ 1167.907920]  [<ffffffff8112fd59>] handle_irq_event_percpu+0x49/0xa0
  [ 1167.907922]  [<ffffffff8112fe28>] handle_irq_event+0x78/0xb0
  [ 1167.907924]  [<ffffffff81132d29>] handle_edge_irq+0x99/0x1a0
  [ 1167.907926]  [<ffffffff8101ea7b>] handle_irq+0xbb/0x150
  [ 1167.907929]  [<ffffffff816c298d>] do_IRQ+0x4d/0xe0
  [ 1167.907931]  [<ffffffff816b7fad>] common_interrupt+0x6d/0x6d
  [ 1167.907931]  <EOI>
  [ 1167.907932]  [<ffffffff816b7616>] ? rt_spin_lock+0x16/0x30
  [ 1167.907934]  [<ffffffff810aaa55>] ? kthread_worker_fn+0xb5/0x170
  [ 1167.907935]  [<ffffffff810aa9a0>] ? flush_kthread_work+0x130/0x130
  [ 1167.907937]  [<ffffffff810aabdf>] kthread+0xcf/0xe0
  [ 1167.907938]  [<ffffffff810aab10>] ? kthread_worker_fn+0x170/0x170
  [ 1167.907940]  [<ffffffff816c0498>] ret_from_fork+0x58/0x90
  [ 1167.907941]  [<ffffffff810aab10>] ? kthread_worker_fn+0x170/0x170
  [ 1167.907951] Code: 90 e8 eb f0 ff ff e9 d4 fd ff ff 66 0f 1f 44 00 00 e8 db f0 ff ff eb b6 0f 0b 0f 1f 80 00 00 00 00 e8 0b f7 a3 ff e8 46 86 9c ff <0f> 0b 0f 0b 66 90 0f 1f 44 00 00 55 48 89 e5 41 57 65 4c 8b 3c
  [ 1167.907952] RIP  [<ffffffff816b62fa>] rt_spin_lock_slowlock+0x34a/0x350
  [ 1167.907952]  RSP <ffff880c3f403ad0>

To get it to work on RT just keep the prologue that clears the chip receive
interrupt and immediately return IRQ_WAKE_THREAD, deferring all packet
processing, with its locking, to the thread.

With this test systems are able to pass traffic over this hardware using a
CONFIG_PREEMPT_RT_FULL patched kernel without triggering these BUGs.

Cc: Clark Williams <williams@redhat.com>
Cc: Dean Luick <dean.luick@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Doug Ledford <dledford@redhat.com>
Cc: Julia Cartwright <julia@ni.com>
Cc: Kaike Wan <kaike.wan@intel.com>
Cc: Leon Romanovsky <leonro@mellanox.com>
Cc: linux-rdma@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sebastian Andrzej Siewior <sebastian.siewior@linutronix.de>
Cc: Sebastian Sanchez <sebastian.sanchez@intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 drivers/infiniband/hw/hfi1/chip.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
index 121a4c920f1b..733a00d8ea4c 100644
--- a/drivers/infiniband/hw/hfi1/chip.c
+++ b/drivers/infiniband/hw/hfi1/chip.c
@@ -8226,15 +8226,17 @@ static irqreturn_t receive_context_interrupt(int irq, void *data)
 {
 	struct hfi1_ctxtdata *rcd = data;
 	struct hfi1_devdata *dd = rcd->dd;
-	int disposition;
-	int present;
 
 	trace_hfi1_receive_interrupt(dd, rcd->ctxt);
 	this_cpu_inc(*dd->int_counter);
 	aspm_ctx_disable(rcd);
 
+#ifdef CONFIG_PREEMPT_RT_FULL
+	return IRQ_WAKE_THREAD;
+#else
+{
 	/* receive interrupt remains blocked while processing packets */
-	disposition = rcd->do_interrupt(rcd, 0);
+	int disposition = rcd->do_interrupt(rcd, 0), present;
 
 	/*
 	 * Too many packets were seen while processing packets in this
@@ -8257,6 +8259,8 @@ static irqreturn_t receive_context_interrupt(int irq, void *data)
 
 	return IRQ_HANDLED;
 }
+#endif
+}
 
 /*
  * Receive packet thread handler.  This expects to be invoked with the
-- 
2.13.6

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Sebastian Andrzej Siewior]

On 2017-10-03 12:49:20 [-0300], Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> The hfi1 driver calls request_threaded_irq with two parameters:
> 
>       handler = receive_context_interrupt;
>       thread = receive_context_thread;
>       request_threaded_irq(me->msix.vector, handler, thread, 0, me->name, arg);
> 
> And tries to process packets on the hard irq one, receive_context_interrupt(),
> only waking up the thread (returning IRQ_WAKE_THREAD) when some threshold is
> crossed in the number of packets available in the NIC, trying to balance
> latency and bandwidth.
> 
> But in a CONFIG_PREEMPT_RT_FULL kernel it ends up calling spin locks from the
> hard irq handler (receive_context_interrupt) which causes BUGs like this:

If I am not mistaken current devel-tree of RT (and a few releases before
that) handle that case correctly and force-thread both threads.

Sebastian

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Dennis Dalessandro]

On 10/3/2017 11:49 AM, Arnaldo Carvalho de Melo wrote:
> Cc: Clark Williams <williams@redhat.com>
> Cc: Dean Luick <dean.luick@intel.com>
> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Cc: Doug Ledford <dledford@redhat.com>
> Cc: Julia Cartwright <julia@ni.com>
> Cc: Kaike Wan <kaike.wan@intel.com>
> Cc: Leon Romanovsky <leonro@mellanox.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Sebastian Andrzej Siewior <sebastian.siewior@linutronix.de>
> Cc: Sebastian Sanchez <sebastian.sanchez@intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Mike has been working with these developers off list and has provided a 
slightly different approach. I'm going to let them drive that before I 
add an RB/Ack here.

-Dennis

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Arnaldo Carvalho de Melo]

Em Tue, Oct 10, 2017 at 03:06:00PM -0400, Dennis Dalessandro escreveu:
> On 10/3/2017 11:49 AM, Arnaldo Carvalho de Melo wrote:
> > Cc: Clark Williams <williams@redhat.com>
> > Cc: Dean Luick <dean.luick@intel.com>
> > Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
> > Cc: Doug Ledford <dledford@redhat.com>
> > Cc: Julia Cartwright <julia@ni.com>
> > Cc: Kaike Wan <kaike.wan@intel.com>
> > Cc: Leon Romanovsky <leonro@mellanox.com>
> > Cc: linux-rdma@vger.kernel.org
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Sebastian Andrzej Siewior <sebastian.siewior@linutronix.de>
> > Cc: Sebastian Sanchez <sebastian.sanchez@intel.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Mike has been working with these developers off list and has provided a
> slightly different approach. I'm going to let them drive that before I add
> an RB/Ack here.

This may not even be needed, i.e. Sebastian Siewior said that recent
rt patches already identify the way this driver uses threaded interrupts
and automagically does the right thing, Sebastian?

He mentioned this in this message:

https://marc.info/?l=linux-rt-users&m=150722087514307&w=2

But I replied to Mike and don't recall getting an answer to my last
question.

Meanwhile a kernel with these two patches survived the original bug
reported at Red Hat's bugzilla.

- Arnaldo

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Sebastian Andrzej Siewior]

On 2017-10-10 16:15:29 [-0300], Arnaldo Carvalho de Melo wrote:
> This may not even be needed, i.e. Sebastian Siewior said that recent
> rt patches already identify the way this driver uses threaded interrupts
> and automagically does the right thing, Sebastian?
> 
> He mentioned this in this message:
> 
> https://marc.info/?l=linux-rt-users&m=150722087514307&w=2
> 
> But I replied to Mike and don't recall getting an answer to my last
> question.

correct, since 4.1.7-rt8
  https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=v4.1.7-rt8-patches&id=7461758b9e982e4ea6280ce9308492e7cceda2ed

> - Arnaldo

Sebastian

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Arnaldo Carvalho de Melo]

Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:
> On 2017-10-10 16:15:29 [-0300], Arnaldo Carvalho de Melo wrote:
> > This may not even be needed, i.e. Sebastian Siewior said that recent
> > rt patches already identify the way this driver uses threaded interrupts
> > and automagically does the right thing, Sebastian?
> > 
> > He mentioned this in this message:
> > 
> > https://marc.info/?l=linux-rt-users&m=150722087514307&w=2
> > 
> > But I replied to Mike and don't recall getting an answer to my last
> > question.
> 
> correct, since 4.1.7-rt8
>   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=v4.1.7-rt8-patches&id=7461758b9e982e4ea6280ce9308492e7cceda2ed

Ok, I'll try backporting that change and will try without 2/2.

- Arnaldo

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Arnaldo Carvalho de Melo]

Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:
> On 2017-10-10 16:15:29 [-0300], Arnaldo Carvalho de Melo wrote:
> > This may not even be needed, i.e. Sebastian Siewior said that recent
> > rt patches already identify the way this driver uses threaded interrupts
> > and automagically does the right thing, Sebastian?
> > 
> > He mentioned this in this message:
> > 
> > https://marc.info/?l=linux-rt-users&m=150722087514307&w=2
> > 
> > But I replied to Mike and don't recall getting an answer to my last
> > question.
> 
> correct, since 4.1.7-rt8
>   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=v4.1.7-rt8-patches&id=7461758b9e982e4ea6280ce9308492e7cceda2ed

Ouch, it seems I looked at the wrong place, as patches-4.11.12-rt14
doesn't have this one, right?

Off I go trying to make sense of this...

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] IB/hfi1: Handle packets in the theaded handler only

[Arnaldo Carvalho de Melo]

Em Wed, Oct 11, 2017 at 04:07:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:
> > On 2017-10-10 16:15:29 [-0300], Arnaldo Carvalho de Melo wrote:
> > > This may not even be needed, i.e. Sebastian Siewior said that recent
> > > rt patches already identify the way this driver uses threaded interrupts
> > > and automagically does the right thing, Sebastian?
> > > 
> > > He mentioned this in this message:
> > > 
> > > https://marc.info/?l=linux-rt-users&m=150722087514307&w=2
> > > 
> > > But I replied to Mike and don't recall getting an answer to my last
> > > question.
> > 
> > correct, since 4.1.7-rt8
> >   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/commit/?h=v4.1.7-rt8-patches&id=7461758b9e982e4ea6280ce9308492e7cceda2ed
> 
> Ouch, it seems I looked at the wrong place, as patches-4.11.12-rt14
> doesn't have this one, right?
> 
> Off I go trying to make sense of this...

Ok, this was merged upstream on v4.4:

[acme@jouet linux]$ git log -1 --oneline 2a1d3ab8986d1
2a1d3ab8986d genirq: Handle force threading of irqs with primary and thread handler

nevermind :-)

- Arnaldo