lkml.org 
[lkml]   [2019]   [Dec]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v11 04/25] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
    From
    Date
    On 12/18/19 8:04 AM, Kirill A. Shutemov wrote:
    > On Mon, Dec 16, 2019 at 02:25:16PM -0800, John Hubbard wrote:
    >> An upcoming patch changes and complicates the refcounting and
    >> especially the "put page" aspects of it. In order to keep
    >> everything clean, refactor the devmap page release routines:
    >>
    >> * Rename put_devmap_managed_page() to page_is_devmap_managed(),
    >> and limit the functionality to "read only": return a bool,
    >> with no side effects.
    >>
    >> * Add a new routine, put_devmap_managed_page(), to handle checking
    >> what kind of page it is, and what kind of refcount handling it
    >> requires.
    >>
    >> * Rename __put_devmap_managed_page() to free_devmap_managed_page(),
    >> and limit the functionality to unconditionally freeing a devmap
    >> page.
    >
    > What the reason to separate put_devmap_managed_page() from
    > free_devmap_managed_page() if free_devmap_managed_page() has exacly one
    > caller? Is it preparation for the next patches?


    Yes. A later patch, #23, adds another caller: __unpin_devmap_managed_user_page().

    ...
    >> @@ -971,7 +971,14 @@ static inline bool put_devmap_managed_page(struct page *page)
    >> return false;
    >> }
    >>
    >> +bool put_devmap_managed_page(struct page *page);
    >> +
    >> #else /* CONFIG_DEV_PAGEMAP_OPS */
    >> +static inline bool page_is_devmap_managed(struct page *page)
    >> +{
    >> + return false;
    >> +}
    >> +
    >> static inline bool put_devmap_managed_page(struct page *page)
    >> {
    >> return false;
    >> @@ -1028,8 +1035,10 @@ static inline void put_page(struct page *page)
    >> * need to inform the device driver through callback. See
    >> * include/linux/memremap.h and HMM for details.
    >> */
    >> - if (put_devmap_managed_page(page))
    >> + if (page_is_devmap_managed(page)) {
    >> + put_devmap_managed_page(page);
    >
    > put_devmap_managed_page() has yet another page_is_devmap_managed() check
    > inside. It looks strange.
    >

    Good point, it's an extra unnecessary check. So to clean it up, I'll note
    that the "if" check is required here in put_page(), in order to stay out of
    non-inlined function calls in the hot path (put_page()). So I'll do the
    following:

    * Leave the above code as it is here

    * Simplify put_devmap_managed_page(), it was trying to do two separate things,
    and those two things have different requirements. So change it to a void
    function, with a WARN_ON_ONCE to assert that page_is_devmap_managed() is true,

    * And change the other caller (release_pages()) to do that check.

    ...
    >> @@ -1102,3 +1102,27 @@ void __init swap_setup(void)
    >> * _really_ don't want to cluster much more
    >> */
    >> }
    >> +
    >> +#ifdef CONFIG_DEV_PAGEMAP_OPS
    >> +bool put_devmap_managed_page(struct page *page)
    >> +{
    >> + bool is_devmap = page_is_devmap_managed(page);
    >> +
    >> + if (is_devmap) {
    >
    > Reversing the condition would save you an indentation level.

    Yes. Done.

    I'll also git-reply with an updated patch so you can see what it looks like.


    thanks,
    --
    John Hubbard
    NVIDIA

    \
     
     \ /
      Last update: 2019-12-19 01:36    [W:2.873 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site