Nice to see the +130.0% this morning. I got back on to this on Monday, here's some follow-up. On Sun, 26 Jul 2020, Hugh Dickins wrote: > > The comparison runs have not yet completed (except for the one started > early), but they have all got past the most interesting tests, and it's > clear that they do not have the "failure"s seen with your patches. > > From that I can only conclude that your patches make a difference. > > I've deduced nothing useful from the logs, will have to leave that > to others here with more experience of them. But my assumption now > is that you have successfully removed one bottleneck, so the tests > get somewhat further and now stick in the next bottleneck, whatever > that may be. Which shows up as "failure", where the unlock_page() > wake_up_page_bit() bottleneck had allowed the tests to proceed in > a more serially sedate way. Yes, that's still how it appears to me. The test failures, all of them, came from fork() returning ENOSPC, which originated from alloc_pid()'s idr_alloc_cyclic(). I did try doubling our already large pid_max, that did not work out well, there are probably good reasons for it to be where it is and I was wrong to dabble with it. I also tried an rcu_barrier() and retry when getting -ENOSPC there, thinking maybe RCU was not freeing them up fast enough, but that didn't help either. I think (but didn't quite make the effort to double-check with an independent count) it was simply running out of pids: that your change speeds up the forking enough, that exiting could not quite keep up (SIGCHLD is SIG_IGNed); whereas before your change, the unlock_page() in do_wp_page(), on a PageAnon stack page, slowed the forking down enough when heavily contended. (I think we could improve the checks there, to avoid taking page lock in more cases; but I don't know if that would help any real-life workload - I see now that Michal's case is do_read_fault() not do_wp_page().) And FWIW a further speedup there is the opposite of what these tests are wanting: for the moment I've enabled a delay to get them passing as before. Something I was interested to realize in looking at this: trylock_page() on a contended lock is now much less likely to jump the queue and succeed than before, since your lock holder hands off the page lock to the next holder: much smaller window than waiting for the next to wake to take it. Nothing wrong with that, but effect might be seen somewhere. > > The xhci handle_cmd_completion list_del bugs (on an older version > of the driver): weird, nothing to do with page wakeups, I'll just > have to assume that it's some driver bug exposed by the greater > stress allowed down, and let driver people investigate (if it > still manifests) when we take in your improvements. Complete red herring. I'll give Greg more info in response to his mail, and there may be an xhci bug in there; but when I looked back, found I'd come across the same bug back in October, and find that occasionally it's been seen in our fleet. Yes, it's odd that your change coincided with it becoming more common on that machine (which I've since replaced by another), yes it's funny that it's in __list_del_entry_valid(), which is exactly where I got crashes on pages with your initial patch; but it's just a distraction. > > One nice thing from the comparison runs without your patches: > watchdog panic did crash one of those with exactly the unlock_page() > wake_up_page_bit() softlockup symptom we've been fighting, that did > not appear with your patches. So although the sample size is much > too small to justify a conclusion, it does tend towards confirming > your changes. > > Thank you for your work on this! And I'm sure you'd have preferred > some hard data back, rather than a diary of my mood swings, but... > we do what we can. > > Hugh
