Re: How to do proper locking

From: Hans Petter Selasky (hselasky_at_c2i.net)
Date: 08/04/05

  • Next message: John Baldwin: "Re: How to do proper locking" 
    To: John Baldwin <jhb@freebsd.org>
Date: Thu, 4 Aug 2005 18:50:12 +0200

    On Thursday 04 August 2005 15:53, John Baldwin wrote:
    > On Thursday 04 August 2005 07:40 am, Hans Petter Selasky wrote:
    > > On Wednesday 03 August 2005 19:21, John Baldwin wrote:
    > > > On Tuesday 02 August 2005 06:23 pm, Hans Petter Selasky wrote:
    > > > > Hi,
    > > > >
    > > > > I am looking for a safe way to access structures that can be freed.
    > > > > The solution I am looking for must not:
    > > > >
    > > > > - hinder scaleability
    > > > > - lead to use of a single lock
    > > > > - lead to lock order reversal
    > >
    > > There is one more thing. When the structure is freed it should not block
    > > waiting for any reference counts.
    > >

    >
    > crhold() is always called when the caller knows that it has a reference to
    > cr that can't go away. Thus, in the case of p_ucred, one holds the
    > associated PROC_LOCK() while doing crhold(). After the crhold() you can
    > drop the proc lock, so that lock is only held for a very short period of
    > time. You then have your own reference to the cred structure that is valid
    > until you call crfree(). In the case of curthread->td_ucred, the reference
    > is valid until curthread releases it on a cred update at the start of a
    > syscall, so your reference is valid without needing locks for the life of a
    > syscall. The key here is that you solve this problem at a higher level,
    > not within the structure itself.

    Yes, this works with mbufs, crhold() and alike, where one doesn't actually
    free the object until the refcount reaches zero. But I am speaking about
    destroying an object while the refcount is non-zero. Do you see the
    difference?

    There are two ways to handle this:

    1) blocking: wait until the refcount reaches zero.

    2) nonblocking: increment some other refcount that the
       callback checks before accessing any data.

    Do you agree that method 2 is going to:

    - avoid deadlock
    - avoid use of sx-locks, locks that allow calls to msleep(),
      to keep synchronization while destroying objects
      with refcounts.

    When calling functions like "callout_stop()", it is likely that one is holding
    a lock to prevent other code from calling "callout_start()" on the same
    callback before "callout_stop()" has been called. Now, if "callout_stop()"
    sleeps, the lock that is held while calling this function, must allow calls
    msleep(). So this must be an sx-lock (see "man sx"). Now if one routine
    sleeps, then any callers of this routine must sleep too. So this affects the
    synchronization of the whole system.

    Isn't nonblocking operation preferred over blocking operation?

    >
    > > > What actual problem are you trying to solve?
    > >
    > > Generic callbacks:
    > >
    > > - setting up timers. Make sure that callback is never called after that
    > > timer has been stopped.
    >
    > See callout_stop() and callout_drain(). callout_drain() won't return until
    > the callout has both been stopped and has finished executing if it was
    > already in progress.

    The callback can be called after that "callout_stop()" has been called!

    You can use _callout_stop_safe(), but the problem is that it will sleep,
    waiting for "the other thread" to finish. The solution I have described,
    makes this process non-blocking, which means that one can hold a lock while
    calling callout_stop() which must be an advantage.

    >
    > > - setting up devices. Make sure that read/write/ioctl/poll callbacks are
    > > never called after that the device has been freed.
    >
    > This is managed with refcounts on the dev_t object by devfs. Once you call
    > destroy_dev() devfs manages the rest.

    No, the callback functions read/write/ioctl/poll can be called after that
    "destroy_dev()" has been called.

    >
    > > - setting up interrupts. Make sure that interrupt routine is never called
    > > after that it is been teared down.
    >
    > bus_teardown_intr() already does this.

    I'm sure it is the same here. If the interrupt handler doesn't have its own
    lock then, I think the interrupt callback can be called after that
    "bus_teardown_intr()" has been called.

    >
    > > And more. There are lots of situations in the kernel that run into the
    > > same scheme, without a proper solution.
    >
    > Are there any other places you can think of that don't handle this already?

    I am thinking about all the device drivers that create devices in /dev/ that
    can be detache, and that really never checks anything before accessing the
    softc:
    int
    ulptread(struct cdev *dev, struct uio *uio, int flags)
    {
            struct ulpt_softc *sc;
            int error;

            USB_GET_SC(ulpt, ULPTUNIT(dev), sc);

    XXX
    XXX here can be an segmentation fault where sc == NULL
    XXX

            if (sc->sc_dying)
                    return (EIO);

            sc->sc_refcnt++;
            error = ulpt_do_read(sc, uio, flags);
            if (--sc->sc_refcnt < 0)
                    usb_detach_wakeup(USBDEV(sc->sc_dev));
            return (error);
    }

    > The approach FreeBSD's kernel has taken is to solve this problem by
    > duplicating a lot of checks all over the place for each data structure, but
    > to solve it in the primitives themselves instead (hence callout_drain(),
    > bus_teardown_intr(), destroy_dev(), etc.)

    Yes, but why are the implementations based on blocking operation, hence this
    is not required?

    What I want is that the kernel provides some routine that can do locking based
    on a structure like below. Also the kernel must provide some routines to
    allocate, free and read a refcount.

    struct callback_args {
      void *sc;
      struct mtx *p_mtx;
      u_int32_t refcount_copy;
      u_int32_t *p_refcount;
    };

    Then I want routines like "callout_reset()", "make_dev()", "bus_setup_intr()"
    and so on, to pass these four parameters to the callback function, either
    directly, or like a pointer to this structure.

    --HPS
    _______________________________________________
    freebsd-hackers@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
    To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"

  • Next message: John Baldwin: "Re: How to do proper locking"

    Relevant Pages

    • Re: How to do proper locking
      ... >> refcount reads and no refcount writes. ... Yes, you are right, but the problem is, that for most callback systems in the ... worries about having to exit a lock before returning. ... call any function that can sleep, without worrying about exiting any locks. ...
      (freebsd-hackers)
    • Re: How to do proper locking
      ... >> actually free the object until the refcount reaches zero. ... >> callback function, either directly, or like a pointer to this structure. ... file-descriptor of the current thread while holding some global lock. ... "dev" should get a copy of "args" while holding a global lock. ...
      (freebsd-hackers)
    • Re: Lockless uidinfo.
      ... contention in the common case. ... uihold- refcount goes to 1 ... on the uid while it drops the uidinfo lock to acquire the table lock. ...
      (freebsd-arch)
    • Re: [PATCH 2/4] send callback when swap slot is freed
      ...     if ) ... place where we'd like to offer a callback at all. ... it's a special for ramzswap or compcache, ... If we really have to do this within the lock then there cannot be ...
      (Linux-Kernel)
    • Re: Resolving race conditions with callbacks and cancellation?
      ... Well, the callback should acquire the SM lock, set the internal state to 'timedOut' and then release the lock. ... This does introduce some possibilities that have to be catered for - the originating thread may, after the timeout notification, immediately issue another read call while the timeout for the preceeding call is still 'live', ie has a 'cancelTimeout' request pending but has not yet acted on it. ...
      (comp.programming.threads)