Infiniband hfi1 + PREEMPT_RT_FULL issues

I'm trying to get an Infiniband test case working with the RT
kernel, and ended over tripping over this case:

In drivers/infiniband/hw/hfi1/pio.c 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

Hi,

I'm trying to get an Infiniband test case working with the RT
kernel, and ended over tripping over this case:

In drivers/infiniband/hw/hfi1/pio.c 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

I'm wondering if turning this sc->alloc_lock to a raw_spin_lock is the right solution, which I'm afraid its not, as there are places where it is held and then the code goes on to grab other non-raw spinlocks...

I got this patch in my test branch and it makes the test case go further before splatting on other problems with infiniband + PREEMPT_RT_FULL, but as I said, I fear its not the right solution, ideas?

The kernel I'm seing this is RHEL's + the PREEMPT_RT_FULL patch:

Linux version 3.10.0-709.rt56.636.test.el7.x86_64 (acme@seventh) (gcc version 4.8.5 20150623 (Red Hat 4.8.5-16) (GCC) ) # 1 SMP PREEMPT RT Wed Sep 20 18:04:55 -03 2017

I will try and build with the latest PREEMPT_RT_FULL patch, but the infiniband codebase in RHEL seems to be up to what is upstream and I just looked at patches-4.11.12-rt14/add_migrate_disable.patch and that WARN_ON_ONCE(p->migrate_disable_atomic) is still there :-\

- Arnaldo

Quoted 3 lines

I'm wondering if turning this sc->alloc_lock to a raw_spin_lock is the right solution, which I'm afraid its not, as there are places where it is held and then the code goes on to grab other non-raw spinlocks...

No, the correct solution is to convert the preempt_disable into a local_lock(), which will be a preempt_disable when PREEMPT_RT is not set. Look for other patches that convert preempt_disable() into local_lock()s for examples.

Quoted 4 lines

No, the correct solution is to convert the preempt_disable into a local_lock(), which will be a preempt_disable when PREEMPT_RT is not set. Look for other patches that convert preempt_disable() into local_lock()s for examples.

Thanks, I had seen a patch patch by Sebastian for random.c doing that, will continue in that direction.

Quoted 8 lines

Quoted 4 lines

No, the correct solution is to convert the preempt_disable into a local_lock(), which will be a preempt_disable when PREEMPT_RT is not set. Look for other patches that convert preempt_disable() into local_lock()s for examples.

Thanks, I had seen a patch patch by Sebastian for random.c doing that, will continue in that direction.

I'm assuming you want to end up with common driver code base?

Will there eventually be a wrapper upstream to allow that?

Or will the rt patch handle those issues?

So what happens is that infrastructure in the rt patch eventually lands upstream, then this difference ceases to exist.

Steven, are there plans for local locks to go upstream?

Hi,

I'm trying to get an Infiniband test case working with the RT
kernel, and ended over tripping over this case:

In drivers/infiniband/hw/hfi1/pio.c sc_buffer_alloc() disables
preemption that will be reenabled by either pio_copy() or seg_pio_copy_end().

Quoted 8 lines

Hi,

I'm trying to get an Infiniband test case working with the RT
kernel, and ended over tripping over this case:

In drivers/infiniband/hw/hfi1/pio.c sc_buffer_alloc() disables
preemption that will be reenabled by either pio_copy() or seg_pio_copy_end().

Perhaps it's worth revisiting why this design decision was chosen in the first place? The introducing commit, a054374f15428cbc1 ("staging/rdma/hfi1: convert buffers allocated atomic to per cpu") mentions the previous atomic as being expensive, but the usage of preempt_disable()/preempt_enable() is also expensive, albeit in a different way.

My first question would be: is disabling preemption here really necessary?

AFAICT, preemption is only disabled to protect the access of the 'buffers_allocated' percpu counters, nothing else.

However, because these counters are only observed in aggregate, across CPUs (see get_buffers_allocated()), the sum will be the same, regardless of a thread is migrated between a this_cpu_inc(buffers_allocated) and a this_cpu_dec(buffers_allocated).

If that's the case, perhaps the fix is as easy as removing the preempt_disable() and preempt_enable()?

Quoted 3 lines

My first question would be: is disabling preemption here really necessary?

The motivation for the preempt manipulation has nothing to do with the counter.

The buffer allocation returns a set of ringed write-combining 64B
MMIO buffers. The allocate lock is then dropped right after
allocation to allow parallel allocates.

The hardware keeps track of the buffer fill across multiple CPUs, so that after the oldest buffer is copied the ring is advanced.

The idea was to minimize the time from the drop of the allocate lock to the end of the copy to insure the highest rate of copy to the ring from multiple cores.

Quoted 8 lines

AFAICT, preemption is only disabled to protect the access of the 'buffers_allocated' percpu counters, nothing else.

However, because these counters are only observed in aggregate, across CPUs (see get_buffers_allocated()), the sum will be the same, regardless of a thread is migrated between a this_cpu_inc(buffers_allocated) and a this_cpu_dec(buffers_allocated).

The counter exists purely as a hot path optimization to avoid an atomic.

The only time an accurate counter is required is during reset sequences to wait for allocate/copy pairs to finish and we don't care if the closing decrement is on the same core.

The percpu refcount might be a better choice, but I don't think it existed at the time the code was written.

Quoted 3 lines

If that's the case, perhaps the fix is as easy as removing the preempt_disable() and preempt_enable()?

That would certainly be the easiest solution, but would compromise performance if a migration occurs after the allocate lock is dropped.

We have been looking at the patch that was submitted for using the raw spin lock and we share Arnaldo's concern, particularly mixing lock types.

Other locks can be procured in the timeout case in sc_wait_for_packet_egress() inside queue_work() and dd_dev_err()/printk().

Quoted 16 lines

Quoted 8 lines

Hi,

I'm trying to get an Infiniband test case working with the RT
kernel, and ended over tripping over this case:

In drivers/infiniband/hw/hfi1/pio.c sc_buffer_alloc() disables
preemption that will be reenabled by either pio_copy() or seg_pio_copy_end().

Perhaps it's worth revisiting why this design decision was chosen in the first place? The introducing commit, a054374f15428cbc1 ("staging/rdma/hfi1: convert buffers allocated atomic to per cpu") mentions the previous atomic as being expensive, but the usage of preempt_disable()/preempt_enable() is also expensive, albeit in a different way.

Quoted 3 lines

My first question would be: is disabling preemption here really necessary?

The motivation for the preempt manipulation has nothing to do with the counter.

The buffer allocation returns a set of ringed write-combining 64B MMIO
buffers. The allocate lock is then dropped right after allocation to
allow parallel allocates.

The hardware keeps track of the buffer fill across multiple CPUs, so that after the oldest buffer is copied the ring is advanced.

The idea was to minimize the time from the drop of the allocate lock to the end of the copy to insure the highest rate of copy to the ring from multiple cores.

Quoted 3 lines

If that's the case, perhaps the fix is as easy as removing the preempt_disable() and preempt_enable()?

That would certainly be the easiest solution, but would compromise performance if a migration occurs after the allocate lock is dropped.

Quoted 3 lines

My first question would be: is disabling preemption here really necessary?

The motivation for the preempt manipulation has nothing to do with the counter.

The buffer allocation returns a set of ringed write-combining 64B MMIO buffers. The allocate lock is then dropped right after allocation to allow parallel allocates.

The hardware keeps track of the buffer fill across multiple CPUs, so that after the oldest buffer is copied the ring is advanced.

The idea was to minimize the time from the drop of the allocate lock to the end of the copy to insure the highest rate of copy to the ring from multiple cores.