infiniband hfi1 PREEMPT_RT_FULL changes

GIT PULL In review Arnaldo Carvalho de Melo 7 participants 27 messages Oct 3, 2017 – Oct 11, 2017

Subsystems (3)

Library code InfiniBand HFI1 driver

lore

Share & tools

lore

Arnaldo Carvalho de Melo

author

2017-10-03 15:49 UTC #0 lore raw

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

Arnaldo Carvalho de Melo

author

2017-10-03 15:49 UTC #1 lore raw

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

From: Arnaldo Carvalho de Melo <[email protected]>

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.

Cc Clark Williams <[email protected]>

Cc Dean Luick <[email protected]>

Cc Dennis Dalessandro <[email protected]>

Cc Doug Ledford <[email protected]>

Cc Kaike Wan <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Cc Sebastian Sanchez <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Thomas Gleixner <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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

drivers/infiniband/hw/hfi1/pio.c +1 -1

index 615be68e40b3..3a30bde9a07b 100644
@@ -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 */

drivers/infiniband/hw/hfi1/pio_copy.c +2 -2

index 03024cec78dd..c3f48f705c97 100644
@@ -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();
 }

Signature · 1 line

2.13.6

Julia Cartwright

2017-10-05 14:17 UTC #2 lore raw

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

Quoted 70 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>
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 <[email protected]>
Cc Clark Williams <[email protected]>
Cc Dean Luick <[email protected]>
Cc Dennis Dalessandro <[email protected]>
Cc Doug Ledford <[email protected]>
Cc Kaike Wan <[email protected]>
Cc Leon Romanovsky <[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Andrzej Siewior <[email protected]>
Cc Sebastian Sanchez <[email protected]>
Cc Steven Rostedt <[email protected]>
Cc Thomas Gleixner <[email protected]>
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 <[email protected]>
---
 drivers/infiniband/hw/hfi1/pio.c      | 2 +-
 drivers/infiniband/hw/hfi1/pio_copy.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/infiniband/hw/hfi1/pio.c +0 -0
index 615be68e40b3..3a30bde9a07b 100644

drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425

Quoted 6 lines

drivers/infiniband/hw/hfi1/pio.c +1 -1

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

Thomas Gleixner

2017-10-05 15:27 UTC #3 lore raw

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

Quoted 7 lines · Julia Cartwright

drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 3 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c +1 -1
-	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Julia Cartwright

2017-10-05 15:37 UTC #4 lore raw

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

Quoted 10 lines · Thomas Gleixner

Quoted 7 lines · Julia Cartwright
drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 3 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c +1 -1
-	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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Steven Rostedt

2017-10-05 15:55 UTC #5 lore raw

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

Quoted 10 lines · On Thu, 5 Oct 2017 10:37:59 -0500 Julia Cartwright…

Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Julia Cartwright
drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 3 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c +1 -1
-	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.

Quoted 5 lines

Quoted 1 line
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Julia Cartwright

2017-10-05 16:05 UTC #6 lore raw

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

Quoted 13 lines · Steven Rostedt

Quoted 10 lines · On Thu, 5 Oct 2017 10:37:59 -0500 Julia Cartwright <[email protected]> wrote:
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Julia Cartwright
drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 3 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c +1 -1
-	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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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().

Quoted 1 line

Getting a cpu # should only care about migration.

I think we're agreeing? :)

Quoted 2 lines

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

Thomas Gleixner

2017-10-05 16:16 UTC #7 lore raw

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

Quoted 22 lines · Julia Cartwright

Quoted 13 lines · Steven Rostedt
Quoted 10 lines · On Thu, 5 Oct 2017 10:37:59 -0500 Julia Cartwright <[email protected]> wrote:
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Julia Cartwright
drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 3 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c +1 -1
-	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.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
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"?

Quoted 7 lines

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

Julia Cartwright

2017-10-05 16:39 UTC #8 lore raw

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

Quoted 36 lines · Thomas Gleixner

Quoted 22 lines · Julia Cartwright
Quoted 13 lines · Steven Rostedt
Quoted 10 lines · On Thu, 5 Oct 2017 10:37:59 -0500 Julia Cartwright <[email protected]> wrote:
Quoted 9 lines · Thomas Gleixner
Quoted 7 lines · Julia Cartwright
Review
Quoted 3 lines · Arnaldo Carvalho de Melo
(quoted diff) +1 -1
-	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.
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
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"?
Quoted 7 lines
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:

Quoted 7 lines

	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 <[email protected]>

---
 lib/smp_processor_id.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

lib/smp_processor_id.c +5 -1

index de3b2d925473..c8091d9eb1b4 100644
@@ -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;

Signature · 1 line

2.14.1

Arnaldo Carvalho de Melo

author

2017-10-05 16:53 UTC #9 lore raw

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

In reply to Julia Cartwright

Em Thu, Oct 05, 2017 at 09:17:44AM -0500, Julia Cartwright escreveu:

Quoted 11 lines · Arnaldo Carvalho de Melo

drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 8 lines
+++ b/drivers/infiniband/hw/hfi1/pio.c
drivers/infiniband/hw/hfi1/pio.c +1 -1
@@ -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?

Quoted 1 line

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)

