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;
MIME-Version: 1.0
References: <20200206035235.2537-1-cai@lca.pw> <480a7dde-f678-c07b-2231-4da8e0a38753@nvidia.com>
 <1580997681.7365.14.camel@lca.pw> <CANpmjNNX1apK0izjPhRG3kG-O_iKG1nGrOEL+PAvpH86QLXZMg@mail.gmail.com>
 <423eb3c6-6db2-87d2-e0b7-a32600ee1cd4@nvidia.com>
In-Reply-To: <423eb3c6-6db2-87d2-e0b7-a32600ee1cd4@nvidia.com>
From:   Marco Elver <elver@google.com>
Date:   Fri, 7 Feb 2020 14:18:06 +0100
Message-ID: <CANpmjNM6AFdMU3Oc8z5yJKhAEr7Z8AsOzgvwGgWAQ5j4J=D3xg@mail.gmail.com>
Subject: Re: [PATCH -next] mm: mark a intentional data race in page_zonenum()
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Qian Cai <cai@lca.pw>, Andrew Morton <akpm@linux-foundation.org>,
        ira.weiny@intel.com, dan.j.williams@intel.com, jack@suse.cz,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 7 Feb 2020 at 00:18, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2/6/20 6:35 AM, Marco Elver wrote:
> ...
> >>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
> >>>> index 52269e56c514..cafccad584c2 100644
> >>>> --- a/include/linux/mm.h
> >>>> +++ b/include/linux/mm.h
> >>>> @@ -920,7 +920,7 @@ vm_fault_t finish_mkwrite_fault(struct vm_fault *vmf);
> >>>>
> >>>>   static inline enum zone_type page_zonenum(const struct page *page)
> >>>>   {
> >>>> -   return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK;
> >>>> +   return data_race((page->flags >> ZONES_PGSHIFT) & ZONES_MASK);
> >>>
> >>>
> >>> I don't know about this. Lots of the kernel is written to do this sort
> >>> of thing, and adding a load of "data_race()" everywhere is...well, I'm not
> >>> sure if it's really the best way.  I wonder: could we maybe teach this
> >>> kcsan thing to understand a few of the key idioms, particularly about page
> >>> flags, instead of annotating all over the place?
> >>
> >> My understanding is that it is rather difficult to change the compilers, but it
> >> is a good question and I Cc Marco who is the maintainer for KCSAN that might
> >> give you a definite answer.
> >
> > The problem is that there is no general idiom where we could say with
> > confidence that a data race is safe across the whole kernel. Here it
>
> Yes. I'm grasping at straws now, but...what about the idiom that page_zonenum()
> uses: a set of bits that are "always" (after a certain early point) read-only?
> What are your thoughts on that?

I have replied to the other thread.

Thanks,
-- Marco


> > might not matter, but somewhere else it might matter a lot.
> >
> > If you think that it turns out the entire file may be littered with
> > 'data_race()', and you do not want to use annotations, you can
> > blacklist the file. I already had to do this for other files in mm/,
> > because concurrent flag modification/checking is pervasive and a lot
> > of them seem 'benign'. We decided to revisit those files later.
> >
> > Feel free to add 'KCSAN_SANITIZE_memory.o := n' or whatever other
> > files you think are full of these to mm/Makefile.
> >
> > The only problem I see with that is that it's not obvious what is
> > concurrently modified and what isn't. The annotations would have
> > helped document what is happening.
> >
> > Thanks,
> > -- Marco
> >
>
>
> thanks,
> --
> John Hubbard
> NVIDIA