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 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 C2CE2C28CBC for ; Wed, 6 May 2020 16:08:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 77FC620736 for ; Wed, 6 May 2020 16:08:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="eyheCbu3" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 77FC620736 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 0E7968E0006; Wed, 6 May 2020 12:08:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0BF9F8E0003; Wed, 6 May 2020 12:08:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EEF5B8E0006; Wed, 6 May 2020 12:08:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0100.hostedemail.com [216.40.44.100]) by kanga.kvack.org (Postfix) with ESMTP id D8BE18E0003 for ; Wed, 6 May 2020 12:08:53 -0400 (EDT) Received: from smtpin07.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 8A1D8440E for ; Wed, 6 May 2020 16:08:53 +0000 (UTC) X-FDA: 76786777746.07.fire55_307973b47b235 X-HE-Tag: fire55_307973b47b235 X-Filterd-Recvd-Size: 7256 Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) by imf29.hostedemail.com (Postfix) with ESMTP for ; Wed, 6 May 2020 16:08:52 +0000 (UTC) Received: by mail-lf1-f65.google.com with SMTP id b26so1830853lfa.5 for ; Wed, 06 May 2020 09:08:52 -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=if5qgNlscRmpJPzG9Z3gnyaxZ0iIBynHsVjhr1Dlctg=; b=eyheCbu3Wl4jWBLi16KmwXC3GUZfSR8nyYtYtWYhHzvEAIgisTgNndWd+2/Dmn4Fu3 1/oZO1IAPn6+vOlMTBDdlshLkF7l+t81nke8HHUt/QG94oWJqyYv91zK/x8cIh2JkzYY SiY6Acsis1eShDIqatzkzYNXXugeecGd27RlgztExMUWAy7ZyTg+hq899Yt9jwljo6NT oTUEHhn9ghJ9p/zYohANqpmvT5SK6bz6NY+aHmz3H2IcRdRkvmLUpcLxtsR0QBj8Wziu Vesw1YJeeNK8g67yToViiZOrPAaIQJBycwOimRy0oehpSLGYq4Y838PHAzbbzZBH0LR8 sHlQ== 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=if5qgNlscRmpJPzG9Z3gnyaxZ0iIBynHsVjhr1Dlctg=; b=iB+R32GHUuhlLpdLVYB18oDwoL1k8LjmT3NY6fHpEVhN8G7KJrZZuRrzVUE73hHV92 AzmZ5Q2Gm7+VaHfwoonWb8nnd5dBNRFGMoTqr1p0VaCz8ligPEhhL/OIVlm1l1srPR7L JFYer4IkBvLXYOWRpriP9vfnzqHFvFIe5Y/eqzP4BHHr79XwvJ2Kpl0hRrQKKVUpldqL GrvPYI5iT8EZorbYt9gTqY1VydGODsNSMGamuAcPEO2TCwMHup8BEaqbgqYI1of/OYAD TMncDGap03qZ72lbMmIrykX4Vvra4snYHBrsNHx+143LrLBnvfsCzjSfeZ3+6VR4lGpz LDjw== X-Gm-Message-State: AGi0PuZk5BLoT/FutHASyB1PG+cLzgqg1KrqiDVB5ON+T2g+ZjGT00kv WwL124jU6GtZUvSmN3eAKh9s8snH1wJZ+uuphFg= X-Google-Smtp-Source: APiQypIkTeJRFv9Pb7bCztfKTE6r/gqFVdDFRACyd8JugwVuYo9Fzohu16CEHDV6PG4WB2H/mhehSpBDfnzO3UangrI= X-Received: by 2002:ac2:4105:: with SMTP id b5mr5786969lfi.94.1588781331398; Wed, 06 May 2020 09:08:51 -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> In-Reply-To: <20200506125930.GJ17863@quack2.suse.cz> From: Souptick Joarder Date: Wed, 6 May 2020 21:38:40 +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.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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(). 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().