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

[Michal Hocko <mhocko@suse.com>]

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. 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.

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.

Just my 2cents
-- 
Michal Hocko
SUSE Labs