diff mbox series

[v10,18/33] mm/filemap: Add folio_unlock

Message ID 20210511214735.1836149-19-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Memory folios | expand

Commit Message

Matthew Wilcox May 11, 2021, 9:47 p.m. UTC 
Convert unlock_page() to call folio_unlock().  By using a folio we
avoid a call to compound_head().  This shortens the function from 39
bytes to 25 and removes 4 instructions on x86-64.  Because we still
have unlock_page(), it's a net increase of 24 bytes of text for the
kernel as a whole, but any path that uses folio_unlock() will execute
4 fewer instructions.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/pagemap.h |  3 ++-
 mm/filemap.c            | 27 ++++++++++-----------------
 mm/folio-compat.c       |  6 ++++++
 3 files changed, 18 insertions(+), 18 deletions(-)

Comments

Vlastimil Babka May 18, 2021, 10:06 a.m. UTC | #1 
On 5/11/21 11:47 PM, Matthew Wilcox (Oracle) wrote:
> Convert unlock_page() to call folio_unlock().  By using a folio we
> avoid a call to compound_head().  This shortens the function from 39
> bytes to 25 and removes 4 instructions on x86-64.  Because we still
> have unlock_page(), it's a net increase of 24 bytes of text for the
> kernel as a whole, but any path that uses folio_unlock() will execute
> 4 fewer instructions.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Jeff Layton <jlayton@kernel.org>
> ---
>  include/linux/pagemap.h |  3 ++-
>  mm/filemap.c            | 27 ++++++++++-----------------
>  mm/folio-compat.c       |  6 ++++++
>  3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index 1f37d7656955..8dbba0074536 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -643,7 +643,8 @@ extern int __lock_page_killable(struct page *page);
>  extern int __lock_page_async(struct page *page, struct wait_page_queue *wait);
>  extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
>  				unsigned int flags);
> -extern void unlock_page(struct page *page);
> +void unlock_page(struct page *page);
> +void folio_unlock(struct folio *folio);
>  
>  /*
>   * Return true if the page was successfully locked
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 817a47059bd0..e7a6a58d6cd9 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1435,29 +1435,22 @@ static inline bool clear_bit_unlock_is_negative_byte(long nr, volatile void *mem
>  #endif
>  
>  /**
> - * unlock_page - unlock a locked page
> - * @page: the page
> + * folio_unlock - Unlock a locked folio.
> + * @folio: The folio.
>   *
> - * Unlocks the page and wakes up sleepers in wait_on_page_locked().
> - * Also wakes sleepers in wait_on_page_writeback() because the wakeup
> - * mechanism between PageLocked pages and PageWriteback pages is shared.
> - * But that's OK - sleepers in wait_on_page_writeback() just go back to sleep.
> + * Unlocks the folio and wakes up any thread sleeping on the page lock.
>   *
> - * Note that this depends on PG_waiters being the sign bit in the byte
> - * that contains PG_locked - thus the BUILD_BUG_ON(). That allows us to
> - * clear the PG_locked bit and test PG_waiters at the same time fairly
> - * portably (architectures that do LL/SC can test any bit, while x86 can
> - * test the sign bit).

Was it necessary to remove the comments about wait_on_page_writeback() and
PG_waiters etc?

> + * Context: May be called from interrupt or process context.  May not be
> + * called from NMI context.

Where did the NMI part come from?

>   */
> -void unlock_page(struct page *page)
> +void folio_unlock(struct folio *folio)
>  {
>  	BUILD_BUG_ON(PG_waiters != 7);
> -	page = compound_head(page);
> -	VM_BUG_ON_PAGE(!PageLocked(page), page);
> -	if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
> -		wake_up_page_bit(page, PG_locked);
> +	VM_BUG_ON_FOLIO(!folio_locked(folio), folio);
> +	if (clear_bit_unlock_is_negative_byte(PG_locked, folio_flags(folio, 0)))
> +		wake_up_page_bit(&folio->page, PG_locked);
>  }
> -EXPORT_SYMBOL(unlock_page);
> +EXPORT_SYMBOL(folio_unlock);
>  
>  /**
>   * end_page_private_2 - Clear PG_private_2 and release any waiters
> diff --git a/mm/folio-compat.c b/mm/folio-compat.c
> index 5e107aa30a62..91b3d00a92f7 100644
> --- a/mm/folio-compat.c
> +++ b/mm/folio-compat.c
> @@ -11,3 +11,9 @@ struct address_space *page_mapping(struct page *page)
>  	return folio_mapping(page_folio(page));
>  }
>  EXPORT_SYMBOL(page_mapping);
> +
> +void unlock_page(struct page *page)
> +{
> +	return folio_unlock(page_folio(page));
> +}
> +EXPORT_SYMBOL(unlock_page);
>
Matthew Wilcox May 18, 2021, 11:30 a.m. UTC | #2 
On Tue, May 18, 2021 at 12:06:42PM +0200, Vlastimil Babka wrote:
> >  /**
> > - * unlock_page - unlock a locked page
> > - * @page: the page
> > + * folio_unlock - Unlock a locked folio.
> > + * @folio: The folio.
> >   *
> > - * Unlocks the page and wakes up sleepers in wait_on_page_locked().
> > - * Also wakes sleepers in wait_on_page_writeback() because the wakeup
> > - * mechanism between PageLocked pages and PageWriteback pages is shared.
> > - * But that's OK - sleepers in wait_on_page_writeback() just go back to sleep.
> > + * Unlocks the folio and wakes up any thread sleeping on the page lock.
> >   *
> > - * Note that this depends on PG_waiters being the sign bit in the byte
> > - * that contains PG_locked - thus the BUILD_BUG_ON(). That allows us to
> > - * clear the PG_locked bit and test PG_waiters at the same time fairly
> > - * portably (architectures that do LL/SC can test any bit, while x86 can
> > - * test the sign bit).
> 
> Was it necessary to remove the comments about wait_on_page_writeback() and
> PG_waiters etc?

I think so.  This kernel-doc is for the person who wants to understand
how to use the function, not for the person who wants to understand why
the function is written the way it is.  For that person, we have git log
messages and other comments dotted throughout, eg the comment on
clear_bit_unlock_is_negative_byte() in mm/filemap.c and the comment
on PG_waiters in include/linux/page-flags.h.

> > + * Context: May be called from interrupt or process context.  May not be
> > + * called from NMI context.
> 
> Where did the NMI part come from?

If you're in NMI context and call unlock_page() and the page has a
waiter on it, we call folio_wake_bit(), which calls spin_lock_irqsave()
on the wait_queue_head_t lock, which I believe cannot be done safely 
from NMI context (as the NMI may have interrupted us while holding
that lock).
diff mbox series

Patch

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 1f37d7656955..8dbba0074536 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -643,7 +643,8 @@  extern int __lock_page_killable(struct page *page);
 extern int __lock_page_async(struct page *page, struct wait_page_queue *wait);
 extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
 				unsigned int flags);
-extern void unlock_page(struct page *page);
+void unlock_page(struct page *page);
+void folio_unlock(struct folio *folio);
 
 /*
  * Return true if the page was successfully locked
diff --git a/mm/filemap.c b/mm/filemap.c
index 817a47059bd0..e7a6a58d6cd9 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1435,29 +1435,22 @@  static inline bool clear_bit_unlock_is_negative_byte(long nr, volatile void *mem
 #endif
 
 /**
- * unlock_page - unlock a locked page
- * @page: the page
+ * folio_unlock - Unlock a locked folio.
+ * @folio: The folio.
  *
- * Unlocks the page and wakes up sleepers in wait_on_page_locked().
- * Also wakes sleepers in wait_on_page_writeback() because the wakeup
- * mechanism between PageLocked pages and PageWriteback pages is shared.
- * But that's OK - sleepers in wait_on_page_writeback() just go back to sleep.
+ * Unlocks the folio and wakes up any thread sleeping on the page lock.
  *
- * Note that this depends on PG_waiters being the sign bit in the byte
- * that contains PG_locked - thus the BUILD_BUG_ON(). That allows us to
- * clear the PG_locked bit and test PG_waiters at the same time fairly
- * portably (architectures that do LL/SC can test any bit, while x86 can
- * test the sign bit).
+ * Context: May be called from interrupt or process context.  May not be
+ * called from NMI context.
  */
