Project

General

Profile

Bug #12918

mlxcx "Deadlock: cycle in blocking chain" panic

Added by Paul Winder about 1 month ago. Updated 28 days ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

I have seen a few of these panics. Unfortunately because of the way we run our OS, a full dump was not captured. But this is from the fma report:

nvlist version: 0
        version = 0x0
        class = list.suspect
        uuid = d9b8a207-cfe7-4237-959e-a7f31d76046f
        code = SUNOS-8000-KL
        diag-time = 1584367326 881804
        de = fmd:///module/software-diagnosis
        fault-list-sz = 0x1
        fault-list = (array of embedded nvlists)
        (start fault-list[0])
        nvlist version: 0
                version = 0x0
                class = defect.sunos.kernel.panic
                certainty = 0x64
                asru = sw:///:path=/var/crash/volatile/.d9b8a207-cfe7-4237-959e-a7f31d76046f
                resource = sw:///:path=/var/crash/volatile/.d9b8a207-cfe7-4237-959e-a7f31d76046f
                savecore-succcess = 1
                dump-dir = /var/crash/volatile
                dump-files = vmdump.6
                os-instance-uuid = d9b8a207-cfe7-4237-959e-a7f31d76046f
                panicstr = Deadlock: cycle in blocking chain
                panicstack = genunix:turnstile_block+76b () | unix:mutex_vector_enter+358 () | mlxcx:mlxcx_mac_ring_intr_enable+27 () | mac:mac_hwring_enable_intr+1f () | mac:mac_rx_srs_poll_ring+4af () | unix:thread_start+b () | 
                crashtime = 1584366998
                panic-time = March 16, 2020 at 09:56:38 AM EDT EDT
        (end fault-list[0])

The deadlock was detected in this function:

static int
mlxcx_mac_ring_intr_enable(mac_intr_handle_t intrh)
{
        mlxcx_completion_queue_t *cq = (mlxcx_completion_queue_t *)intrh;
        mlxcx_event_queue_t *eq = cq->mlcq_eq;
        mlxcx_t *mlxp = cq->mlcq_mlx;

        /*
         * We are going to call mlxcx_arm_cq() here, so we take the EQ lock
         * as well as the CQ one to make sure we don't race against
         * mlxcx_intr_n().
         */
        mutex_enter(&eq->mleq_mtx);
        mutex_enter(&cq->mlcq_mtx);
        if (cq->mlcq_state & MLXCX_CQ_POLLING) {
                cq->mlcq_state &= ~MLXCX_CQ_POLLING;
                if (!(cq->mlcq_state & MLXCX_CQ_ARMED))
                        mlxcx_arm_cq(mlxp, cq);
        }
        mutex_exit(&cq->mlcq_mtx);
        mutex_exit(&eq->mleq_mtx);

        return (0);
}

By disassembling the function, I was able to determine the deadlock was detected attempting to acquire eq->mleq_mtx. Since the deadlock is detected acquiring the first lock in the function, it implies the deadlock is not between it and cq->mlcq_mtx.

Looking elsewhere mleq_mtx is only acquired in one other significant piece of code, and that is the interrupt handler mlxcx_intr_n(), and it is held from the start until the end of the interrupt routine. Now, tracing back through the panic stack reveals when the interrupt enable call back is called srs_lock in mac is held, and if we follow the calling sequence in mlxcx_intr_n() we see (with mleq_mtx held) mlxcx_intr_n() -> mac_rx() -> mac_rx_common() -> mac_rx_srs_process(). And mac_rx_srs_process() will acquire srs_lock - so we have the locks being acquired in a different order.

The mleq_mtx is held to maintain consistency with accumulations in a CQ events consumer counter (mlcq_ec) and the armed state of the CQ. The lock should not be held across the call into mac, but I also don't think it is safe to temporarily release and re-acquire it either. The interrupt routine goes to great lengths to avoid unnecessary waits behind mlcq_mtx, since any given interrupt routine may process several CQs and we do not want one CQ waiting for polling to finish blocking other CQs. Which is why the consumer counter and armed status are update outside mlcq_mtx.

I'm proposing an alternate lock purely used to maintain the integrity between mlcq_ec and the armed status. It will be held inside the interrupt routine from the point the armed status is updated, whilst the CQ is processing events until the handler calls into mac. It is also acquired in the polling routines only when re-enabling events for the CQ (ie re-arming it). It is unlikely there will be a collision (and thus contention) with the interrupt handler and polling mode, as once polling mode is active, the interrupt routine will not receive further events. Although there is an execution window which necessitates the use of a lock.

The armed status is maintained in a state variable - mlcq_state. This variable is sometimes modified using atomic ops and sometimes not. The use of atomics is because sometimes it is changed without a common lock being held, but changing with an atomic op under a mutex and then atomically without a mutex does not guarantee its integrity. To guarantee integrity the use of atomic ops needs to be consistent. I have made that change too.

History

#1

Updated by Electric Monk about 1 month ago

  • Gerrit CR set to 768
#2

Updated by Paul Winder about 1 month ago

Based on feedback from the 1st pass of the review, the following additional changes have been made:
  1. The mleq_mtx lock is no longer held continuously in mlxcx_intr_n().
  2. During driver shutdown, interrupts are disabled and shutdown will wait for any active interrupt vectors before continuing.
Testing including:
  1. Using iperf, and verified the driver had significant number of switches between non-polling and polling modes.
  2. Using iperf to send UDP packets to the device, and repeated configuring the interface, down, unloading the driver and re-configuring the device up. Hardware counters from kstat confirmed packets still being received by the h/w even when interface is configured down.
#3

Updated by Electric Monk 28 days ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit 0207f820281e2416190c7ed5f1cb4d11188c082b

commit  0207f820281e2416190c7ed5f1cb4d11188c082b
Author: Paul Winder <paul@winder.uk.net>
Date:   2020-07-10T15:49:19.000Z

    12918 mlxcx "Deadlock: cycle in blocking chain" panic
    Reviewed by: Garrett D'Amore <garrett@damore.org>
    Reviewed by: Andy Stormont <astormont@racktopsystems.com>
    Reviewed by: Robert Mustacchi <rm@fingolfin.org>
    Reviewed by: Alex Wilson <alex@cooperi.net>
    Approved by: Dan McDonald <danmcd@joyent.com>

Also available in: Atom PDF