On Mon, Mar 6, 2023 at 5:00 PM Peter Xu peterx@redhat.com wrote:
On Mon, Mar 06, 2023 at 02:50:22PM -0800, Axel Rasmussen wrote:
Many userfaultfd ioctl functions take both a 'mode' and a 'wp_copy' argument. In future commits we plan to plumb the flags through to more places, so we'd be proliferating the very long argument list even further.
Let's take the time to simplify the argument list. Combine the two arguments into one - and generalize, so when we add more flags in the future, it doesn't imply more function arguments.
Since the modes (copy, zeropage, continue) are mutually exclusive, store them as an integer value (0, 1, 2) in the low bits. Place combine-able flag bits in the high bits.
This is quite similar to an earlier patch proposed by Nadav Amit ("userfaultfd: introduce uffd_flags" - for some reason Lore no longer has a copy of the patch). The main difference is that patch only handled
Lore has. :)
https://lore.kernel.org/all/20220619233449.181323-2-namit@vmware.com
And btw sorry to review late.
flags, whereas this patch *also* combines the "mode" argument into the same type to shorten the argument list.
Acked-by: James Houghton jthoughton@google.com Signed-off-by: Axel Rasmussen axelrasmussen@google.com
Mostly good to me, a few nitpicks below.
[...]
+/* A combined operation mode + behavior flags. */ +typedef unsigned int __bitwise uffd_flags_t;
+/* Mutually exclusive modes of operation. */ +enum mfill_atomic_mode {
MFILL_ATOMIC_COPY = (__force uffd_flags_t) 0,MFILL_ATOMIC_ZEROPAGE = (__force uffd_flags_t) 1,MFILL_ATOMIC_CONTINUE = (__force uffd_flags_t) 2,NR_MFILL_ATOMIC_MODES,};
I never used enum like this. I had a feeling that this will enforce setting the enum entries but would the enforce applied to later assignments? I'm not sure.
I had a quick test and actually I found sparse already complains about calculating the last enum entry:
---8<--- $ cat a.c typedef unsigned int __attribute__((bitwise)) flags_t;
enum { FLAG1 = (__attribute__((force)) flags_t) 0, FLAG_NUM, };
void main(void) { uffd_flags_t flags = FLAG1; } $ sparse a.c a.c:5:5: error: can't increment the last enum member ---8<---
Maybe just use the simple "#define"s?
Agreed, if sparse isn't happy with this then using the force macros is pointless.
The enum is valuable because it lets us get the # of modes; assuming we agree that's useful below ...
+#define MFILL_ATOMIC_MODE_BITS (const_ilog2(NR_MFILL_ATOMIC_MODES - 1) + 1)
Here IIUC it should be "const_ilog2(NR_MFILL_ATOMIC_MODES) + 1", but maybe.. we don't bother and define every bit explicitly?
If my reading of const_ilog2's definition is correct, then:
const_ilog2(4) = 2 const_ilog2(3) = 1 const_ilog2(2) = 1
For either 3 or 4 modes, we need 2 bits to represent them (0, 1, 2, 3), i.e. we want MFILL_ATOMIC_MODE_BITS = 2. I think this is correct as is, because const_ilog2(4 - 1) + 1 = 2, and const_ilog2(3 - 1) + 1 = 2.
In other words, I think const_ilog2 is defined as floor(log2()), whereas what we want is ceil(log2()).
The benefit of doing this vs. just doing defines with fixed values is, if we ever added a new mode, we wouldn't have to do bit twiddling and update the mask, flag bits, etc. - it would happen "automatically". I prefer it this way, but I agree it is a matter of opinion / taste. :) If you or others feel strongly this is overcomplicated, I can take the other approach.
+#define MFILL_ATOMIC_BIT(nr) ((__force uffd_flags_t) BIT(MFILL_ATOMIC_MODE_BITS + (nr))) +#define MFILL_ATOMIC_MODE_MASK (MFILL_ATOMIC_BIT(0) - 1)
+/* Flags controlling behavior. */ +#define MFILL_ATOMIC_WP MFILL_ATOMIC_BIT(0)
[...]
@@ -312,9 +312,9 @@ static __always_inline ssize_t mfill_atomic_hugetlb( unsigned long dst_start, unsigned long src_start, unsigned long len,
enum mcopy_atomic_mode mode,bool wp_copy)
uffd_flags_t flags){
int mode = flags & MFILL_ATOMIC_MODE_MASK; struct mm_struct *dst_mm = dst_vma->vm_mm; int vm_shared = dst_vma->vm_flags & VM_SHARED; ssize_t err;@@ -333,7 +333,7 @@ static __always_inline ssize_t mfill_atomic_hugetlb( * by THP. Since we can not reliably insert a zero page, this * feature is not supported. */
if (mode == MCOPY_ATOMIC_ZEROPAGE) {
if (mode == MFILL_ATOMIC_ZEROPAGE) {The mode comes from "& MFILL_ATOMIC_MODE_MASK" but it doesn't quickly tell whether there's a shift for the mask.
Would it look better we just have a helper to fetch the mode? The function tells that whatever it returns must be the mode:
if (uffd_flags_get_mode(flags) == MFILL_ATOMIC_ZEROPAGE)We also avoid quite a few "mode" variables. All the rest bits will be fine to use "flags & FLAG1" if it's a boolean (so only this "mode" is slightly tricky).
Agreed, this is simpler. I'll make this change.
What do you think?
Thanks,
-- Peter Xu