Re: [PATCH 4/6] Do not call _xend if no transaction is active.

[Dominik Vogt <vogt at linux dot vnet dot ibm dot com>]

On Mon, Sep 02, 2013 at 02:01:23PM -0700, Andi Kleen wrote:
> Dominik Vogt <vogt@linux.vnet.ibm.com> writes:
> 
> > If pthread_mutex_unlock is called without the futex locked and no transaction
> > open does not necessarily mean that pthread_mutex_lock has not been called.
> > The transaction may have been closed in third party code.  While this _may_ be
> > a bug in the software as a whole, unlock should not crash the program.
> >
> > Overall it is problematic to close transactions if you are not completely sure
> > that you have opened the transaction yourself.  I see no general solution for
> > this except installing coding rules for transactional code.
> 
> The XTEST is currently 4 cycles.

I understand that.

> Also unlocking free locks is undefined.

This patch is _not_ about what happens when the user unlocks an
unlocked lock.  It is about what happens when the user unlocks a
mutex that has been elided, but when at the time of unlock() no
transaction is open.  This can happen through third party code.

One could argue that this patch is required to keep Posix
semantics intact.  One could also argue that in aforementioned
case the program has a bug.

Consider this

Example: A program uses glibc with lock elision and a library
"foo".  Foo provides the functions foo_lock() and foo_unlock() and
also uses transactions.  In foo it is perfectly valid to call
foo_unlock() without foo_lock().  Let foo_unlock() be implemented
like this (let's ignore any race conditions for simplicity):

  foo_unlock(foo_lock *x)
  {
      if (x->lock_count > 0)
          if (_xtest() != 0)
              _xend();
      else
          x->lock_count--;
  }

If the program looks like this:

    foo_lock(x);
    phtread_mutex_lock(m);
    foo_unlock(x);
    phtread_mutex_unlock(m);

If all instructions are executed in a row, this is correct code,
regardless of whether elision is available in foo, in glibc or
both or not at all.  Now consider this:

  if (some_condition) goto side_entrance
    foo_lock(x);
  side_entrance:
    phtread_mutex_lock(m);
    foo_unlock(x);
    phtread_mutex_unlock(m);

Under some_condition, the initial foo_lock(x) is skipped.  Now
mutex_lock() starts a transaction, foo_unlock detects an unlocked
lock and assumes it is elided and calls _xend() and thus commits
the transaction.  The final mutex_unlock() would try to xend the
assumed transaction and crash.

Note that this program is correct without elision, or with elision
just in glibc, or with elision just in foo.  But if both, glibc
and foo support elision, the program would crash.  The crash is
caused by wrong assumptions being made in foo _and_ in glibc in
the unlock functions:

 * Glibc assumes that if the lock is free, either a transaction is
   open _or_ unlock has been called without lock.
 * Foo assumes that if its lock is free and a transaction is
   open, the transaction can be committed.

> The plan was to wait for user feedback if that is really needed because
> of the performance overhead.

That should not keep us from identifying corner cases that may
violate specifications.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany