Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220308141437.144919-1-david@redhat.com> <20220308141437.144919-6-david@redhat.com>
 <D7AB3A84-3FFB-4E8D-9E68-BF721CC4C6FC@vmware.com> <CAHk-=wiYw9hb-EtjcQ2WDSh4TMCWjJmYitfAn=0y_gguhDtf8Q@mail.gmail.com>
 <0FFA6BBC-766F-4ABC-821A-062632632475@vmware.com>
In-Reply-To: <0FFA6BBC-766F-4ABC-821A-062632632475@vmware.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 8 Mar 2022 10:42:22 -0800
Message-ID: <CAHk-=whBkpWpQpzyVxoVbaYBC06eHGMt=7x=fKe6Uae5O0jjcA@mail.gmail.com>
Subject: Re: [PATCH v1 05/15] mm/rmap: convert RMAP flags to a proper distinct
 rmap_t type
To:     Nadav Amit <namit@vmware.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        David Rientjes <rientjes@google.com>,
        Shakeel Butt <shakeelb@google.com>,
        John Hubbard <jhubbard@nvidia.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Mike Rapoport <rppt@linux.ibm.com>,
        Yang Shi <shy828301@gmail.com>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        Vlastimil Babka <vbabka@suse.cz>, Jann Horn <jannh@google.com>,
        Michal Hocko <mhocko@kernel.org>,
        Rik van Riel <riel@surriel.com>,
        Roman Gushchin <guro@fb.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Peter Xu <peterx@redhat.com>,
        Donald Dutile <ddutile@redhat.com>,
        Christoph Hellwig <hch@lst.de>,
        Oleg Nesterov <oleg@redhat.com>, Jan Kara <jack@suse.cz>,
        Liang Zhang <zhangliang5@huawei.com>,
        Pedro Gomes <pedrodemargomes@gmail.com>,
        Oded Gabbay <oded.gabbay@gmail.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 8, 2022 at 10:24 AM Nadav Amit <namit@vmware.com> wrote:
>
> I see your point regarding passing an arg. The or=E2=80=99ing of bitfield=
s
> can easily be resolved, unless I am missing something, with a union
> that holds the aggregate value and an anonymous struct that holds
> the individual flags.

I think that falls under the same heading as passing them as
arguments: it's certainly doable, but it requires special work that is
hidden by helper macros/functions/types.

I mean, even the "pass as arguments" can certainly work. It's not
impossible to hide the odd syntax behind a macro, particularly if you
only ever have a couple of specific cases. So  you can do

  typedef struct rmap_flags {
      unsigned int exclusive:1,
          compound:1;
  } rmap_t;

   #define RMAP_EXCLUSIVE (rmap_t) { .exclusive =3D 1 }
   #define RMAP_COMPOUND (rmap_t) { .compound =3D 1 }

and now you can use RMAP_EXCLUSIVE when you pass it as an argument,
and in the functions themselves you can use

      if (flags.exclusive) {...

which is certainly not unreadable. But it does mean that you basically
have one syntax for testing "is this exclusive" and another for
passing that value in.

And you can't do RMAP_EXCLUSIVE | RMAP_COMPOUND to say "I want to pass
in both exclusive and compound", but you *can* do

      flags.exclusive =3D 1;

to set the exclusive bit. Again - that is certainly not unreadable on
its own, but it's an example of how inconsistent and inconvenient the
interface gets once you do anything outside of some very specific
cases.

And yes, you can solve these cases by simply always limiting yourself
to specific syntax (in particular, just make the rule be that you can
never create values out of thin air, you always have a variable that
gets set.

The bitfield thing does have the advantage that it ends up having very
strict type checking.

But in general, I'd say that the disadvantages are huge enough that
you should never use a bitfield "on its own". Once it's part of a
structure that you have to pass around and initialize as a structure
*anyway*, then most of the problems go away.

So bitfields as part of structures are fine - and we obviously use
them extensively in that form. Even then they can have problems if
there are any kinds of atomicity issues (ie think "page flags" or
anything like that) but that's obviously a different thing, and using
a union to get both ways to access things isn't out of the question.

Of course, if you use unions to get "both as a bitfield and as a
value" things working, you suddenly have "bit order issues", so that
can be really problematic too. Bit endianness doesn't even have to
follow byte endianness.

End result: bitfields are actually often more complex than you think they a=
re.

                  Linus