Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <158823509800.2094061.9683997333958344535.stgit@dwillia2-desk3.amr.corp.intel.com>
 <CAHk-=wh6d59KAG_6t+NrCLBz-i0OUSJrqurric=m0ZG850Ddkw@mail.gmail.com>
 <CALCETrVP5k25yCfknEPJm=XX0or4o2b2mnzmevnVHGNLNOXJ2g@mail.gmail.com>
 <CAHk-=widQfxhWMUN3bGxM_zg3az0fRKYvFoP8bEhqsCtaEDVAA@mail.gmail.com>
 <CALCETrVq11YVqGZH7J6A=tkHB1AZUWXnKwAfPUQ-m9qXjWfZtg@mail.gmail.com> <20200430192258.GA24749@agluck-desk2.amr.corp.intel.com>
In-Reply-To: <20200430192258.GA24749@agluck-desk2.amr.corp.intel.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 30 Apr 2020 12:50:40 -0700
Message-ID: <CAHk-=wg0Sza8uzQHzJbdt7FFc7bRK+o1BB=VBUGrQEvVv6+23w@mail.gmail.com>
Subject: Re: [PATCH v2 0/2] Replace and improve "mcsafe" with copy_safe()
To:     "Luck, Tony" <tony.luck@intel.com>
Cc:     Andy Lutomirski <luto@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        stable <stable@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Paul Mackerras <paulus@samba.org>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Erwin Tsaur <erwin.tsaur@intel.com>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        linux-nvdimm <linux-nvdimm@lists.01.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Apr 30, 2020 at 12:23 PM Luck, Tony <tony.luck@intel.com> wrote:
>
> How about
>
>         try_copy_catch(void *dst, void *src, size_t count, int *fault)
>
> returns number of bytes not-copied (like copy_to_user etc).
>
> if return is not zero, "fault" tells you what type of fault
> cause the early stop (#PF, #MC).

That just makes things worse.

The problem isn't "what fault did I get?".

The problem is "what is the point of this function?".

In other words, I want the code to explain _why_ it uses a particular function.

So the question the function should answer is not "Why did it take a
fault?", but "Why isn't this just a 'memcpy()'"?

When somebody writes

    x = copy_to_user(a,b,c);

the "why is it not a memcpy" question never comes up, because the code
is obvious. It's not a memory copy, because it's copying to user
space, and user space is different!

In contrast, if you write

    x = copy_safe(a,b,c);

then what is going on? There is no rhyme or reason to the call. Is the
source unsafe? Wny? Is the destination faulting? Why? Both? How?

So "copy_to_user()" _answers_ a question. But "copy_safe()" just
results in more questions. See the difference?

And notice that the "why did it fault" question is NOT about your
"what kind of fault did it take" question. That question is generally
pretty much uninteresting.

The question I want answered is "why is this function being called AT ALL".

Again, "copy_to_user()" can fail, and we know to check failure cases.
But more importantly, the _reason_ it can fail is obvious from the
name and from the use. There's no confusion about "why".

"copy_safe()"? or "try_copy_catch()"? No such thing. It doesn't answer
that fundamental "why" question.

And yes, this also has practical consequences. If you know that the
failure is due to the source being some special memory (and if we care
about the MC case with unaligned accesses), then the function in
question should probably strive to make those _source_ accesses be the
aligned ones.  But if it's the destination that is special memory,
then it's the writes to the destination that should be aligned. If you
need both, you may need to be either mutually aligned, or do byte
accesses, or do special shifting copies. So it matters for any initial
alignment code (if the thing has alignment issues to begin with).

I haven't even gotten an answer to the "why would the write fail".
When I asked, Dan said

 "writes can mmu-fault now that memcpy_mcsafe() is also used by
_copy_to_iter_mcsafe()"

but as mentioned, the only reason I can find for _that_ is that the
destination is user space.

In which case a "copy_safe()" absolutely could never work.

If I can't figure out the "why is this function used" or even figure
out why it needs the semantics it claims it needs, then there's a
problem.

Personally, I suspect that the *easiest* way to support the broken
nvdimm semantics is to not have a "copy" function at all as the basic
accessor model.

Why? Exactly because "copy" is not a fundamental well-defined action.
You get nasty combinatorial explosions of different things, where you
have three different kinds of sources (user, RAM, nvdimm) and three
different kinds of destinations.

And that's ignoring the whole "maybe you don't want to do a plain
copy, maybe you want to calculate a RAID checksum, or do a 'strcpy()'
or whatever". If those are ever issues, you get another explosion of
combinations.

The only *fundamental* access would likely be a single read/write
operation, not a copy operation. Think "get_user()" instead of
"copy_from_user()".  Even there you get combinatorial explosions with
access sizes, but you can often generate those automatically or with
simple patterns, and then you can build up the copy functions from
that if you really need to.

                   Linus