Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH resend] mm/gup: fix try_grab_compound_head() race with
 split_huge_page()
To:     Jann Horn <jannh@google.com>
CC:     Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        kernel list <linux-kernel@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Jan Kara <jack@suse.cz>, stable <stable@vger.kernel.org>
References: <20210611161545.998858-1-jannh@google.com>
 <20210611153624.65badf761078f86f76365ab9@linux-foundation.org>
 <CAG48ez3mjsf41Z2vCjmkuBaHe9XgoXbWDGvM4OJdUjqiCQbN4A@mail.gmail.com>
 <9041f85d-515d-576d-21a9-6f10b6e1279e@nvidia.com>
 <CAG48ez3Vb1BxaZ0EHhR9ctkjCCygMWOQqFMGqt-=Ea2yXrvKiw@mail.gmail.com>
From:   John Hubbard <jhubbard@nvidia.com>
Message-ID: <ad8601a0-f520-9986-1a6c-2852e2e1d3c2@nvidia.com>
Date:   Mon, 14 Jun 2021 17:38:18 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <CAG48ez3Vb1BxaZ0EHhR9ctkjCCygMWOQqFMGqt-=Ea2yXrvKiw@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jun 2021 00:38:19.7560
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 22ef54fe-f7b2-4202-188b-08d92f95dfa9
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT053.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR12MB1320
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 6/13/21 9:47 PM, Jann Horn wrote:
...
>> The VM_WARN_ON_ONCE_PAGE() is not implemented exactly right
>> in the !CONFIG_DEBUG_VM case. IMHO it should follow the WARN*()
>> behavior, and return the original condition and keep going
>> in that case.
> 
> But the point of the existing definition is that the compiler can
> avoid generating code for the condition in !DEBUG_VM builds, even if
> it can't prove that the condition is free of side effects, right? If
> VM_WARN_ON_ONCE_PAGE() was changed as you propose, then I think that
> in mem_cgroup_page_lruvec(), the compiler would have to generate code
> for mem_cgroup_disabled(), which calls static_branch_likely(), which
> ends up in "asm volatile" statements; so the compiler probably won't
> be able to eliminate the condition.
> 
>> Then you could use it directly here.
> 
> Depending on whether the intended behavior here is to skip the check
> in !DEBUG_VM builds (which was the case before) or also perform the
> check in DEBUG_VM builds. And if DEBUG_VM is a config option because
> it might have some performance impact, isn't the cost of the check
> probably quite large compared to the cost of printing the warning on a
> codpath that should never execute?
> 

That's true for these VM_WARN*() macros, but not true for the more widely
used WARN*() macros. And I was hoping to bring VM macros closer to the
WARN macros. But as you point out, pre-existing callers expect to have
zero impact in !DEBUG_VM builds, and so some caution is required.

I feel like a separate set of macros would be reasonable. Something that
has WARN*() type of behavior, and accepts a struct page (which typically
means that WARN_ON_ONCE is required, because for pages you have to limit
it to that pretty much always).

thanks,
-- 
John Hubbard
NVIDIA