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

[Michal Hocko <mhocko@suse.com>]

On Fri 06-06-25 13:44:12, David Hildenbrand wrote:
> On 06.06.25 13:04, Lorenzo Stoakes wrote:
> > On Fri, Jun 06, 2025 at 12:28:28PM +0200, David Hildenbrand wrote:
> > > On 06.06.25 12:19, Lorenzo Stoakes wrote:
> > > > On Fri, Jun 06, 2025 at 12:13:27PM +0200, Michal Hocko wrote:
> > > > > On Fri 06-06-25 11:01:18, David Hildenbrand wrote:
> > > > > > On 06.06.25 10:31, Michal Hocko wrote:
> > > > > [...]
> > > > > > > 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 ...
> > > > > 
> > > > > *WARN_ONCE ha a very limited use from code paths which are generally
> > > > > shared by many callers. You just see one and then nothing. Some time ago
> > > > > we have discussed an option to have _ONCE per call trace but I haven't
> > > > > see any follow up.
> > > > > 
> > > > > Anyway starting without _ONCE seems like safer option because we are not
> > > > > losing potentially useful debugging information. Afterall this is
> > > > > debugging only thing. But no strong position on my side.
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > You meand VM_BUG_ON expand to VM_WARN_ON by default?
> > > > 
> > > > Sorry to interject in the conversation, but to boldly throw my two English pence
> > > > into the mix:
> > > > 
> > > > As the "king of churn" (TM) you'll not be surprised to hear that I'm in favour
> > > > of us just doing a big patch and convert all VM_BUG_ON() -> VM_WARN_ON_ONCE()
> > > > and remove VM_BUG_ON*().
> > > > 
> > > > Pull the band-aid off... I think better than a #define if this indeed what you
> > > > meant David.
> > > > 
> > > > But of course, you'd expect me to have this opinion ;)
> > > 
> > > See my reply to Michal regarding keeping VM_BUG_ON() until we actually
> > > decided what the right cleanup is.
> > 
> > Sure, but to me the concept of VM_BUG_ON() is surely fundamentally broken - if
> > BUG_ON() means 'stop everything we're going to corrupt' then it makes no sense
> > to add a '...but only if CONFIG_DEBUG_VM is set' in there.
> > 
> > So to me the only assessment needed is 'do we want to warn on this or not?'.
> 
> Well, when done carefully, it would be when reworking a VM_BUG_ON:
> 
> (a) Should this really only be checked with DEBUG_VM or should this
>     actually be a WARN_ON_ONCE() + recovery
> (b) Does this check even still make sense in current code, or were we
>     just extra careful initially.
> (c) Do we even understand why it is checked or should we add a comment.
> (d) Should we use one of the _PAGE / _FOLIO / _VMA etc. variants instead
>     or even add new ones.

This is surelly a very responsible approach and I salute to that. But
then the reality hits...
 
> One could argue that the same is true for any other VM_WARN_ON ... but my
> point from the beginning was that if we're already touching them, why not
> spend some extra time and do it properly ..
> 
> ... but yeah, 600 instances are a bit much.

... exactly this. And I believe that the same could be achieved post
factum while having the ugly VM_BUG on gone.

> So agreed, let's move forward with a simple conversion.

Good call IMHO.

[...]
> > Am happy to come up with the churn-meister version of this patch and take the
> > heat :P
> 
> I assume with "this patch" you mean "a patch that gets rid of VM_BUG_ON
> completely", because I want this patch here (that started the discussion) to
> go in first.

Yes this makes sense.

> Fascinating how you are always looking for work :P

Thanks to both of you!
-- 
Michal Hocko
SUSE Labs