2:22 a.m.
I should have looked and commented earlier, but I hadn't spotted
that thing until -next conflicts had shown up. As the matter of fact,
I don't have this series in my mailbox - it had been Cc'd my way, apparently,
but it looks like it never made it there, so I'm posting from scratch instead
of replying. Sorry.
The following "primitive" is complete crap
+#ifdef CONFIG_COPY_FROM_ITER_OPS
+size_t copy_from_iter_ops(void *addr, size_t bytes, struct iov_iter *i,
+ int (*user)(void *, const void __user *, unsigned),
+ void (*page)(char *, struct page *, size_t, size_t),
+ void (*copy)(void *, void *, unsigned))
+{
+ char *to = addr;
+
+ if (unlikely(i->type & ITER_PIPE)) {
+ WARN_ON(1);
+ return 0;
+ }
+ iterate_and_advance(i, bytes, v,
+ user((to += v.iov_len) - v.iov_len, v.iov_base,
+ v.iov_len),
+ page((to += v.bv_len) - v.bv_len, v.bv_page, v.bv_offset,
+ v.bv_len),
+ copy((to += v.iov_len) - v.iov_len, v.iov_base, v.iov_len)
+ )
+
+ return bytes;
+}
+EXPORT_SYMBOL_GPL(copy_from_iter_ops);
+#endif
1) Every time we get a new copy-from flavour of iov_iter, you will
need an extra argument and every caller will need to be updated.
2) If it's a general-purpose primitive, it should *not* be
behind a CONFIG_<whatever> to be selected by callers. If it isn't,
it shouldn't be there at all, period. And no, EXPORT_SYMBOL_GPL doesn't
make it any better.
3) The caller makes very little sense. Is that thing meant to
be x86-only? What are the requirements regarding writeback? Is that thing
just go-fast stripes, or...? Basically, all questions asked back in Decemeber
thread (memcpy_nocache()) still apply.
I strongly object to that interface. Let's figure out what's
really needed for your copy_from_iter_pmem() and bloody put the
iterator-related part (without the callbacks, etc.) into lib/iov_iter.c
With memcpy_to_pmem() and pmem_from_user() used by it.
Incidentally, your fallback for memcpy_to_pmem() is... odd.
It used to be "just use memcpy()" and now it's "just do nothing".
What
the hell? If it's really "you should not use that if you don't have
arch-specific variant", let it at least BUG(), if not fail to link.
On the uaccess side, should pmem_from_user() zero what it had failed
to copy? And for !@#!@# sake, comments like this
+ * On x86_64 __copy_from_user_nocache() uses non-temporal stores
+ * for the bulk of the transfer, but we need to manually flush
+ * if the transfer is unaligned. A cached memory copy is used
+ * when destination or size is not naturally aligned. That is:
+ * - Require 8-byte alignment when size is 8 bytes or larger.
+ * - Require 4-byte alignment when size is 4 bytes.
mean only one thing: this should live in arch/x86/lib/usercopy_64.c,
right next to the actual function that does copying. NOT in
drivers/nvdimm/x86.c. At the very least it needs a comment in usercopy_64.c
with dire warnings along the lines of "don't touch that code without
looking into <filename>:pmem_from_user()"...