linux-kernel - Re: [PATCH v9 10/42] x86/mm: Introduce _PAGE_SAVED

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Jun 2023 19:37:50 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "Torvalds, Linus" <torvalds@...ux-foundation.org>
CC:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "Xu, Pengfei" <pengfei.xu@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "david@...hat.com" <david@...hat.com>,
        "Schimpe, Christina" <christina.schimpe@...el.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "dethoma@...rosoft.com" <dethoma@...rosoft.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "pavel@....cz" <pavel@....cz>, "bp@...en8.de" <bp@...en8.de>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "john.allen@....com" <john.allen@....com>,
        "arnd@...db.de" <arnd@...db.de>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "bsingharora@...il.com" <bsingharora@...il.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "oleg@...hat.com" <oleg@...hat.com>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>,
        "Yu, Yu-cheng" <yu-cheng.yu@...el.com>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "szabolcs.nagy@....com" <szabolcs.nagy@....com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "debug@...osinc.com" <debug@...osinc.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "Eranian, Stephane" <eranian@...gle.com>
Subject: Re: [PATCH v9 10/42] x86/mm: Introduce _PAGE_SAVED_DIRTY

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@...el.com> 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.