Re: [PATCH v2 0/2] Replace and improve "mcsafe" with copy_safe()

[Dan Williams]

On Thu, Apr 30, 2020 at 12:56 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Apr 30, 2020 at 12:23 PM Luck, Tony <tony.luck@xxxxxxxxx> 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.

You had me until here. Up to this point I was grokking that Andy's
"_fallible" suggestion does help explain better than "_safe", because
the copy is doing extra safety checks. copy_to_user() and
copy_to_user_fallible() mean *something* where copy_to_user_safe()
does not.

However you lose me on this "broken nvdimm semantics" contention.
There is nothing nvdimm-hardware specific about the copy_safe()
implementation, zero, nada, nothing new to the error model that DRAM
did not also inflict on the Linux implementation.

This is about Linux treating memory as a disk and performing bulk
transfers with the CPU instead of a DMA engine. Whereas existing
memory error handling has a high chance for it to trigger on userspace
accesses, large copies in kernel mode now have a higher chance of
tripping over the same errors in kernel space. Since there is an error
model overlaid on top of the block-I/O, software is well prepared to
handle the short transfer case.

> 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.

True, copy is not a fundamentally well defined operation. It's
unfortunate that the low-level of a direct-I/O read(2) eventually
turns into a call to memcpy() inside a typical ramdisk driver. So
you're right, this isn't a copy routine as much as it's the backend of
read(2)/write(2) with short transfer semantics, but by the time it
gets to pmem driver it's been disconnected from its original intent
and all it sees is "move bytes from here to there if you can".

> 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.

The CPU overhead of synchronous bulk block-I/O transfers with the CPU
is already painful, a get_user() loop adds to that without a benefit
that I can currently perceive. The vast majority of driver actions and
DAX operations are in PAGE_SIZE units.