Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v3] mm/swap: Fix release_pages() when releasing devmap
 pages
To:     Dan Williams <dan.j.williams@intel.com>
CC:     "Weiny, Ira" <ira.weiny@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Michal Hocko <mhocko@suse.com>, Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= <jglisse@redhat.com>
References: <20190604164813.31514-1-ira.weiny@intel.com>
 <cfd74a0f-71b5-1ece-80af-7f415321d5c1@nvidia.com>
 <CAPcyv4hmN7M3Y1HzVGSi9JuYKUUmvBRgxmkdYdi_6+H+eZAyHA@mail.gmail.com>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <4d97645c-0e55-37c0-1a16-8649706b9e78@nvidia.com>
Date:   Tue, 4 Jun 2019 13:17:42 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.0
MIME-Version: 1.0
In-Reply-To: <CAPcyv4hmN7M3Y1HzVGSi9JuYKUUmvBRgxmkdYdi_6+H+eZAyHA@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 6/4/19 1:11 PM, Dan Williams wrote:
> On Tue, Jun 4, 2019 at 12:48 PM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 6/4/19 9:48 AM, ira.weiny@intel.com wrote:
>>> From: Ira Weiny <ira.weiny@intel.com>
>>>
...
>>> diff --git a/mm/swap.c b/mm/swap.c
>>> index 7ede3eddc12a..6d153ce4cb8c 100644
>>> --- a/mm/swap.c
>>> +++ b/mm/swap.c
>>> @@ -740,15 +740,20 @@ void release_pages(struct page **pages, int nr)
>>>               if (is_huge_zero_page(page))
>>>                       continue;
>>>
>>> -             /* Device public page can not be huge page */
>>> -             if (is_device_public_page(page)) {
>>> +             if (is_zone_device_page(page)) {
>>>                       if (locked_pgdat) {
>>>                               spin_unlock_irqrestore(&locked_pgdat->lru_lock,
>>>                                                      flags);
>>>                               locked_pgdat = NULL;
>>>                       }
>>> -                     put_devmap_managed_page(page);
>>> -                     continue;
>>> +                     /*
>>> +                      * Not all zone-device-pages require special
>>> +                      * processing.  Those pages return 'false' from
>>> +                      * put_devmap_managed_page() expecting a call to
>>> +                      * put_page_testzero()
>>> +                      */
>>
>> Just a documentation tweak: how about:
>>
>>                         /*
>>                          * ZONE_DEVICE pages that return 'false' from
>>                          * put_devmap_managed_page() do not require special
>>                          * processing, and instead, expect a call to
>>                          * put_page_testzero().
>>                          */
> 
> Looks better to me, but maybe just go ahead and list those
> expectations explicitly. Something like:
> 
>                         /*
>                          * put_devmap_managed_page() only handles
>                          * ZONE_DEVICE (struct dev_pagemap managed)
>                          * pages when the hosting dev_pagemap has the
>                          * ->free() or ->fault() callback handlers
>                          *  implemented as indicated by
>                          *  dev_pagemap.type. Otherwise the expectation
>                          *  is to fall back to a plain decrement /
>                          *  put_page_testzero().
>                          */

I like it--but not here, because it's too much internal detail in a
call site that doesn't use that level of detail. The call site looks
at the return value, only.

Let's instead put that blurb above (or in) the put_devmap_managed_page() 
routine itself. And leave the blurb that I wrote where it is. And then I
think everything will have an appropriate level of detail in the right places.


thanks,
-- 
John Hubbard
NVIDIA