Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Byungchul Park]

On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> Although lock_page() and its family can cause deadlock, the lock
> correctness validator could not be applied to them until now, becasue
> things like unlock_page() might be called in a different context from
> the acquisition context, which violates lockdep's assumption.
> 
> Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> detector to page locks. Applied it.

I expect applying this into lock_page() is more useful than
wait_for_completion(). Could you consider this as the next?

> Signed-off-by: Byungchul Park 
> ---
>  include/linux/mm_types.h |   8 
>  include/linux/pagemap.h  | 101 
> ---
>  lib/Kconfig.debug|   8 
>  mm/filemap.c |   4 +-
>  mm/page_alloc.c  |   3 ++
>  5 files changed, 116 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ff15181..f1e3dba 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -16,6 +16,10 @@
>  
>  #include 
>  
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#include 
> +#endif
> +
>  #ifndef AT_VECTOR_SIZE_ARCH
>  #define AT_VECTOR_SIZE_ARCH 0
>  #endif
> @@ -216,6 +220,10 @@ struct page {
>  #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
>   int _last_cpupid;
>  #endif
> +
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> + struct lockdep_map_cross map;
> +#endif
>  }
>  /*
>   * The struct page can be forced to be double word aligned so that atomic ops
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 9717ca8..9f448c6 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -14,6 +14,9 @@
>  #include 
>  #include  /* for in_interrupt() */
>  #include 
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#include 
> +#endif
>  
>  /*
>   * Bits in mapping->flags.
> @@ -450,26 +453,91 @@ static inline pgoff_t linear_page_index(struct 
> vm_area_struct *vma,
>   return pgoff;
>  }
>  
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#define lock_page_init(p)\
> +do { \
> + static struct lock_class_key __key; \
> + lockdep_init_map_crosslock((struct lockdep_map *)&(p)->map, \
> + "(PG_locked)" #p, &__key, 0);   \
> +} while (0)
> +
> +static inline void lock_page_acquire(struct page *page, int try)
> +{
> + page = compound_head(page);
> + lock_acquire_exclusive((struct lockdep_map *)>map, 0,
> +try, NULL, _RET_IP_);
> +}
> +
> +static inline void lock_page_release(struct page *page)
> +{
> + page = compound_head(page);
> + /*
> +  * lock_commit_crosslock() is necessary for crosslocks.
> +  */
> + lock_commit_crosslock((struct lockdep_map *)>map);
> + lock_release((struct lockdep_map *)>map, 0, _RET_IP_);
> +}
> +#else
> +static inline void lock_page_init(struct page *page) {}
> +static inline void lock_page_free(struct page *page) {}
> +static inline void lock_page_acquire(struct page *page, int try) {}
> +static inline void lock_page_release(struct page *page) {}
> +#endif
> +
>  extern void __lock_page(struct page *page);
>  extern int __lock_page_killable(struct page *page);
>  extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
>   unsigned int flags);
> -extern void unlock_page(struct page *page);
> +extern void do_raw_unlock_page(struct page *page);
>  
> -static inline int trylock_page(struct page *page)
> +static inline void unlock_page(struct page *page)
> +{
> + lock_page_release(page);
> + do_raw_unlock_page(page);
> +}
> +
> +static inline int do_raw_trylock_page(struct page *page)
>  {
>   page = compound_head(page);
>   return (likely(!test_and_set_bit_lock(PG_locked, >flags)));
>  }
>  
> +static inline int trylock_page(struct page *page)
> +{
> + if (do_raw_trylock_page(page)) {
> + lock_page_acquire(page, 1);
> + return 1;
> + }
> + return 0;
> +}
> +
>  /*
>   * lock_page may only be called if we have the page's inode pinned.
>   */
>  static inline void lock_page(struct page *page)
>  {
>   might_sleep();
> - if (!trylock_page(page))
> +
> + if (!do_raw_trylock_page(page))
>   __lock_page(page);
> + /*
> +  * acquire() must be after actual lock operation for crosslocks.
> +  * This way a crosslock and current lock can be ordered like:
> +  *
> +  *  CONTEXT 1   CONTEXT 2
> +  *  -   -
> +  *  lock A (cross)
> +  *  acquire A
> +  *X = atomic_inc_return(_gen_id)
> +  *  ~
> +  *  acquire B
> +  *Y = 

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Byungchul Park]

On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> Although lock_page() and its family can cause deadlock, the lock
> correctness validator could not be applied to them until now, becasue
> things like unlock_page() might be called in a different context from
> the acquisition context, which violates lockdep's assumption.
> 
> Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> detector to page locks. Applied it.

I expect applying this into lock_page() is more useful than
wait_for_completion(). Could you consider this as the next?

> Signed-off-by: Byungchul Park 
> ---
>  include/linux/mm_types.h |   8 
>  include/linux/pagemap.h  | 101 
> ---
>  lib/Kconfig.debug|   8 
>  mm/filemap.c |   4 +-
>  mm/page_alloc.c  |   3 ++
>  5 files changed, 116 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ff15181..f1e3dba 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -16,6 +16,10 @@
>  
>  #include 
>  
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#include 
> +#endif
> +
>  #ifndef AT_VECTOR_SIZE_ARCH
>  #define AT_VECTOR_SIZE_ARCH 0
>  #endif
> @@ -216,6 +220,10 @@ struct page {
>  #ifdef LAST_CPUPID_NOT_IN_PAGE_FLAGS
>   int _last_cpupid;
>  #endif
> +
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> + struct lockdep_map_cross map;
> +#endif
>  }
>  /*
>   * The struct page can be forced to be double word aligned so that atomic ops
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 9717ca8..9f448c6 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -14,6 +14,9 @@
>  #include 
>  #include  /* for in_interrupt() */
>  #include 
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#include 
> +#endif
>  
>  /*
>   * Bits in mapping->flags.
> @@ -450,26 +453,91 @@ static inline pgoff_t linear_page_index(struct 
> vm_area_struct *vma,
>   return pgoff;
>  }
>  
> +#ifdef CONFIG_LOCKDEP_PAGELOCK
> +#define lock_page_init(p)\
> +do { \
> + static struct lock_class_key __key; \
> + lockdep_init_map_crosslock((struct lockdep_map *)&(p)->map, \
> + "(PG_locked)" #p, &__key, 0);   \
> +} while (0)
> +
> +static inline void lock_page_acquire(struct page *page, int try)
> +{
> + page = compound_head(page);
> + lock_acquire_exclusive((struct lockdep_map *)>map, 0,
> +try, NULL, _RET_IP_);
> +}
> +
> +static inline void lock_page_release(struct page *page)
> +{
> + page = compound_head(page);
> + /*
> +  * lock_commit_crosslock() is necessary for crosslocks.
> +  */
> + lock_commit_crosslock((struct lockdep_map *)>map);
> + lock_release((struct lockdep_map *)>map, 0, _RET_IP_);
> +}
> +#else
> +static inline void lock_page_init(struct page *page) {}
> +static inline void lock_page_free(struct page *page) {}
> +static inline void lock_page_acquire(struct page *page, int try) {}
> +static inline void lock_page_release(struct page *page) {}
> +#endif
> +
>  extern void __lock_page(struct page *page);
>  extern int __lock_page_killable(struct page *page);
>  extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
>   unsigned int flags);
> -extern void unlock_page(struct page *page);
> +extern void do_raw_unlock_page(struct page *page);
>  
> -static inline int trylock_page(struct page *page)
> +static inline void unlock_page(struct page *page)
> +{
> + lock_page_release(page);
> + do_raw_unlock_page(page);
> +}
> +
> +static inline int do_raw_trylock_page(struct page *page)
>  {
>   page = compound_head(page);
>   return (likely(!test_and_set_bit_lock(PG_locked, >flags)));
>  }
>  
> +static inline int trylock_page(struct page *page)
> +{
> + if (do_raw_trylock_page(page)) {
> + lock_page_acquire(page, 1);
> + return 1;
> + }
> + return 0;
> +}
> +
>  /*
>   * lock_page may only be called if we have the page's inode pinned.
>   */
>  static inline void lock_page(struct page *page)
>  {
>   might_sleep();
> - if (!trylock_page(page))
> +
> + if (!do_raw_trylock_page(page))
>   __lock_page(page);
> + /*
> +  * acquire() must be after actual lock operation for crosslocks.
> +  * This way a crosslock and current lock can be ordered like:
> +  *
> +  *  CONTEXT 1   CONTEXT 2
> +  *  -   -
> +  *  lock A (cross)
> +  *  acquire A
> +  *X = atomic_inc_return(_gen_id)
> +  *  ~
> +  *  acquire B
> +  *Y = atomic_read_acquire(_gen_id)
> +  *   

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Peter Zijlstra]

On Thu, Aug 10, 2017 at 10:35:02AM +0900, Byungchul Park wrote:
> On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> > Although lock_page() and its family can cause deadlock, the lock
> > correctness validator could not be applied to them until now, becasue
> > things like unlock_page() might be called in a different context from
> > the acquisition context, which violates lockdep's assumption.
> > 
> > Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> > detector to page locks. Applied it.
> 
> Is there any reason excluding applying it into PG_locked?

Wanted to start small.. 

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Peter Zijlstra]

On Thu, Aug 10, 2017 at 10:35:02AM +0900, Byungchul Park wrote:
> On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> > Although lock_page() and its family can cause deadlock, the lock
> > correctness validator could not be applied to them until now, becasue
> > things like unlock_page() might be called in a different context from
> > the acquisition context, which violates lockdep's assumption.
> > 
> > Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> > detector to page locks. Applied it.
> 
> Is there any reason excluding applying it into PG_locked?

Wanted to start small.. 

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Byungchul Park]

On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> Although lock_page() and its family can cause deadlock, the lock
> correctness validator could not be applied to them until now, becasue
> things like unlock_page() might be called in a different context from
> the acquisition context, which violates lockdep's assumption.
> 
> Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> detector to page locks. Applied it.

Is there any reason excluding applying it into PG_locked?

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[Byungchul Park]

On Mon, Aug 07, 2017 at 04:12:58PM +0900, Byungchul Park wrote:
> Although lock_page() and its family can cause deadlock, the lock
> correctness validator could not be applied to them until now, becasue
> things like unlock_page() might be called in a different context from
> the acquisition context, which violates lockdep's assumption.
> 
> Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> detector to page locks. Applied it.

Is there any reason excluding applying it into PG_locked?

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[kbuild test robot]

Hi Byungchul,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.13-rc4 next-20170807]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Byungchul-Park/lockdep-Implement-crossrelease-feature/20170807-172617
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All warnings (new ones prefixed by >>):

warning: (LOCKDEP_COMPLETE && LOCKDEP_PAGELOCK) selects LOCKDEP_CROSSRELEASE 
which has unmet direct dependencies (PROVE_LOCKING)

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v8 11/14] lockdep: Apply crossrelease to PG_locked locks

[kbuild test robot]

Hi Byungchul,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.13-rc4 next-20170807]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Byungchul-Park/lockdep-Implement-crossrelease-feature/20170807-172617
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All warnings (new ones prefixed by >>):

warning: (LOCKDEP_COMPLETE && LOCKDEP_PAGELOCK) selects LOCKDEP_CROSSRELEASE 
which has unmet direct dependencies (PROVE_LOCKING)

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip