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@xxxxxxxxxx
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@xxxxxxxxxx>
> Signed-off-by: Axel Rasmussen <axelrasmussen@xxxxxxxxxx>
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?
>
> +#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?
> +#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).
What do you think?
Thanks,
--
Peter Xu
