Re: [PATCH v14 022/138] mm/filemap: Add __folio_lock_or_retry()

Vlastimil Babka
2021-08-10T16:08:15Z

On 7/15/21 5:35 AM, Matthew Wilcox (Oracle) wrote:
> Convert __lock_page_or_retry() to __folio_lock_or_retry(). This actually
> saves 4 bytes in the only caller of lock_page_or_retry() (due to better
> register allocation) and saves the 14 byte cost of calling page_folio()
> in __folio_lock_or_retry() for a total saving of 18 bytes. Also use
> a bool for the return type.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Jeff Layton <jlayton@kernel.org>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: William Kucharski <william.kucharski@oracle.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Nit:

> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1625,48 +1625,46 @@ static int __folio_lock_async(struct folio *folio, struct wait_page_queue *wait)
>
> /*
> * Return values:
> - * 1 - page is locked; mmap_lock is still held.
> - * 0 - page is not locked.
> + * true - folio is locked; mmap_lock is still held.
> + * false - folio is not locked.
> * mmap_lock has been released (mmap_read_unlock(), unless flags had both
> * FAULT_FLAG_ALLOW_RETRY and FAULT_FLAG_RETRY_NOWAIT set, in
> * which case mmap_lock is still held.
> *
> * If neither ALLOW_RETRY nor KILLABLE are set, will always return 1

s/1/true/ ? :)

> - * with the page locked and the mmap_lock unperturbed.
> + * with the folio locked and the mmap_lock unperturbed.
> */
> -int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
> +bool __folio_lock_or_retry(struct folio *folio, struct mm_struct *mm,
> unsigned int flags)
> {
> - struct folio *folio = page_folio(page);
> -
> if (fault_flag_allow_retry_first(flags)) {
> /*
> * CAUTION! In this case, mmap_lock is not released
> * even though return 0.
> */
> if (flags & FAULT_FLAG_RETRY_NOWAIT)
> - return 0;
> + return false;
>
> mmap_read_unlock(mm);
> if (flags & FAULT_FLAG_KILLABLE)
> folio_wait_locked_killable(folio);
> else
> folio_wait_locked(folio);
> - return 0;
> + return false;
> }
> if (flags & FAULT_FLAG_KILLABLE) {
> - int ret;
> + bool ret;
>
> ret = __folio_lock_killable(folio);
> if (ret) {
> mmap_read_unlock(mm);
> - return 0;
> + return false;
> }
> } else {
> __folio_lock(folio);
> }
>
> - return 1;
> + return true;
> }
>
> /**