On Fri, Oct 22, 2021 at 12:15:10PM +1100, NeilBrown wrote: > On Tue, 19 Oct 2021, Mel Gorman wrote: > > Changelog since v3 > > o Count writeback completions for NR_THROTTLED_WRITTEN only > > o Use IRQ-safe inc_node_page_state > > o Remove redundant throttling > > > > This series is also available at > > > > git://git.kernel.org/pub/scm/linux/kernel/git/mel/linux.git mm-reclaimcongest-v4r2 > > > > This series that removes all calls to congestion_wait > > in mm/ and deletes wait_iff_congested. > > Thanks for this. > I don't have sufficient expertise for a positive review, but it seems to > make sense with one exception which I have commented on separately. > A test battering NFS would still be nice! > In general, I still don't like the use of wake_up_all(), though it won't > cause incorrect behaviour. > Removing wake_up_all would be tricky. Ideally it would be prioritised but more importantly, some sort of guarantee should exist that enough wakeup events trigger to wake tasks before the timeout. That would need careful thinking about each reclaim reason. For example, if N tasks throttle on NOPROGRESS, there is no guarantee that N tasks are currently in reclaim that would wake each sleeping task as progress is made. It's similar for writeback, are enough pages under writeback to trigger each wakeup? A more subtle issue is if each reason should be strict if waking tasks one at a time. For example, a task sleeping on writeback might make progress for other reasons such as the working set changing during reclaim or a large task exiting. Of course the same concerns exist for the series as it stands but the worst case scenarios are mitigated by wake_up_all. > I would prefer the first patch would: > - define NR_VMSCAN_THROTTLE > - make reclaim_wait an array > - spelled nr_reclaim_throttled as nr_writeback_throttled > > rather than leaving those changes for the second patch. I think that > would make review easier. > I can do this. Normally I try structure series from least-to-most controversial so that it can be cut at any point and still make sense so the array was defined in the second patch because that's when it is required. However, I already had defined the enum in patch 1 for the tracepoint so I might as well make it an array too. -- Mel Gorman SUSE Labs
