From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D70C2C38A2A for ; Thu, 7 May 2020 10:32:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8C5D0207DD for ; Thu, 7 May 2020 10:32:26 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="abEqiduR" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8C5D0207DD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 37C8A900006; Thu, 7 May 2020 06:32:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 35291900002; Thu, 7 May 2020 06:32:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 267A1900006; Thu, 7 May 2020 06:32:26 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0181.hostedemail.com [216.40.44.181]) by kanga.kvack.org (Postfix) with ESMTP id 0FAA1900002 for ; Thu, 7 May 2020 06:32:26 -0400 (EDT) Received: from smtpin22.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C5E4182499A8 for ; Thu, 7 May 2020 10:32:25 +0000 (UTC) X-FDA: 76789558650.22.ducks44_4f61bf0ee9349 X-HE-Tag: ducks44_4f61bf0ee9349 X-Filterd-Recvd-Size: 8634 Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) by imf04.hostedemail.com (Postfix) with ESMTP for ; Thu, 7 May 2020 10:32:25 +0000 (UTC) Received: by mail-lj1-f194.google.com with SMTP id w20so5752875ljj.0 for ; Thu, 07 May 2020 03:32:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=lxR8iUX+nq/8LE2/542OxQ8ktlSJ6cXRsEzWoH8lfzA=; b=abEqiduRP5XA93Rx8i0WKwGEH53+CCt70vEpJwG+coIZSFwLyoUVgyzxNQnhjSMrhr 6zKwL9MZfH+5sx9PZfTjosjryBVs8a7aq6aXudAxh15XVqz6iPDDQSVO08IHWKbBZFhH CYQgB0X/jVcaIR/7aGSsp0v7BTgC1XfXRGaRSP+5nSf409yP6qAEQupO9gEmqRFwLcEe H0pnccwikFJtaFyj3dxp7wVF9McYQ8TKb9l/rbtyEudKRCSQDWTnnKv9kipYQ+rsuQFD CS+QGZ/zIXq6iK4F2JEY1s6tyYuYIM07mjId6iMVZCrPXuWYp4/siWmeNVpn7W+WWJCx t9tA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=lxR8iUX+nq/8LE2/542OxQ8ktlSJ6cXRsEzWoH8lfzA=; b=VM9isdi78ugMv6DeHxV+fcHWs8U6kh+Z274xNiSHK3jL4rWi4trlEYXgoGOJBCfWNx pMFXbqC4F6fPDauw89k8bA/JKHvC8tP37hJuURXdWyoqGEr18Kve2AaRrrISVgwJWqn+ hJ+pgi/dokjbH2wiIqbsNAsWXtA8yqNw8i938E4NbyrLlAr47aEw7bitOeXNjQQaw2mV GJdiF0UE3Z8acPTkiEe+bHizjDZEplqPqRaxDfwUN+4PxGi9xhH7nOtZRRQ6LKI7HdcY v79aZVvzZHi0+n8xjikOZb/qM030WOgSDEEycfW25Jktes71Rp9hZIMTgiGjvHYvnoxN 613g== X-Gm-Message-State: AGi0PuZMOENt+p2zUtNaGyt5Ehtv48Hi6Af7nmHXx3g4QwKevM553F4c UTEyxmpD1kr/HW/OYUR02E48dNx26kHWQpVoPy0= X-Google-Smtp-Source: APiQypLqntGlzwMTPnOwy1h5uEIH4hExzJxYqIPLwFHTltoybWYvpOoMaGdS9r61zRjkIUO9oVrCQTWqJErRd8MJv5U= X-Received: by 2002:a2e:7613:: with SMTP id r19mr7726906ljc.29.1588847542720; Thu, 07 May 2020 03:32:22 -0700 (PDT) MIME-Version: 1.0 References: <1588706059-4208-1-git-send-email-jrdr.linux@gmail.com> <0bfe4a8a-0d91-ef9b-066f-2ea7c68571b3@nvidia.com> <20200506100649.GI17863@quack2.suse.cz> <20200506125930.GJ17863@quack2.suse.cz> <20200507101322.GB30922@quack2.suse.cz> In-Reply-To: <20200507101322.GB30922@quack2.suse.cz> From: Souptick Joarder Date: Thu, 7 May 2020 16:02:10 +0530 Message-ID: Subject: Re: [RFC] mm/gup.c: Updated return value of {get|pin}_user_pages_fast() To: Jan Kara Cc: John Hubbard , Tony Luck , fenghua.yu@intel.com, Rob Springer , Todd Poynor , benchan@chromium.org, Greg KH , Jens Wiklander , Andrew Morton , santosh.shilimkar@oracle.com, "David S. Miller" , kuba@kernel.org, Ira Weiny , =?UTF-8?B?SsOpcsO0bWUgR2xpc3Nl?= , inux-ia64@vger.kernel.org, linux-kernel@vger.kernel.org, "open list:ANDROID DRIVERS" , tee-dev@lists.linaro.org, Linux-MM , netdev@vger.kernel.org, linux-rdma@vger.kernel.org, rds-devel@oss.oracle.com Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000017, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, May 7, 2020 at 3:43 PM Jan Kara wrote: > > On Wed 06-05-20 21:38:40, Souptick Joarder wrote: > > On Wed, May 6, 2020 at 6:29 PM Jan Kara wrote: > > > > > > On Wed 06-05-20 17:51:39, Souptick Joarder wrote: > > > > On Wed, May 6, 2020 at 3:36 PM Jan Kara wrote: > > > > > > > > > > On Wed 06-05-20 02:06:56, Souptick Joarder wrote: > > > > > > On Wed, May 6, 2020 at 1:08 AM John Hubbard wrote: > > > > > > > > > > > > > > On 2020-05-05 12:14, Souptick Joarder wrote: > > > > > > > > Currently {get|pin}_user_pages_fast() have 3 return value 0, -errno > > > > > > > > and no of pinned pages. The only case where these two functions will > > > > > > > > return 0, is for nr_pages <= 0, which doesn't find a valid use case. > > > > > > > > But if at all any, then a -ERRNO will be returned instead of 0, which > > > > > > > > means {get|pin}_user_pages_fast() will have 2 return values -errno & > > > > > > > > no of pinned pages. > > > > > > > > > > > > > > > > Update all the callers which deals with return value 0 accordingly. > > > > > > > > > > > > > > Hmmm, seems a little shaky. In order to do this safely, I'd recommend > > > > > > > first changing gup_fast/pup_fast so so that they return -EINVAL if > > > > > > > the caller specified nr_pages==0, and of course auditing all callers, > > > > > > > to ensure that this won't cause problems. > > > > > > > > > > > > While auditing it was figured out, there are 5 callers which cares for > > > > > > return value > > > > > > 0 of gup_fast/pup_fast. What problem it might cause if we change > > > > > > gup_fast/pup_fast > > > > > > to return -EINVAL and update all the callers in a single commit ? > > > > > > > > > > Well, first I'd ask a different question: Why do you want to change the > > > > > current behavior? It's not like the current behavior is confusing. Callers > > > > > that pass >0 pages can happily rely on the simple behavior of < 0 return on > > > > > error or > 0 return if we mapped some pages. Callers that can possibly ask > > > > > to map 0 pages can get 0 pages back - kind of expected - and I don't see > > > > > any benefit in trying to rewrite these callers to handle -EINVAL instead... > > > > > > > > Callers with a request to map 0 pages doesn't have a valid use case. But if any > > > > caller end up doing it mistakenly, -errno should be returned to caller > > > > rather than 0 > > > > which will indicate more precisely that map 0 pages is not a valid > > > > request from caller. > > > > > > Well, I believe this depends on the point of view. Similarly as reading 0 > > > bytes is successful, we could consider mapping 0 pages successful as well. > > > And there can be valid cases where number of pages to map is computed from > > > some input and when 0 pages should be mapped, it is not a problem and your > > > change would force such callers to special case this with explicitely > > > checking for 0 pages to map and not calling GUP in that case at all. > > > > > > I'm not saying what you propose is necessarily bad, I just say I don't find > > > it any better than the current behavior and so IMO it's not worth the > > > churn. Now if you can come up with some examples of current in-kernel users > > > who indeed do get the handling of the return value wrong, I could be > > > convinced otherwise. > > > > There are 5 callers of {get|pin}_user_pages_fast(). > > Oh, there are *much* more callers that 5. It's more like 70. Just grep the > source... And then you have all other {get|pin}_user_pages() variants that > need to be kept consistent. So overall we have over 200 calls to some > variant of GUP. Sorry, I mean, there are 5 callers of {get|pin}_user_pages_fast() who have interest in return value 0, out of total 42. > > > arch/ia64/kernel/err_inject.c#L145 > > staging/gasket/gasket_page_table.c#L489 > > > > Checking return value 0 doesn't make sense for above 2. > > > > drivers/platform/goldfish/goldfish_pipe.c#L277 > > net/rds/rdma.c#L165 > > drivers/tee/tee_shm.c#L262 > > > > These 3 callers have calculated the no of pages value before passing it to > > {get|pin}_user_pages_fast(). But if they end up passing nr_pages <= 0, a return > > value of either 0 or -EINVAL doesn't going to harm any existing > > behavior of callers. > > > > IMO, it is safe to return -errno for nr_pages <= 0, for > > {get|pin}_user_pages_fast(). > > OK, so no real problem with any of these callers. I still don't see a > justification for the churn you suggest... Auditting all those code sites > is going to be pretty tedious. I try to audit all 42 callers of {get|pin}_user_pages_fast() and figure out these 5 callers which need to be updated and I think, other callers of {get|pin}_user_pages_fast() will not be effected. But I didn't go through other variants of gup/pup except {get|pin}_user_pages_fast().