How to do proper locking

[Hans Petter Selasky]

On Thursday 04 August 2005 14:08, Max Laier wrote:
> On Thursday 04 August 2005 13:40, Hans Petter Selasky wrote:
> > This is a copy and paste from the kernel sources:
> >
> > struct ucred *
> > crhold(struct ucred *cr)
> > {
> > The problem is, what happens if the kernel switches thread right here,
> > and then the other thread calls "crfree()" on this structure, that will
> > "free()" memory pointed to by "cr". Then the first line of code below
> > will access freed memory, and then we are going for a segment fault or
> > worse.
> >
> >         mtx_lock(cr->cr_mtxp);
> >         cr->cr_ref++;
> >         mtx_unlock(cr->cr_mtxp);
> >         return (cr);
> > }
>
> It seems that you are misunderstanding the concept of reference counting. 
> The idea is to have a reference as long as you are handing around the
> object.  In order to call crhold() you must already hold a reference to the
> credential. This will prevent the credential structure to vanish and allow
> you to pass the credential to another consumer that now can assume to hold
> a reference of it's own.

Maybe "crhold()" is not exactly what I am after. From what I can see, we are 
speaking about two different refcount systems:

- crhold()/crfree() is incrementing/decrementing the refcount to indicate the 
total number of references hold.

- I am only incrementing the refcount at object destruction, to make sure that 
the last refcount value cannot be used again, to get access to the object.

>
> If you can access your objects from a external list, you have to hold a
> global lock that protects:
>  1) Sanity of the list
>  2) Free operations to the member objects

Yes, that is what I am doing, but there is a problem. Here is another 
pseudo-code example on what I'm trying to get a solution for:

struct mtx global;

struct my_object {
  struct mtx *mtx;
  u_int8_t some_data[256];
  struct my_object *next;
} *m;

/* Here is the enforced locking order, which
 * makes most sense to me:
 *
 * 1. m->mtx
 * 2. global
 */

void()
thread_1()
{
  struct my_object *m;

  sleep(&global);

  lock(global);
  /*
   * to access this object
   * we dequeue it from a
   * one-way linked list:
   */
  m = dequeue_object();
  mtx = m->mtx;
  unlock(global);

  wakeup(m);
  sleep(m);

  /* What makes this so special is that
   * the object pointed to by "m" is owned
   * by "thread_2". When that thread
   * exits the object is gone.
   *
   * A big problem is that one cannot
   * hold any lock right here, to
   * block "thread_2" from freeing the 
   * object, due to the enforced locking 
   * order. Switching locking order
   * does not change anything.
   */
  lock(mtx);

  /* When the CPU gets here a check
   * is neccesary to ensure that the
   * other thread has not freed the
   * object pointed to by "m".
   */
  if(refcount_copy == some_atomic_read)
  {
    /* object is still here */
  }

  unlock(mtx);

  return;
}

void
thread_2()
{
  struct my_object *m;

  m = malloc(sizeof(*m));

  if(m == NULL) return;

  m->mtx = mtx_alloc();

  lock(global);
  enqueue_object(m);
  unlock(global);

  wakeup(&global);
  sleep(m);

  lock(global);
  remove_object_from_list_if_present(m);
  unlock(global);

  lock(mtx);

  /*
   * XXX increase some refcount atomically
   * XXX even if we are locked
   */
  unlock(mtx);

  free(m);

  wakeup(m);
  return;
}

>
> If you access the objects via the list/tree/whatsoever "most of the time" -
> refcounting isn't for you.
>

If you use a single lock, you don't need refcounting. But if you use two 
locks, like in this example, and you have more than one source of object 
destruction, refcounting is required.

Do you see that more than one thread can be about to destroy the object at the 
same time? 

--HPS