lkml.org 
[lkml]   [2008]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch] x86: some lock annotations for user copy paths
    On Wed, Sep 10, 2008 at 05:01:04PM +0200, Peter Zijlstra wrote:
    > On Wed, 2008-09-10 at 16:48 +0200, Nick Piggin wrote:
    >
    > > @@ -3016,3 +3016,18 @@ void print_vma_addr(char *prefix, unsign
    > > }
    > > up_read(&current->mm->mmap_sem);
    > > }
    > > +
    > > +void might_fault(void)
    > > +{
    > > + /*
    > > + * it would be nicer only to annotatea paths which are not under
    > > + * pagefault_disable, however that requires a larger audit and
    > > + * providing helpers like get_user_atomic.
    > > + */
    > > + if (!in_atomic()) {
    > > + might_sleep();
    > > + if (current->mm)
    > > + might_lock_read(&current->mm->mmap_sem);
    > > + }
    > > +}
    > > +EXPORT_SYMBOL(might_fault);
    >
    > >From the nitpick squad :-), I prefer the form:
    >
    > void might_fault(void)
    > {
    > if (in_atomic())
    > return;
    >
    > might_sleep();
    >
    > if (!current->mm)
    > might_lock_read(&current->mm->mmap_sem);
    > }
    >
    >
    > Due to it being one nesting level less.

    Well... yeah. I find it doesn't matter unless the function is complex or
    nesting level high in absolute terms ;)


    > > Index: linux-2.6/include/linux/kernel.h
    > > ===================================================================
    > > --- linux-2.6.orig/include/linux/kernel.h
    > > +++ linux-2.6/include/linux/kernel.h
    > > @@ -140,6 +140,15 @@ extern int _cond_resched(void);
    > > (__x < 0) ? -__x : __x; \
    > > })
    > >
    > > +#ifdef CONFIG_LOCKDEP
    > > +void might_fault(void);
    > > +#else
    > > +static inline void might_fault(void)
    > > +{
    > > + might_sleep();
    > > +}
    > > +#endif
    > > +
    > > extern struct atomic_notifier_head panic_notifier_list;
    > > extern long (*panic_blink)(long time);
    > > NORET_TYPE void panic(const char * fmt, ...)
    >
    > This forgets that in_atomic() again - possibly triggering might_sleep()
    > where not appropriate.

    OK, drat... maybe we'll just noop that path and lose a couple of
    might_sleep() points -- testing and developing should be done mostly
    under lockdep anyway.


    > I'm not sure its worth it to out-of-line the thing though (its only big
    > on debug builds), and CONFIG_LOCKDEP is the wrong CONFIG_* variable, I
    > think CONFIG_PROVE_LOCKING would be the appropriate one.

    It is so that we don't need to pull sched.h and lockdep.h into those low
    level user access functions.


    \
     
     \ /
      Last update: 2008-09-10 17:21    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean