Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <48a32ceb-0540-7a88-1fdc-419708d2bb8d@nvidia.com>
Date:   Tue, 1 Feb 2022 00:33:03 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
Subject: Re: [PATCH v1 1/1] mm/gup: skip pinnable check for refs==1
Content-Language: en-US
From:   John Hubbard <jhubbard@nvidia.com>
To:     Minchan Kim <minchan@kernel.org>
Cc:     Will McVicker <willmcvicker@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        kernel-team@android.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220131203504.3458775-1-willmcvicker@google.com>
 <27e5f98a-0709-1a80-18ed-e4ccaaf39fe6@nvidia.com>
 <YfjhGKCkQ6h5H5uI@google.com>
 <36068cbd-2020-1961-5034-866a4c7b20cf@nvidia.com>
 <93a8440b-a5f5-1ddd-0834-57808c26d9bb@nvidia.com>
In-Reply-To: <93a8440b-a5f5-1ddd-0834-57808c26d9bb@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ajNsNFQvcFVGc1RYZHY1czFxK2FWS2ljNEZQWHZiODRPMGtkK3dXUlpnRVMx?=
 =?utf-8?B?dmdvNzVrUzBWaDRoZUkyMVVpZ1dzWWZVUXFyNlJ6NDljTE1kdTlLY1JjYWZ2?=
 =?utf-8?B?ODVIREJXQkNmaythaFk3VU5jbHRIZUViQTQwZjFUNTk3TXJ3cTNLWjllZ0R6?=
 =?utf-8?B?ZkRlUTcrRXpqR25ld1hSVFJhOFo4T3d4aUU5VnJCYjZINE0vMWZNU2JhZ3ZV?=
 =?utf-8?B?K1FzdFc2elkxM2d2Q08rejlpZ1A4TWRXSWRVZjEzU3lBajlmakJxRU9jMW4x?=
 =?utf-8?B?anhlTWlzVVNNdWUxN2ZLVTVJUkx0OWJZbkx4eUNVUmptRnhoUkVEQnRiMTE2?=
 =?utf-8?B?NGdBakZycElsZkIrRU41cnc5TllYYjNRa1RzdFdmNTNQTCtueHZsK3Vwb3Iy?=
 =?utf-8?B?MkZ3L2YvbFpMRjBHRDdHTlB2QjF0RW04V0g2eTJNQS8vclhwd1pYT3hKLy90?=
 =?utf-8?B?bFRMbHpMSVpCWEdkVUFNM0pmaDJLbmE1ZUJXUlI5M1I3TzBqM0pxaHAxSzZZ?=
 =?utf-8?B?a1hTTU9ad0VMc0lYUW5iUmM3cWZZL3NrNmprRzFmbDFkTE9Nb2NYRVo1UDNh?=
 =?utf-8?B?ek42V2lNN0VXd0phcCtZWkxmbVZxZm42TE1ScWEyS0FjS3FXLy81WTU2RVM0?=
 =?utf-8?B?REZaZGFTd3ZIb3RLMjVMdzAvOXNsOHZOblNFUzN5WEE0cTROMVRGZFNINFZs?=
 =?utf-8?B?MDhiZ3JnVi9ocEtSQ1NSdGt4aDk1Mk9xbjl4OE5JNWxLa2tLMnR2Yk5IRkxH?=
 =?utf-8?B?eVJPaXZhL3UrRUV0V3hJSmpoZmszWTFTL2RlMFlJQVBaTlVrS3NuK1FTalgz?=
 =?utf-8?B?aE1WanJaYnY0NmJqNkwwN2RjOU9LTWMzRDB0QW9YTnJZTFZSQXlPZWVCbU1r?=
 =?utf-8?B?aS9CS3Yzb3krWWJkYmxLUUNkRWtmMjlnNnNQTWVhUHRMSXU2TW1Zd0NLNHVY?=
 =?utf-8?B?YUFzOVlETUFTVWtTZTlNT1hoNGtMa09XY2F0L3ZLa05ZV3FSOUpxdldMNFZD?=
 =?utf-8?B?clp2MDRVSVMvRUxMOTZRUGlOcHBEajNkMURCUUV6WXhaa0wwa1grdzFXSDMr?=
 =?utf-8?B?OHB6TVo4NnIva2tpN1pkK1liY1FOaGlETGhFbXAyL0RlVmFDWVdyeXlPakhL?=
 =?utf-8?B?N3BJL0FjZm9Lbng1MFJPNHpONndYeVNzRnMvUTFmRGswMDNGbzd4eS8rb0dO?=
 =?utf-8?B?eGlzR3pTQktNQkpuY1VrTWl4UWJnbUd3NlZRR2VpTTJON3ZsRTRaREJscDBP?=
 =?utf-8?B?b3QzaXZpVGFVTEduMWNENVhmNlBsSGFxWHl1RTZHejhkRU5HN3ZMQnVPMHJt?=
 =?utf-8?B?N3g0ZHY3NTcwTTR2Qm1jR2xFSGQ1aGs3QnlhWEdJRzU2TWF2L2dqSmtDWlRj?=
 =?utf-8?B?NFU1K2svbTFua0hMTVkwRDJ0eWx0V3hCZ0c4TVllc0hDQnhlc3RWZXdyR1VY?=
 =?utf-8?B?VmlqMng0MVJiS3ZhUjJVSUZDSHV4R0cwNjBrUWk4ZEFsSVlOZSs5WjI3UU1U?=
 =?utf-8?B?REI4OFdqSEt1YnJkMWJSUEN0bzNobGtNZVNBQkRaUTNUSm1hU1dzaFYwSUZu?=
 =?utf-8?B?anpubmwyTk51UUx6dlhBa29ITFI4WFBVVlVCQzM5KzYzMEp0M1hza2ptdmxv?=
 =?utf-8?B?SGEvRnpJeUprUmEvemd1aDNJalFOUGNMNWhKQTI1cUVhVVRyZ3dVMVlSSzdL?=
 =?utf-8?B?SENPemFkVWtRcmF0UWRZcjM4SFBJS3FIRzFXcnVYWng2bVFycW94S1hBekUx?=
 =?utf-8?B?MUVWOUtoNnRxSkdHcGMxQzRhcVUxblhsY3FiNzVST2VWUVJ4QTBxSDdXRElQ?=
 =?utf-8?B?d05ac2lmQ1krUFFSdFROOFRXWlVvMHBqYmJmQ2R1ckxoR1pvUlZDQlNPdWpX?=
 =?utf-8?B?UVYvTG5OcjVCMW4vUnExNmlEYTlXMktUK2hmNjlDV28wbXI5ZWgrbWc2Y1Nj?=
 =?utf-8?B?Q1dnOE1hMWwxSkxsekhBVlhFVTdXTkFWaExMTy9QTTBWb281YnhaR3BNQzJH?=
 =?utf-8?B?N2NhZ05TQlJTL21RM0owcjFZWWVYSkFrM0RrNzZEQ2kxOTg3K3pJNlcrdVdC?=
 =?utf-8?B?S1FueXgzQndVYVR0L3R1eVJHV1pvVWdKOGFpcFJ3eTYvbTRZeWRYYkZmdWt3?=
 =?utf-8?B?K0dtb2l0MEh4MXozQ0hHTUg0Vkd1VjdpcXAzRzdGRXVrZXc4SlRwZjBDaHc3?=
 =?utf-8?Q?Lbs+v5usb/VXyatJ+XinNoc=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9938e55d-9979-49a2-a833-08d9e55d775e
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2022 08:33:04.8765
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XS6R2OoeJ0e1GIVD/B0wV+kuLqErJrQqyAmJqOhroqC6tzQ3Ng8RaZWt9x6GKW9LLlFXo+0eV1Nu5tVD83kZXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB3294
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/31/22 23:43, John Hubbard wrote:
>>> pin_user_pages
>>>    __get_user_pages_locked
>>>      follow_page_mask
>>>        follow_page_pte
>>>          try_grab_page
>>>            !is_pinnable_page(page)
>>>              return NULL;
>>>          return ERR_PTR(-ENOMEM);
>>>       return -ENOMEM without faultin_page
>>
>> Yes, that's all clear.
>> ...oh, but I guess you're pointing out that it's always going to be
> page-at-a-time this deep in the pin_user_pages() call path. Which is true.
> 
> I hadn't worked through how to fix this yet, my initial reaction was
> that allowing single refs to go through, while prohibiting multiple refs,
> was clearly *not* the way to go.
> 

OK, so after looking at this some more, I think that the real problem
with commit 54d516b1d62f ("mm/gup: small refactoring: simplify
try_grab_page()") is that it funnels fast and slow gup through the same
routine (try_grab_compound_head()). And the problem with *that*, is that
try_grab_compound_head() is coded up with that assumption that it is
being called *only* by fast-gup:

		/*
		 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
		 * right zone, so fail and let the caller fall back to the slow
		 * path.
		 */
		if (unlikely((flags & FOLL_LONGTERM) &&
			     !is_pinnable_page(page)))
			return NULL;

Now, to fix that, I'd really rather not conflate "refs == 1" with "on
the slow path", because that's a conceptual mismatch.

So, other ideas:

a) Remove the above check, and fail fast gup at a different point for
the (FOLL_LONGTERM && !is_pinnable) case. Haven't looked closely at this
yet.

b) Pass in FOLL_FAST_ONLY from all call sites *except* try_grag_page().
Skip the above check in slow-gup (!FOLL_FAST_ONLY) cases.

This makes the code ugly, though, and I'm just listing it here for
completeness.

c) Just call the entire refactoring idea a mistake, and roll it back
either entirely, or enough to keep fast and slow gup separate.

Unless a better idea shows up, (c) is probably the way to go, I think...


thanks,
-- 
John Hubbard
NVIDIA