2022-03-08 19:48:45

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH v1 05/15] mm/rmap: convert RMAP flags to a proper distinct rmap_t type

On Tue, Mar 8, 2022 at 9:15 AM Nadav Amit <[email protected]> wrote:
>
> It would be much easier to read. The last time I made such a suggestion,
> Ingo said "I personally like bitfields in theory … [but] older versions of
> GCC did a really poor job of optimizing them.”

Yeah, even not that old versions had serious issues, iirc.

Bitfields can look nice, but they have some _serious_ syntax issues.
In particular, they are nice when you want to *test* one single field
(ie bit in this case), but basically atrociously bad in almost all
other circumstances.

For example, passing a bitfield aggregate as an argument is just
crazy. Oh, you can do it, with syntax like

(struct type) { .field1 = 1, .field3 = 1 }

as the argument but when you say "much easier to read" I laugh in your
face and call your mother a hamster.

And that's ignoring all the issues when you want to combine two
bitfields. You can't do it. There is nothing like the binary "or"
operator. Again, it's easy to modify *one* field, but taking two
bitfields and merging them? Not going to happen.

So no. Bitfields have their place, but they are close to useless as
"flags" type things that get passed around as arguments, unless you
have very very specific and limited use.

Linus


2022-03-09 00:34:59

by Nadav Amit

[permalink] [raw]
Subject: Re: [PATCH v1 05/15] mm/rmap: convert RMAP flags to a proper distinct rmap_t type



> On Mar 8, 2022, at 10:09 AM, Linus Torvalds <[email protected]> wrote:
>
> On Tue, Mar 8, 2022 at 9:15 AM Nadav Amit <[email protected]> wrote:
>>
>> It would be much easier to read. The last time I made such a suggestion,
>> Ingo said "I personally like bitfields in theory … [but] older versions of
>> GCC did a really poor job of optimizing them.”
>
> Yeah, even not that old versions had serious issues, iirc.
>
> Bitfields can look nice, but they have some _serious_ syntax issues.
> In particular, they are nice when you want to *test* one single field
> (ie bit in this case), but basically atrociously bad in almost all
> other circumstances.
>
> For example, passing a bitfield aggregate as an argument is just
> crazy. Oh, you can do it, with syntax like
>
> (struct type) { .field1 = 1, .field3 = 1 }
>
> as the argument but when you say "much easier to read" I laugh in your
> face and call your mother a hamster.
>
> And that's ignoring all the issues when you want to combine two
> bitfields. You can't do it. There is nothing like the binary "or"
> operator. Again, it's easy to modify *one* field, but taking two
> bitfields and merging them? Not going to happen.
>
> So no. Bitfields have their place, but they are close to useless as
> "flags" type things that get passed around as arguments, unless you
> have very very specific and limited use.

I see your point regarding passing an arg. The or’ing of bitfields
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.

At the time, I thought that bitfields are much better fit for cpuid
fields (which are not just flags).

Anyhow, I will refrain from using bitfields for flags, if only for
the sake of my mother. :)

2022-03-09 01:18:47

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH v1 05/15] mm/rmap: convert RMAP flags to a proper distinct rmap_t type

On Tue, Mar 8, 2022 at 10:24 AM Nadav Amit <[email protected]> wrote:
>
> I see your point regarding passing an arg. The or’ing of bitfields
> 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 = 1 }
#define RMAP_COMPOUND (rmap_t) { .compound = 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 = 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 are.

Linus