On Wed, 5 Aug 2020, Roman Gushchin wrote: > On Wed, Aug 05, 2020 at 08:01:33PM -0700, Hugh Dickins wrote: > > > > I shall certainly want to reintroduce those stats to checking for > > negatives, even if it's in a patch that never earns your approval, > > and just ends up kept internal for debugging. But equally certainly, > > I must not suddenly reintroduce that checking without gaining some > > experience of it (and perhaps getting as irritated as you by more > > transient negatives). > > > > I said earlier that I'd prefer you to rip out all that checking for > > negatives, rather than retaining it with the uselessly over-generous > > 125 * nr_cpus leeway. Please, Roman, would you send Andrew a patch > > doing that, to replace the patch in this thread? Or if you prefer, > > I can do so. > > Sure, I can do it. But can you, please, explain why you want them to be > eliminated? Is this because they will never fire, you think?
Yes, I've never seen a machine on which vm/stat_refresh reported -16000 or less, or anything approaching that (true, this laptop does not have 128 cpus, but...). Maybe they get reinstalled or otherwise rebooted before reaching numbers of that magnitude. Maybe having the warnings shown at much lower magnitudes has helped to root-cause and eliminate some of them. Waiting until the heat-death-of-the-universe theoretical worst case is so unhelpful as to defeat the purpose of the warning. I think you do understand, but perhaps not all readers of this thread understand, that vm/stat_refresh merges all per-cpu counts into the global atomic immediately before deciding negative. The only problem is that "immediately" is not instantaneous across cpus, so the possibility of work started on one cpu but completed on another during the course of the refresh, causing false negatives, is real though not great. > > In my humble opinion they might be quite useful: any systematic under- or > overflow will eventually trigger this warning, and we'll know for sure that > something is wrong. So I'd add a similar check for node counters without > any hesitation. It's true that while developing, we can all make mistakes so big that "eventually" will show up quickly, and even that warning could help. But since you'll only show them when they reach -16000 (sorry, I keep going back to the 128 cpu case, just to make it more vivid), it won't be of any use to catch races later in development, or in production. Whereas my own patch would just fix the missing items, and continue to annoy you with occasional ignorable warnings - so I cannot submit that, and wouldn't want to submit it right now, without having tried it out for a while, to check what kind of noise it will generate. So I thought it best, either to leave mm/vmstat.c alone for the moment, or else just delete the disputed and incomplete code; coming back to it later when we have something we can agree upon. But I think you're preferring to resubmit your 125*nr_cpus patch to akpm, with the missing NR_VM_NODE_STAT_ITEMS added in (as either one or two patches), with foreshortened testing but the reassurance that since it's so hard to reach the point of showing the warnings, new negatives will either be quiet, or easily fixed before v5.10 released: okay, I can more easily Hack that to my preference than Ack it or Nak it. Hugh
