All of lore.kernel.org
 help / color / mirror / Atom feed
* [NAK] copy_from_iter_ops()
@ 2017-04-25  1:22 Al Viro
  2017-04-25  2:35 ` Dan Williams
  2017-04-26 21:56   ` Dan Williams
  0 siblings, 2 replies; 53+ messages in thread
From: Al Viro @ 2017-04-25  1:22 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jan Kara, Matthew Wilcox, linux-nvdimm, Linus Torvalds,
	Christoph Hellwig

	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()"...
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 53+ messages in thread

end of thread, other threads:[~2017-05-08 20:40 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-25  1:22 [NAK] copy_from_iter_ops() Al Viro
2017-04-25  2:35 ` Dan Williams
2017-04-26 21:56 ` [RFC PATCH] x86, uaccess, pmem: introduce copy_from_iter_writethru for dax + pmem Dan Williams
2017-04-26 21:56   ` Dan Williams
2017-04-26 21:56   ` Dan Williams
2017-04-27  6:30   ` Ingo Molnar
2017-04-27  6:30     ` Ingo Molnar
2017-04-27  6:30     ` Ingo Molnar
2017-04-28 19:39     ` [PATCH v2] x86, uaccess: introduce copy_from_iter_wt for pmem / writethrough operations Dan Williams
2017-04-28 19:39       ` Dan Williams
2017-04-28 19:39       ` Dan Williams
2017-05-05  6:54       ` Ingo Molnar
2017-05-05  6:54         ` Ingo Molnar
2017-05-05  6:54         ` Ingo Molnar
2017-05-05 14:12         ` Dan Williams
2017-05-05 14:12           ` Dan Williams
2017-05-05 14:12           ` Dan Williams
2017-05-05 20:39       ` Kani, Toshimitsu
2017-05-05 20:39         ` Kani, Toshimitsu
2017-05-05 20:39         ` Kani, Toshimitsu
2017-05-05 20:39         ` Kani, Toshimitsu
2017-05-05 22:25         ` Dan Williams
2017-05-05 22:25           ` Dan Williams
2017-05-05 22:25           ` Dan Williams
2017-05-05 22:44           ` Kani, Toshimitsu
2017-05-05 22:44             ` Kani, Toshimitsu
2017-05-05 22:44             ` Kani, Toshimitsu
2017-05-05 22:44             ` Kani, Toshimitsu
2017-05-06  2:15             ` Dan Williams
2017-05-06  2:15               ` Dan Williams
2017-05-06  2:15               ` Dan Williams
2017-05-06  3:17               ` Kani, Toshimitsu
2017-05-06  3:17                 ` Kani, Toshimitsu
2017-05-06  3:17                 ` Kani, Toshimitsu
2017-05-06  3:17                 ` Kani, Toshimitsu
2017-05-06  9:46               ` Ingo Molnar
2017-05-06  9:46                 ` Ingo Molnar
2017-05-06  9:46                 ` Ingo Molnar
2017-05-06 13:57                 ` Dan Williams
2017-05-06 13:57                   ` Dan Williams
2017-05-06 13:57                   ` Dan Williams
2017-05-07  8:57                   ` Ingo Molnar
2017-05-07  8:57                     ` Ingo Molnar
2017-05-07  8:57                     ` Ingo Molnar
2017-05-08  3:01                     ` Kani, Toshimitsu
2017-05-08  3:01                       ` Kani, Toshimitsu
2017-05-08  3:01                       ` Kani, Toshimitsu
2017-05-08 20:32       ` Ross Zwisler
2017-05-08 20:32         ` Ross Zwisler
2017-05-08 20:32         ` Ross Zwisler
2017-05-08 20:40         ` Dan Williams
2017-05-08 20:40           ` Dan Williams
2017-05-08 20:40           ` Dan Williams

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.