lkml.org 
[lkml]   [2008]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [patch] x86: some lock annotations for user copy paths
    From
    Date
    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.

    > 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.

    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.



    \
     
     \ /
      Last update: 2008-09-10 17:03    [W:3.479 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site