Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Qian Cai <cai@lca.pw>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH] mm: fix a data race in put_page()
Date:   Mon, 10 Feb 2020 07:16:36 -0500
Message-Id: <26B88005-28E6-4A09-B3A7-DC982DABE679@lca.pw>
References: <CANpmjNNaHAnKCMLb+Njs3AhEoJT9O6-Yh63fcNcVTjBbNQiEPg@mail.gmail.com>
Cc:     John Hubbard <jhubbard@nvidia.com>, Jan Kara <jack@suse.cz>,
        David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>, ira.weiny@intel.com,
        Dan Williams <dan.j.williams@intel.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        kasan-dev <kasan-dev@googlegroups.com>
In-Reply-To: <CANpmjNNaHAnKCMLb+Njs3AhEoJT9O6-Yh63fcNcVTjBbNQiEPg@mail.gmail.com>
To:     Marco Elver <elver@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Feb 10, 2020, at 2:48 AM, Marco Elver <elver@google.com> wrote:
>=20
> Here is an alternative:
>=20
> Let's say KCSAN gives you this:
>   /* ... Assert that the bits set in mask are not written
> concurrently; they may still be read concurrently.
>     The access that immediately follows is assumed to access those
> bits and safe w.r.t. data races.
>=20
>     For example, this may be used when certain bits of @flags may
> only be modified when holding the appropriate lock,
>     but other bits may still be modified locklessly.
>   ...
>  */
>   #define ASSERT_EXCLUSIVE_BITS(flags, mask)   ....
>=20
> Then we can write page_zonenum as follows:
>=20
> static inline enum zone_type page_zonenum(const struct page *page)
> {
> +       ASSERT_EXCLUSIVE_BITS(page->flags, ZONES_MASK << ZONES_PGSHIFT);
>        return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK;
> }
>=20
> This will accomplish the following:
> 1. The current code is not touched, and we do not have to verify that
> the change is correct without KCSAN.
> 2. We're not introducing a bunch of special macros to read bits in various=
 ways.
> 3. KCSAN will assume that the access is safe, and no data race report
> is generated.
> 4. If somebody modifies ZONES bits concurrently, KCSAN will tell you
> about the race.
> 5. We're documenting the code.
>=20
> Anything I missed?

I don=E2=80=99t know. Having to write the same line twice does not feel me a=
ny better than data_race() with commenting occasionally.=