Re: [PATCH v9 10/42] x86/mm: Introduce _PAGE_SAVED_DIRTY

[Edgecombe, Rick P]

On Tue, 2023-06-13 at 10:58 -0700, Linus Torvalds wrote:
> On Mon, Jun 12, 2023 at 5:14 PM Rick Edgecombe
> <rick.p.edgecombe@xxxxxxxxx> wrote:
> > 
> > +static inline unsigned long mksaveddirty_shift(unsigned long v)
> > +{
> > +       unsigned long cond = !(v & (1 << _PAGE_BIT_RW));
> > +
> > +       v |= ((v >> _PAGE_BIT_DIRTY) & cond) <<
> > _PAGE_BIT_SAVED_DIRTY;
> > +       v &= ~(cond << _PAGE_BIT_DIRTY);
> 
> I assume you checked that the compiler does the right thing here?
> 
> Because the above is kind of an odd way to do things, I feel.
> 
> You use boolean operators and then work with an "unsigned long" and
> then shift things by hand. So you're kind of mixing two different
> mental models.
> 
> To me, it would be more natural to do that 'cond' calculation as
> 
>         unsigned long cond = (~v >> _PAGE_BIT_RW) & 1;
> 
> and keep everything in the "bitops" domain.

That makes sense. It lets the reader's brain stay in bitmath mode.

> 
> I suspect - and hope - that the compiler is smart enough to turn that
> boolean test into just the shift, but if that's the intent, why not
> just write it with that in mind and not have that "both ways" model?

Well, it wasn't for this reason, but gcc likes to emit two more
instructions for the boolean-less version. Clang generates identical
code. If it makes this complicated code any simpler to read, it's
probably still worth it.