Quoted 1 line

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Julia Cartwright

2017-10-05 18:29 UTC #10 lore raw

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

Quoted 40 lines · Arnaldo Carvalho de Melo

Em Thu, Oct 05, 2017 at 09:17:44AM -0500, Julia Cartwright escreveu:
Quoted 11 lines · Arnaldo Carvalho de Melo
drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1425
Quoted 8 lines
+++ b/drivers/infiniband/hw/hfi1/pio.c
drivers/infiniband/hw/hfi1/pio.c +1 -1
@@ -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
Quoted 1 line
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)

Quoted 4 lines

Quoted 1 line
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

Arnaldo Carvalho de Melo

author

2017-10-05 18:53 UTC #11 lore raw

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

Em Thu, Oct 05, 2017 at 01:29:00PM -0500, Julia Cartwright escreveu:

Quoted 12 lines · Arnaldo Carvalho de Melo

Quoted 2 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Steven Rostedt

2017-10-05 19:15 UTC #12 lore raw

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

Quoted 14 lines · On Thu, 5 Oct 2017 15:53:30 -0300 Arnaldo Carvalho de Melo <[email protected]>…

Em Thu, Oct 05, 2017 at 01:29:00PM -0500, Julia Cartwright escreveu:
Quoted 12 lines · Arnaldo Carvalho de Melo
Quoted 2 lines
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.

Quoted 8 lines

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

drivers/infiniband/hw/hfi1/pio.c +1 -1

index 615be68e40b3..3a30bde9a07b 100644
@@ -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 */

drivers/infiniband/hw/hfi1/pio_copy.c +2 -2

index 03024cec78dd..c3f48f705c97 100644
@@ -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

Sebastian Andrzej Siewior

2017-10-05 16:30 UTC #13 lore raw

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

In reply to Arnaldo Carvalho de Melo

Quoted 7 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>
drivers/infiniband/hw/hfi1/pio.c +0 -0
index 615be68e40b3..3a30bde9a07b 100644

drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1427

Quoted 8 lines

drivers/infiniband/hw/hfi1/pio.c +1 -1

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

Sebastian Andrzej Siewior

2017-10-06 09:19 UTC #14 lore raw

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

Quoted 15 lines · To Arnaldo Carvalho de Melo

Quoted 14 lines · Arnaldo Carvalho de Melo
From: Arnaldo Carvalho de Melo <[email protected]>
drivers/infiniband/hw/hfi1/pio.c +1 -1
index 615be68e40b3..3a30bde9a07b 100644
@@ -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);
 

Quoted 5 lines

