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;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-431-g1d6a3ebb56-fm-20230511.001-g1d6a3ebb
Mime-Version: 1.0
Message-Id: <7d7ddc48-5985-4678-9f87-6e9b574a24d9@app.fastmail.com>
In-Reply-To: <5b071f65-7f87-4a7b-a76a-f4a1c1568ae7@lucifer.local>
References: <20230519093953.10972-1-arnd@kernel.org>
 <5b071f65-7f87-4a7b-a76a-f4a1c1568ae7@lucifer.local>
Date:   Fri, 19 May 2023 17:09:35 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Lorenzo Stoakes" <lstoakes@gmail.com>,
        "Arnd Bergmann" <arnd@kernel.org>
Cc:     "Andrew Morton" <akpm@linux-foundation.org>,
        "Catalin Marinas" <catalin.marinas@arm.com>,
        "Will Deacon" <will@kernel.org>,
        "Peter Zijlstra" <peterz@infradead.org>,
        "Ingo Molnar" <mingo@redhat.com>,
        "Arnaldo Carvalho de Melo" <acme@kernel.org>,
        "Mark Rutland" <mark.rutland@arm.com>,
        "Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
        "Jiri Olsa" <jolsa@kernel.org>,
        "Namhyung Kim" <namhyung@kernel.org>,
        "Ian Rogers" <irogers@google.com>,
        "Adrian Hunter" <adrian.hunter@intel.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] [suggestion] mm/gup: avoid IS_ERR_OR_NULL
Content-Type: text/plain
Precedence: bulk

On Fri, May 19, 2023, at 16:51, Lorenzo Stoakes wrote:
> Given you are sharply criticising the code I authored here, is it too much
> to ask for you to cc- me, the author on commentaries like this? Thanks.

My mistake, I expected this to get added automatically based on
the "Fixes:" tag, I probably dropped you by accident in the end.

> On Fri, May 19, 2023 at 11:39:13AM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> While looking at an unused-variable warning, I noticed a new interface coming
>> in that requires the use of IS_ERR_OR_NULL(), which tends to indicate bad
>> interface design and is usually surprising to users.
>
> I am not sure I understand your reasoning, why does it 'tend to indicate
> bad interface design'? You say that as if it is an obvious truth. Not
> obvious to me at all.
>
> There are 3 possible outcomes from the function - an error, the function
> failing to pin a page, or it succeeding in doing so. For some of the
> callers that results in an error, for others it is not an error.
>
> Overloading EIO on the assumption that gup will never, ever return this
> indicating an error seems to me a worse solution.

The problem is that we have inconsistent error handling in functions
that return an object, about half of them use NULL to indicate an error,
and the other half use ERR_PTR(), and users frequently get those
wrong by picking the wrong one. Functions that can return both make
this worse because whichever of the two normal ways a user expects,
they still get it wrong.

> Not a fan at all of this patch, it doesn't achieve anything useful, is in
> service of some theoretical improvement, and actually introduces a new
> class of bug (differentiating EIO and failing to pin).

Having another -EIO return code is a problem, so I agree that
my patch wouldn't be good either. Maybe separating the error return
from the page pointer by passing a 'struct page **p' argument that
gets filled would help?

    Arnd