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

[Linus Torvalds]

Small nit.

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.

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?

> +static inline unsigned long clear_saveddirty_shift(unsigned long v)
> +{
> +       unsigned long cond = !!(v & (1 << _PAGE_BIT_RW));

Same comment here.

             Linus