drivers/infiniband/hw/hfi1/pio.c struct pio_buf *sc_buffer_alloc(struct send_context *sc, u32 dw_len, · line 1427
Quoted 1 line
drivers/infiniband/hw/hfi1/pio.c +0 -0
 	/* 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

Dennis Dalessandro

Reviewed-by on v1 1/2

2017-10-10 18:59 UTC #15 lore raw

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

In reply to Arnaldo Carvalho de Melo

Quoted 45 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

     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

Quoted 15 lines

Suggested-by Julia Cartwright <[email protected]>
Cc Clark Williams <[email protected]>
Cc Dean Luick <[email protected]>
Cc Dennis Dalessandro <[email protected]>
Cc Doug Ledford <[email protected]>
Cc Kaike Wan <[email protected]>
Cc Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Andrzej Siewior <[email protected]>
Cc Sebastian Sanchez <[email protected]>
Cc Steven Rostedt <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/20170926210045.GO29872-ew3lsbMjNqt5wtABiV/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnaldo Carvalho de Melo

author

2017-10-10 19:02 UTC #16 lore raw

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

Em Tue, Oct 10, 2017 at 02:59:18PM -0400, Dennis Dalessandro escreveu:

Quoted 62 lines · Arnaldo Carvalho de Melo

Quoted 45 lines
From: Arnaldo Carvalho de Melo <[email protected]>
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.
Quoted 15 lines
Suggested-by Julia Cartwright <[email protected]>
Cc Clark Williams <[email protected]>
Cc Dean Luick <[email protected]>
Cc Dennis Dalessandro <[email protected]>
Cc Doug Ledford <[email protected]>
Cc Kaike Wan <[email protected]>
Cc Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Andrzej Siewior <[email protected]>
Cc Sebastian Sanchez <[email protected]>
Cc Steven Rostedt <[email protected]>
Cc Thomas Gleixner <[email protected]>
Link http://lkml.kernel.org/r/20170926210045.GO29872-ew3lsbMjNqt5wtABiV/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

Quoted 2 lines

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?

Quoted 3 lines

tree? Anyway, for this patch:
Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sebastian Andrzej Siewior

2017-10-11 11:03 UTC #17 lore raw

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

Quoted 4 lines · Arnaldo Carvalho de Melo

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.

Quoted 3 lines

Quoted 3 lines
tree? Anyway, for this patch:
Reviewed-by Dennis Dalessandro <[email protected]>

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnaldo Carvalho de Melo

author

2017-10-11 13:43 UTC #18 lore raw

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

Em Wed, Oct 11, 2017 at 01:03:55PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 10 lines · Arnaldo Carvalho de Melo

Quoted 4 lines
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

Quoted 1 line

I guess that there were no splat with CONFIG_DEBUG_PREEMPT ?

I haven't tried that

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines
Quoted 3 lines
tree? Anyway, for this patch:
Reviewed-by Dennis Dalessandro <[email protected]>
Sebastian

Arnaldo Carvalho de Melo

author

2017-10-03 15:49 UTC #19 lore raw

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

In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>

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 <[email protected]>

Cc Dean Luick <[email protected]>

Cc Dennis Dalessandro <[email protected]>

Cc Doug Ledford <[email protected]>

Cc Julia Cartwright <[email protected]>

Cc Kaike Wan <[email protected]>

Cc Leon Romanovsky <[email protected]>

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Cc Sebastian Andrzej Siewior <[email protected]>

Cc Sebastian Sanchez <[email protected]>

Cc Steven Rostedt <[email protected]>

Cc Thomas Gleixner <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 drivers/infiniband/hw/hfi1/chip.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

drivers/infiniband/hw/hfi1/chip.c +7 -3

index 121a4c920f1b..733a00d8ea4c 100644
@@ -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

Signature · 1 line

2.13.6

Sebastian Andrzej Siewior

2017-10-05 16:27 UTC #20 lore raw

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

Quoted 8 lines · Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>
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);

Quoted 7 lines

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

Dennis Dalessandro

2017-10-10 19:06 UTC #21 lore raw

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

In reply to Arnaldo Carvalho de Melo

Quoted 6 lines · Arnaldo Carvalho de Melo

Cc Clark Williams <[email protected]>
Cc Dean Luick <[email protected]>
Cc Dennis Dalessandro <[email protected]>
Cc Doug Ledford <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc Kaike Wan <[email protected]>

Quoted 8 lines

Cc Leon Romanovsky <[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Andrzej Siewior <[email protected]>
Cc Sebastian Sanchez <[email protected]>
Cc Steven Rostedt <[email protected]>
Cc Thomas Gleixner <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

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

Arnaldo Carvalho de Melo

author

2017-10-10 19:15 UTC #22 lore raw

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

Em Tue, Oct 10, 2017 at 03:06:00PM -0400, Dennis Dalessandro escreveu:

Quoted 16 lines · Arnaldo Carvalho de Melo

Quoted 6 lines
Cc Clark Williams <[email protected]>
Cc Dean Luick <[email protected]>
Cc Dennis Dalessandro <[email protected]>
Cc Doug Ledford <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc Kaike Wan <[email protected]>
Quoted 8 lines
Cc Leon Romanovsky <[email protected]>
Cc [email protected]
Cc Peter Zijlstra <[email protected]>
Cc Sebastian Andrzej Siewior <[email protected]>
Cc Sebastian Sanchez <[email protected]>
Cc Steven Rostedt <[email protected]>
Cc Thomas Gleixner <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

Quoted 3 lines

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

Sebastian Andrzej Siewior

2017-10-11 10:44 UTC #23 lore raw

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

Quoted 10 lines · Arnaldo Carvalho de Melo

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

Quoted 1 line

- Arnaldo

Sebastian

Arnaldo Carvalho de Melo

author

2017-10-11 13:42 UTC #24 lore raw

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

Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo

Quoted 6 lines
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:

Quoted 7 lines

Quoted 4 lines
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

Arnaldo Carvalho de Melo

author

2017-10-11 19:07 UTC #25 lore raw

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

In reply to Sebastian Andrzej Siewior

Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:

Quoted 7 lines · Arnaldo Carvalho de Melo

Quoted 6 lines
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:

Quoted 7 lines

Quoted 4 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Arnaldo Carvalho de Melo

author

2017-10-11 19:14 UTC #26 lore raw

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

Em Wed, Oct 11, 2017 at 04:07:45PM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 13 lines

Em Wed, Oct 11, 2017 at 12:44:56PM +0200, Sebastian Andrzej Siewior escreveu:
Quoted 12 lines · Arnaldo Carvalho de Melo
Quoted 10 lines
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.

Quoted 7 lines

Quoted 2 lines
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