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;
Message-ID: <d56f9851-43be-91dc-1d45-f70eb1169209@nvidia.com>
Date:   Mon, 1 Nov 2021 12:42:44 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [RFC 3/8] mm: Avoid using set_page_count() in
 set_page_recounted()
Content-Language: en-US
To:     Pasha Tatashin <pasha.tatashin@soleen.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, linux-mm <linux-mm@kvack.org>,
        linux-m68k@lists.linux-m68k.org,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        william.kucharski@oracle.com,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        schmitzmic@gmail.com, Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Roman Gushchin <guro@fb.com>,
        Muchun Song <songmuchun@bytedance.com>, weixugc@google.com,
        Greg Thelen <gthelen@google.com>
References: <20211026173822.502506-1-pasha.tatashin@soleen.com>
 <20211026173822.502506-4-pasha.tatashin@soleen.com>
 <7b131cb1-68d8-6746-f9c1-2b01d4838869@nvidia.com>
 <CA+CK2bD6x01PevPqshzYqkO3aokjP2jBbt_4e5H5U3DVEdcJ5Q@mail.gmail.com>
 <b346cafd-d8b8-57a4-c7b9-6574b256a400@nvidia.com>
 <CA+CK2bBiomTe-vOuxM_R+0CMAippyrfZ6AgpXQGqps3ZFQCtRA@mail.gmail.com>
 <19d16b40-355f-3f79-dcba-e1d8d2216d33@nvidia.com>
 <CA+CK2bBn81pz5NqCvS9jz+DvXbGG6d52Q=xTySJvJuqNRmFkkg@mail.gmail.com>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <CA+CK2bBn81pz5NqCvS9jz+DvXbGG6d52Q=xTySJvJuqNRmFkkg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?L3RTNTdNbnpTR0Vhb2FWelJPOWNvVktaUllFemZtUU5teDFVQ3k5ZXQzTC80?=
 =?utf-8?B?eldIQ1Vja2lUUEl3QTZxK0dMUkR0a1RrMjhLTzVvTlFVOEdYWlJnQzhwOHdO?=
 =?utf-8?B?TUx5bnBjRytucVlYVWltdWVzZU9HOTloUTRvNnNtUmROQUlNbXBzTENLbndJ?=
 =?utf-8?B?Qkp0bzdqaFBZZE5EV2lYK2xvUTBTY2tjWkp5b0I0K05VVHVaUjladEVsQXR4?=
 =?utf-8?B?M01TbnVrM2Eyb0FROTVDaWp1N21iTjVyNHlmazZYbk1VZ3ZNbStFSWt3cmJz?=
 =?utf-8?B?ZGVBV091bU0vS1lvWmU2RFF4SFlOMjZoaUFjV0FsYllIZ3NHZ3JjNFF0Umpr?=
 =?utf-8?B?aVZVU1V3bW1hOFVyRTdzWGtnQWhNaEJYVTZ2VEdmV2xyM1A2Ni9ZcU5KRWF5?=
 =?utf-8?B?b1llUm45NHhVYW5TU3RpSmdUWkR2d0RuWXdGem96VHYrdDN0RU5FNFVDL2Ja?=
 =?utf-8?B?Zy82R1ZrOU1Tczg2Tzd0MzZPaSthQTVTNFFVQXdYNzQ1V3MzTkFlQlpZcEdW?=
 =?utf-8?B?ZHVVamlDa3ZIem83TmlESmNHM2x1ZFFmWHoycFNSZlk4TERKeS80ZjloYmVC?=
 =?utf-8?B?VHhTNm9tTDV3RWI3NjZEUzRULzNBekw2UjB2QWRlQnJMbmh4am45aWpWNm9V?=
 =?utf-8?B?SzdTSlQ4Y09xdm96NUF0akxCYVo0aUIyVzJqbUNCUjloTXlvelR0YytwNHlm?=
 =?utf-8?B?ZmhxTTA1MkFyZG5GUDZBajl5S2oyVVhYdWNwZ2l0b1NxL1NTeS9zaUZkaXh2?=
 =?utf-8?B?dG1ENklKUm95YTl0ZW1PN3hvbU5lZkNLQlpsYXZrUFd3cTArQXVKeHkvdEJS?=
 =?utf-8?B?L0R4UmtKNXF0MG4vaTlidzd4ZjFpSzdqR2NNcnFJazBGTnlnajNxY2gzTVpt?=
 =?utf-8?B?Z29zSFBUZTczTWUvaHV1ZzNWZ2JWMndlb2YyMFJKVmp6YXkvWlFERURkbkZJ?=
 =?utf-8?B?eDNzcFZ6UXJOQWtFWXlXZjdxTkFiMmRucnJpNHFiZGZGOWhVcGw2cGpjVi9h?=
 =?utf-8?B?VnJRRFU0U3BGc3FBeklCRzFmK2JMOTlVR3BDTFNINnZRK1hnbUJIRGN2VHZr?=
 =?utf-8?B?cTFkNnJsYTVyK3ZTOEZ3cktDa0NZYld4RUFrb3RUZjIzQmp1NkxUaGZJcjlO?=
 =?utf-8?B?OElrTWRQMk14M2E3Qy9DN0taTCtHNGRRRkh1OURMVjZybW5PY1NDa3Q4REM3?=
 =?utf-8?B?ck92RjRnS2pOYldzdElPblEvdWR1ajFNUm4wOE5mS1RFWU1GREFFRjF0TlhK?=
 =?utf-8?B?M0ZwRmlDbVZnemExaTVCZFJPZkVkQUhVQmxubUxZNWg1bUNPdlRNRmliQ00r?=
 =?utf-8?B?NFZRUlBYMXR4V0JCMXYxaDJpTmlXNklBN0ZWUWhadmFOWURJeXNVYzhsM3hn?=
 =?utf-8?B?N2tSVGZaV0ZEYy9Xa09relp1OVZ6TmlQSzdITFpqYlZETEo2WkY2NHVKS3pF?=
 =?utf-8?B?azRVVHRKaDliSVpQMW9TUHhKNldxL2I3MkF2MXVnZkZQS0x0TkFRa1JFZHpz?=
 =?utf-8?B?WDVtSlpISnl2NWg5ajIwNG1sWVVJbm40VHFDeU1TOFVmZk5DSDl3bzlwc2cv?=
 =?utf-8?B?KzVrM0FrUVFIRDlqZDI1WGJLUERRa2VVUEVJSXMrN0trbzhWeUdXNEc4a2Fu?=
 =?utf-8?B?QStvZnZMZHJGL0thdjJobld2RjlSb1hWWThySFh6NFBVQkZSTUM3WEcrSktM?=
 =?utf-8?B?Vnh1Q1FmYXViN3RuOUV1anlUaUwwc0lERy8raXQ5S3YzdmQ1dVdwdnpXcUha?=
 =?utf-8?B?cStQWVFFSzc2VUFCY2JxZjc5V2NLL2pxcEVvQnpHU1JqKy91bGovQ1ZUNnJw?=
 =?utf-8?B?NmdIMkF4TlRybkZGakQ5MGNFRUhlc2VuMkJmRWtOMFB2Z3M0R3VpdWNtOTZI?=
 =?utf-8?B?bC9tL0k0cDU0STIvUmY4QWh1K2M4bWIxcFFJRXVmTWVxcFV3aVJoUGZKdzNp?=
 =?utf-8?B?OG9jNkl6ZlhYRlhqM05VbDFBTTlnbFVTalQ1ZVUvRGxwUnBmcmQ4OE1BR2JU?=
 =?utf-8?B?a0lXZENCcWlCQ0lZL3ZQSWlGWGpocXNUbHRQVXhIZUZEV1RIalZybXZ2azZ4?=
 =?utf-8?B?TmpGcnY2Vk5lNEl5SGZ5L2lYWUlTNjcreUNYc1FPNXBIelpqL2FEVDNDdlNS?=
 =?utf-8?B?TjFWd2hRbGQyVy9IOExTTnh0dEE4bFpoUHFjWFRHSGptVmY1SHg0V3JQckh3?=
 =?utf-8?Q?4GlQEHM5S0aZABQIOrsKZV8=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 83c1fcfb-f097-48af-635d-08d99d6fc762
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Nov 2021 19:42:46.3721
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: v9zoLKMTdfkN+L4xZX+ab7RBKUH1+cxdT+YjzqgeLxIi2eC08J0pqPNE2G6NG6jdGiYd+gR8iFfslH45d853vA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3288
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/1/21 07:22, Pasha Tatashin wrote:
>>>> Yes, you are just repeating what the diffs say.
>>>>
>>>> But it's still not good to have this function name doing something completely
>>>> different than its name indicates.
>>>
>>> I see, I can rename it to: 'set_page_recounted/get_page_recounted' ?
>>>
>>
>> What? No, that's not where I was going at all. The function is already
>> named set_page_refcounted(), and one of the problems I see is that your
>> changes turn it into something that most certainly does not
>> set_page_refounted(). Instead, this patch *increments* the refcount.
>> That is not the same thing.
>>
>> And then it uses a .config-sensitive assertion to "prevent" problems.
>> And by that I mean, the wording throughout this series seems to equate
>> VM_BUG_ON_PAGE() assertions with real assertions. They are only active,
>> however, in CONFIG_DEBUG_VM configurations, and provide no protection at
>> all for normal (most distros) users. That's something that the wording,
>> comments, and even design should be tweaked to account for.
> 
> VM_BUG_ON and BUG_ON should be treated the same. Yes, they are config
> sensitive, but in both cases *BUG_ON() means that there is an
> unrecoverable problem that occured. The only difference between the
> two is that VM_BUG_ON() is not enabled when distros decide to reduce
> the size of their kernel and improve runtime performance by skipping
> some extra checking.
> 
> There is no logical separation between VM_BUG_ON and BUG_ON, there is
> been a lengthy discussion about this:

Actually I do want to mention one more thing about this, before we move
on to the next version of the patchset. The above is inaccurate. The
intent of VM_BUG_ON() and BUG_ON() is similar, but there is *definitely*
a logical separation between them: they do not behave the same at runtime.

Just because some distros enable VM_BUG_ON(), does not mean that we can
treat it the same as BUG_ON() in "both directions". From a "don't BUG()
crash the kernel unless really warranted", they are about the same, as
Linus keeps repeating. From the other direction, though ("I need to BUG()-
crash the kernel"), they are NOT the same. BUG_ON() is more reliably
available.

And that's the essence of my object to treating this as if you have
reliably stopped the kernel with a VM_BUG_ON(). It's not really the same!


thanks,
-- 
John Hubbard
NVIDIA