| Message ID | 20210519192321.3247175-1-atomlin@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm/page_alloc: bail out on fatal signal during reclaim/compaction retry attempt | expand |
On Wed, May 19, 2021 at 08:23:21PM +0100, Aaron Tomlin wrote: > +++ b/mm/page_alloc.c > @@ -4252,6 +4252,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > if (!order) > return false; > > + if (fatal_signal_pending(current)) > + goto out; I think 'goto out' will be confusing. It'll output a tracepoint, which isn't going to record that a fatal signal is pending, so it'll cause some head scratching for someone looking through the traces. I think we should just return false here and skip the tracepoint. But I'd defer to someone like Vlastimil or Michal who know this code far better than I do.
On Wed 2021-05-19 20:32 +0100, Matthew Wilcox wrote: > On Wed, May 19, 2021 at 08:23:21PM +0100, Aaron Tomlin wrote: > > +++ b/mm/page_alloc.c > > @@ -4252,6 +4252,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > > if (!order) > > return false; > > > > + if (fatal_signal_pending(current)) > > + goto out; > > I think 'goto out' will be confusing. It'll output a tracepoint, which > isn't going to record that a fatal signal is pending, so it'll cause > some head scratching for someone looking through the traces. I > think we should just return false here and skip the tracepoint. I agree. Having said this, I do plan to send a patch to illustrate why compaction should not retry to avoid possible confusion. Nevertheless, I am happy to modify as per your request. Kind regards,
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index aaa1655cf682..49f416ffb54f 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4252,6 +4252,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, if (!order) return false; + if (fatal_signal_pending(current)) + goto out; + if (compaction_made_progress(compact_result)) (*compaction_retries)++;
It does not make sense to retry compaction when a fatal signal is pending. In the context of try_to_compact_pages(), indeed COMPACT_SKIPPED can be returned; albeit, not every zone, on the zone list, would be considered in the case a fatal signal is found to be pending. Yet, in should_compact_retry(), given the last known compaction result, each zone, on the zone list, can be considered/or checked (see compaction_zonelist_suitable()). For example, if a zone was found to succeed, then reclaim/compaction would be tried again (notwithstanding the above). This patch ensures that compaction is not needlessly retried irrespective of the last known compaction result e.g. if it was skipped, in the unlikely case a fatal signal is found pending. So, OOM is at least attempted. Signed-off-by: Aaron Tomlin <atomlin@redhat.com> --- mm/page_alloc.c | 3 +++ 1 file changed, 3 insertions(+)