Re: [PATCH v1] mm/gup: remove (VM_)BUG_ONs

[David Hildenbrand <david@redhat.com>]

On 06.06.25 10:31, Michal Hocko wrote:
> On Fri 06-06-25 10:10:14, David Hildenbrand wrote:
>> On 05.06.25 09:10, Michal Hocko wrote:
>>> On Wed 04-06-25 16:05:44, David Hildenbrand wrote:
>>>> Especially once we hit one of the assertions in
>>>> sanity_check_pinned_pages(), observing follow-up assertions failing
>>>> in other code can give good clues about what went wrong, so use
>>>> VM_WARN_ON_ONCE instead.
>>>>
>>>> While at it, let's just convert all VM_BUG_ON to VM_WARN_ON_ONCE as
>>>> well. Add one comment for the pfn_valid() check.
>>>>
>>>> We have to introduce VM_WARN_ON_ONCE_VMA() to make that fly.
>>>>
>>>> Drop the BUG_ON after mmap_read_lock_killable(), if that ever returns
>>>> something > 0 we're in bigger trouble. Convert the other BUG_ON's into
>>>> VM_WARN_ON_ONCE as well, they are in a similar domain "should never
>>>> happen", but more reasonable to check for during early testing.
>>>
>>> The patch itself makes sense and I think it is good time to revisit old
>>> discussion [1] and finally drop VM_BUG_ON altogether and replace it by
>>> VM_WARN_ON which could be still a useful debugging aid.
>>
>> Yes. I think we should check all cases if they are really relevant (and are
>> clear), and also handle BUG_ON on the way.
> 
> There are more than 600 VM_BUG_ON instances.I am not sure it is
> feasible to review single one of them.

I'm sure we would be done if we would have started back when Dave R. 
sent his patch ... :)

It's not that many files, and many checks in a file are often of a 
similar kind.

Like many "folio_test_locked" checks.

> Turning them into VM_WARN_ON
> should be reasonably safe as they are not enabled in production
> environment anyway so we cannot really rely on those. Having them in
> WARN form would be still useful for debugging and those that really need
> a crash dump while debugging can achieve the same result.

One question is if we should be VM_WARN_ON vs. VM_WARN_ON_ONCE ...

VM_BUG_ON is essentially a "once" thing so far, but then, we don't 
continue ... so probably most should be _ONCE.

> 
> So while I agree that many of them could be dropped or made more clear
> those could be dealt with after a mass move. An advantage of this would
> be that we can drop VM_BUG_ON* and stop new instances from being added.

As a first step we could probably just #define them to go to the 
VM_WARN_ON_* variants and see what happens.

-- 
Cheers,

David / dhildenb