Make __wt_spin_lock reentrant, and get rid of lock flags

XMLWordPrintableJSON

    • Type: Improvement
    • Resolution: Unresolved
    • Priority: Major - P3
    • None
    • Affects Version/s: None
    • Component/s: None
    • None
    • Storage Engines, Storage Engines - Foundations
    • None
    • None

      WiredTiger uses spinlocks (or mutexes) at times, and for each lock we generally have a session flag (e.g. WT_SESSION_LOCKED_CHECKPOINT ) and a macro to control its use.  The flag is used to ensure that the lock is reentrant.  That is, if my session already has this lock, it can lock it again multiple times, provided that each lock has a matching unlock. But there's another way to make this reentrant.  A spinlock already holds the session_id of the locking session, and the locking code looks roughly like this:

         _lock_implementation_function_(&t->lock);
         t->session_id = session->id;
      

      If we add one more field to the spinlock, lock_count, then we can make the lock reentrant like this:

        if (t->session_id != session->id) {
           _lock_implementation_function_(&t->lock);
           t->session_id = session->id; 
        }
        t->lock_count++;

      The corresponding unlock function would be like:

        assert(t->session_id == session->id);
        assert(t->lock_count > 0);
        if (--t->lock_count == 0) {
           t->session_id = WT_SESSION_ID_INVALID;
           _unlock_implementation_function(&t->lock);
        }

      This technique is already used to lock dhandles exclusively in a reentrant way (although the session and lock_count fields are kept in the dhandle).

      Benefits:

      No more lock flags.

      Most of the macros in schema.h can either be removed or stripped down to a few easy to understand lines. Some of these macros are big, and some have special handling for read/write locks.  I'd advocate putting the smarts into the readlock and writelock functions rather than the macros.

      It's convenient to still have a macro to take a lock, do an operation and then unlock - but it could probably be a single generic macro that can be used for every lock - taking the lock field as a parameter.

      Note that it's still easy to check "does this session already hold this lock?" by comparing, say, conn->checkpoint_lock->session_id == session->id.  When necessary we could have a specific macro to do a certain kind of lock with some extra checks - like "when locking X, I must also have Y locked".  But this too could be made easy with a additional field on a lock (a pointer to another lock that is a dependency) and handled within a locking function, rather than a macro.

      When we need a new capability for an existing lock, like adding a "trylock" capability, we don't have to create new macro(s), generally just call a different function or existing generic macro.

      Better visibility on system lockups and contention. We can functionally look at all the locks and print the session names for all holders.  We can also more easily ask questions like - what thread holds the X lock the longest, or if X lock is held, what is the holding thread doing? 

            Assignee:
            [DO NOT USE] Backlog - Storage Engines Team
            Reporter:
            Donald Anderson
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: