[asterisk-dev] Poor locking model in Asterisk (I know you can fix it)

[Hans Petter Selasky]

Hi,

I am a bit lazy, so I just post this here, leaving it for someone else to make 
a PR on this issue, hence mantis was a little complicated for me, and I don't 
have so much time at the moment.


Number 1)

You should define the locking rules! Otherwise very nasty things may and will 
happen. 

Let me explain. A mutex is something that prevents 
two processes from colliding. Below is what I currently use for 
some channel driver. It does not cover all of Asterisk. The big problem is 
that there are two locks protecting a single channel. My private "call 
descriptor" lock, cd->p_app->lock, and the "Asterisk channel" lock, 
cd->pbx_chan->lock. This leads nowhere else than to trouble. I hope you can 
fix this, so that it is possible to pass a lock to Asterisk, which it should 
lock before calling any callbacks in my channel driver.

/*
 * LOCKING RULES EXAMPLE
 * =====================
 *
 * This channel driver uses several locks. One must be 
 * careful not to reverse the locking order, which will
 * lead to a deadlock. Here is the locking order
 * that must be followed:
 *
 * struct call_desc *cd;
 * struct cc_capi_application *p_app;
 *
 * 1. cc_mutex_lock(&do_periodic_lock);
 *
 * 2. cc_mutex_lock(&cd->pbx_chan->lock); **
 *
 * 3. cc_mutex_lock(&p_app->lock); ***
 *
 * 4. cc_mutex_lock(&capi_global_lock);
 *
 *
 *  ** the PBX will call the callback functions with 
 *     this lock locked. This lock protects the 
 *     structure pointed to by 'cd->pbx_chan'. Also note
 *     that calling some PBX functions will lock
 *     this lock!
 *
 *  *** in case multiple CAPI applications should be locked 
 *      in series, the CAPI application with the lowest 
 *      memory address should be locked first.
 *      (One maybe want to check/use the address of the mutex.)
 */

Number 2)

To get my private "PVT" pointer, I have to do an extensive search, just 
because two different locks are used:

/*---------------------------------------------------------------------------*
 *      cd_by_pbx_chan - find "call descriptor" by PBX channel
 *
 * NOTE: returns a locked call descriptor
 *---------------------------------------------------------------------------*/
static struct call_desc *
cd_by_pbx_chan(struct ast_channel *pbx_chan)
{
#if 0

    /* in the future the PBX should lock our
     * private lock before calling any callbacks !
     */
    struct call_desc *cd = CC_CHANNEL_PVT(pbx_chan);

    cc_mutex_assert(&cd->p_app->lock, MA_OWNED);

#else
    struct call_desc *cd = NULL;
    struct cc_capi_application *p_app;
    u_int16_t n;

    for(n = 0; n < CAPI_MAX_CONTROLLERS; n++) 
    {
        p_app = capi_application[n];

        if (p_app) {

          cc_mutex_lock(&p_app->lock);

          cd = p_app->cd_root_ptr;

          while(cd) {

            if ((cd->pbx_chan == pbx_chan) &&
                (!CD_IS_UNUSED(cd)))
            {
                goto found;
            }
            cd = cd->next;
          }

          cc_mutex_unlock(&p_app->lock);
        }
    }
 found:

    if (cd == NULL) {
        cc_log(LOG_ERROR, "PBX channel has no interface!\n");
    }
#endif

    return cd;
}


I would prefer if Asterisk would wrap all channel driver callbacks like this:

/*---------------------------------------------------------------------------*
 *      chan_capi_write - called from "ast_write()"
 *---------------------------------------------------------------------------*/
static int
chan_capi_write(struct ast_channel *pbx_chan, struct ast_frame *p_frame)
{
    struct call_desc *cd = cd_by_pbx_chan(pbx_chan);
    int error = 0;

    if (cd == NULL) {
        return -1;
    }

    cd_verbose(cd, 3, 1, 2, "\n");

    /* call descriptor is still valid */

    error = __chan_capi_write(cd, p_frame);

    cd_unlock(cd);

    return error;
}

Number 3)

All channel driver locking functions, should take two arguments, to easily 
handle locking of two different channels at the same time.

/*---------------------------------------------------------------------------*
 *      chan_capi_bridge - called from "ast_channel_bridge()"
 *---------------------------------------------------------------------------*/
