lkml.org 
[lkml]   [2017]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 06/13] lockdep: Implement crossrelease feature
    On Wed, Jan 18, 2017 at 10:17:32PM +0900, Byungchul Park wrote:

    > +struct cross_lock {
    > + /*
    > + * When more than one acquisition of crosslocks are overlapped,
    > + * we do actual commit only when ref == 0.
    > + */
    > + atomic_t ref;

    That comment doesn't seem right, should that be: ref != 0 ?

    Also; would it not be much clearer to call this: nr_blocked, or waiters
    or something along those lines, because that is what it appears to be.

    > + /*
    > + * Seperate hlock instance. This will be used at commit step.
    > + *
    > + * TODO: Use a smaller data structure containing only necessary
    > + * data. However, we should make lockdep code able to handle the
    > + * smaller one first.
    > + */
    > + struct held_lock hlock;
    > +};

    > +static int add_xlock(struct held_lock *hlock)
    > +{
    > + struct cross_lock *xlock;
    > + unsigned int gen_id;
    > +
    > + if (!depend_after(hlock))
    > + return 1;
    > +
    > + if (!graph_lock())
    > + return 0;
    > +
    > + xlock = &((struct lockdep_map_cross *)hlock->instance)->xlock;
    > +
    > + /*
    > + * When acquisitions for a xlock are overlapped, we use
    > + * a reference counter to handle it.

    Handle what!? That comment is near empty.

    > + */
    > + if (atomic_inc_return(&xlock->ref) > 1)
    > + goto unlock;

    So you set the xlock's generation only once, to the oldest blocking-on
    relation, which makes sense, you want to be able to related to all
    historical locks since.

    > +
    > + gen_id = (unsigned int)atomic_inc_return(&cross_gen_id);
    > + xlock->hlock = *hlock;
    > + xlock->hlock.gen_id = gen_id;
    > +unlock:
    > + graph_unlock();
    > + return 1;
    > +}

    > +void lock_commit_crosslock(struct lockdep_map *lock)
    > +{
    > + struct cross_lock *xlock;
    > + unsigned long flags;
    > +
    > + if (!current->xhlocks)
    > + return;
    > +
    > + if (unlikely(current->lockdep_recursion))
    > + return;
    > +
    > + raw_local_irq_save(flags);
    > + check_flags(flags);
    > + current->lockdep_recursion = 1;
    > +
    > + if (unlikely(!debug_locks))
    > + return;
    > +
    > + if (!graph_lock())
    > + return;
    > +
    > + xlock = &((struct lockdep_map_cross *)lock)->xlock;
    > + if (atomic_read(&xlock->ref) > 0 && !commit_xhlocks(xlock))

    You terminate with graph_lock() held.

    Also, I think you can do the atomic_read() outside of graph lock, to
    avoid taking graph_lock when its 0.

    > + return;
    > +
    > + graph_unlock();
    > + current->lockdep_recursion = 0;
    > + raw_local_irq_restore(flags);
    > +}
    > +EXPORT_SYMBOL_GPL(lock_commit_crosslock);
    > +
    > +/*
    > + * return 0: Need to do normal release operation.
    > + * return 1: Done. No more release ops is needed.
    > + */
    > +static int lock_release_crosslock(struct lockdep_map *lock)
    > +{
    > + if (cross_lock(lock)) {
    > + atomic_dec(&((struct lockdep_map_cross *)lock)->xlock.ref);
    > + return 1;
    > + }
    > + return 0;
    > +}
    > +
    > +static void cross_init(struct lockdep_map *lock, int cross)
    > +{
    > + if (cross)
    > + atomic_set(&((struct lockdep_map_cross *)lock)->xlock.ref, 0);
    > +
    > + lock->cross = cross;
    > +}

    \
     
     \ /
      Last update: 2017-02-28 16:51    [W:4.532 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site