Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 15:33:48 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Axel Rasmussen <axelrasmussen@google.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        David Hildenbrand <david@redhat.com>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>,
        Leonardo Bras Soares Passos <lsoaresp@redhat.com>
Subject: Re: [PATCH v2 14/31] selftests/mm: uffd_[un]register()
Message-ID: <ZDcHnEgCsoDBhFYM@x1n>
References: <20230412163922.327282-1-peterx@redhat.com>
 <20230412164247.328293-1-peterx@redhat.com>
 <CAJHvVchBhRf3pP4fCwsrr2s3S5jQP8z4sct5-_0n=tRd4XB9jQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAJHvVchBhRf3pP4fCwsrr2s3S5jQP8z4sct5-_0n=tRd4XB9jQ@mail.gmail.com>
Precedence: bulk

On Wed, Apr 12, 2023 at 11:20:35AM -0700, Axel Rasmussen wrote:
> > -       uffdio_register.range.start = (unsigned long)addr;
> > -       uffdio_register.range.len = len;
> > -       uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > -       if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > +       if (uffd_register(uffd, addr, len, true, false, false)) {
> 
> For what it's worth, I agree with Mike that the booleans here are hard
> to read. It's not clear what "true, false, false" means without going
> to look at the header file, whereas "uffd_register(uffd, addr, len,
> MINOR)" would be immediately clear.
> 
> One solution I've seen outside the kernel is to comment the bools, like:
> 
> uffd_register(..., /*minor=*/true, /*wp=*/false, /*minor=*/false);
> 
> But, then I feel we lose most of the benefit we wanted from switching
> to bools anyway (code length). :)
> 
> I do agree the macro names are unwieldy, and lots of tests already use
> booleans so we'd have to convert from bool -> flag. If it were me, I
> would resolve that by:
> 
> - Define much shorter aliases for these macros in uffd-common.h
> - Consider refactoring callers to not use bools either.
> 
> Then again, I also agree with Mike that it's not a deal breaker, if
> you like this way much more than that alternative we can leave it.

No strong feeling here either.  I kept it just to avoid code churns and
rebases.

I'll see whether I'll need a repost, if so I can go back to use modes.

Thanks,

-- 
Peter Xu