static CC_BRIDGE_RETURN
chan_capi_bridge(struct ast_channel *pbx_chan0,
                 struct ast_channel *pbx_chan1,
                 int flags, 
                 struct ast_frame **fo,
                 struct ast_channel **rc
#ifdef CC_AST_BRIDGE_WITH_TIMEOUTMS
                 , int timeoutms
#endif
                 )
{
    struct call_desc *cd0;
    struct call_desc *cd1;
    int error = AST_BRIDGE_COMPLETE;

    if(cd_mutex_lock_double_pbx_chan(pbx_chan0,pbx_chan1,&cd0,&cd1)) {
        return AST_BRIDGE_FAILED_NOWARN;
    }

    cd_verbose(cd0, 3, 0, 2, "\n");
    cd_verbose(cd1, 3, 0, 2, "\n");

    /* call descriptors are still valid */
    error = __chan_capi_bridge(cd0,cd1,flags,fo,rc
#ifdef CC_AST_BRIDGE_WITH_TIMEOUTMS
                               , timeoutms
#endif
                               );
    cd_mutex_unlock_double(cd0, cd1);
    return error;
}

#define cd_lock(cd) cc_mutex_lock(&(cd)->p_app->lock)
#define cd_unlock(cd) cc_mutex_unlock(&(cd)->p_app->lock)

/*---------------------------------------------------------------------------*
 *      cd_mutex_unlock_double - unlock two call descriptors
 *---------------------------------------------------------------------------*/
static void
cd_mutex_unlock_double(struct call_desc *cd0,
                       struct call_desc *cd1)
{
    if (cd0 == cd1) {

        cc_mutex_unlock(&cd0->p_app->lock);

    } else if (cd0->p_app < cd1->p_app) {

        cc_mutex_unlock(&cd0->p_app->lock);
        cc_mutex_unlock(&cd1->p_app->lock);

    } else {

        cc_mutex_unlock(&cd1->p_app->lock);
        cc_mutex_unlock(&cd0->p_app->lock);

    }
    return;
}

/*---------------------------------------------------------------------------*
 *      cd_mutex_lock_double - lock two call descriptors
 *---------------------------------------------------------------------------*/
static void
cd_mutex_lock_double(struct call_desc *cd0,
                     struct call_desc *cd1)
{
    if (cd0 == cd1) {

        cc_mutex_lock(&cd0->p_app->lock);

    } else if (cd0->p_app < cd1->p_app) {

        cc_mutex_lock(&cd0->p_app->lock);
        cc_mutex_lock(&cd1->p_app->lock);

    } else {

        cc_mutex_lock(&cd1->p_app->lock);
        cc_mutex_lock(&cd0->p_app->lock);

    }
    return;
}

/*---------------------------------------------------------------------------*
 *      cd_mutex_lock_double_pbx_chan - lock two PBX channels
 *---------------------------------------------------------------------------*/
static u_int8_t
cd_mutex_lock_double_pbx_chan(struct ast_channel *pbx_chan0,
                              struct ast_channel *pbx_chan1,
                              struct call_desc **pp_cd0,
                              struct call_desc **pp_cd1)
{
    struct call_desc *cd0 = CC_CHANNEL_PVT(pbx_chan0);
    struct call_desc *cd1 = CC_CHANNEL_PVT(pbx_chan1);

#if 0
    cc_mutex_assert(&pbx_chan0->lock, MA_OWNED);
    cc_mutex_assert(&pbx_chan1->lock, MA_OWNED);
#endif

    if (cd0 == cd1)
    {
        cd1 = cd0 = cd_by_pbx_chan(pbx_chan0);
    }
    else if (cd0->p_app < cd1->p_app)
    {
        cd0 = cd_by_pbx_chan(pbx_chan0);
        cd1 = cd_by_pbx_chan(pbx_chan1);
    }
    else
    {
        cd1 = cd_by_pbx_chan(pbx_chan1);
        cd0 = cd_by_pbx_chan(pbx_chan0);
    }

    if (cd1 && cd0) 
    {
        /* success */
        *pp_cd0 = cd0;
        *pp_cd1 = cd1;
        return 0;
    }

    /* failure */

    if (cd0) 
    {
        cd_unlock(cd0);
    }

    if (cd1)
    {
        cd_unlock(cd1);
    }

    /* no channel interface */
    cc_log(LOG_ERROR, "PBX channel has no interface!\n");
    return 1;
}

That was it. Hope you can do it like this. Then it will fit my "channel 
driver" very well, and it will solve some problems, and remove extra code.
I am also very sure that it will make Asterisk more stable.

Please feel free to send me your comments :-)

--HPS