-void unlock_page(struct page *page)
+void folio_unlock(struct folio *folio)
 {
 	BUILD_BUG_ON(PG_waiters != 7);
-	page = compound_head(page);
-	VM_BUG_ON_PAGE(!PageLocked(page), page);
-	if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
-		wake_up_page_bit(page, PG_locked);
+	VM_BUG_ON_FOLIO(!folio_locked(folio), folio);
+	if (clear_bit_unlock_is_negative_byte(PG_locked, folio_flags(folio, 0)))
+		wake_up_page_bit(&folio->page, PG_locked);
 }
-EXPORT_SYMBOL(unlock_page);
+EXPORT_SYMBOL(folio_unlock);
 
 /**
  * end_page_private_2 - Clear PG_private_2 and release any waiters
diff --git a/mm/folio-compat.c b/mm/folio-compat.c
index 5e107aa30a62..91b3d00a92f7 100644
--- a/mm/folio-compat.c
+++ b/mm/folio-compat.c
@@ -11,3 +11,9 @@  struct address_space *page_mapping(struct page *page)
 	return folio_mapping(page_folio(page));
 }
 EXPORT_SYMBOL(page_mapping);
+
+void unlock_page(struct page *page)
+{
+	return folio_unlock(page_folio(page));
+}
+EXPORT_SYMBOL(unlock_page);