Mike Kravetz reports that "hugetlb allocations could stall for minutes or hours when should_compact_retry() would return true more often then it should. Specifically, this was in the case where compact_result was COMPACT_DEFERRED and COMPACT_PARTIAL_SKIPPED and no progress was being made." The problem is that the compaction_withdrawn() test in should_compact_retry() includes compaction outcomes that are only possible on low compaction priority, and results in a retry without increasing the priority. This may result in furter reclaim, and more incomplete compaction attempts. With this patch, compaction priority is raised when possible, or should_compact_retry() returns false. The COMPACT_SKIPPED result doesn't really fit together with the other outcomes in compaction_withdrawn(), as that's a result caused by insufficient order-0 pages, not due to low compaction priority. With this patch, it is moved to a new compaction_needs_reclaim() function, and for that outcome we keep the current logic of retrying if it looks like reclaim will be able to help.
Unified · Split
| @@ -129,11 +129,8 @@ static inline bool compaction_failed(enum compact_result result) · 128 unmodified lines | |||
| 129 | return false; | 129 | return false; |
| 130 | } | 130 | } |
| 131 | 131 | ||
| 132 | /* | 132 | /* Compaction needs reclaim to be performed first, so it can continue. */ |
| 133 | * Compaction has backed off for some reason. It might be throttling or | 133 | static inline bool compaction_needs_reclaim(enum compact_result result) |
| 134 | * lock contention. Retrying is still worthwhile. | ||
| 135 | */ | ||
| 136 | static inline bool compaction_withdrawn(enum compact_result result) | ||
| 137 | { | 134 | { |
| 138 | /* | 135 | /* |
| 139 | * Compaction backed off due to watermark checks for order-0 | 136 | * Compaction backed off due to watermark checks for order-0 |
| @@ -142,6 +139,16 @@ static inline bool compaction_withdrawn(enum compact_result result) · 2 unmodified lines | |||
| 142 | if (result == COMPACT_SKIPPED) | 139 | if (result == COMPACT_SKIPPED) |
| 143 | return true; | 140 | return true; |
| 144 | 141 | ||
| 142 | return false; | ||
| 143 | } | ||
| 144 | |||
| 145 | /* | ||
| 146 | * Compaction has backed off for some reason after doing some work or none | ||
| 147 | * at all. It might be throttling or lock contention. Retrying might be still | ||
| 148 | * worthwhile, but with a higher priority if allowed. | ||
| 149 | */ | ||
| 150 | static inline bool compaction_withdrawn(enum compact_result result) | ||
| 151 | { | ||
| 145 | /* | 152 | /* |
| 146 | * If compaction is deferred for high-order allocations, it is | 153 | * If compaction is deferred for high-order allocations, it is |
| 147 | * because sync compaction recently failed. If this is the case | 154 | * because sync compaction recently failed. If this is the case |
| @@ -207,6 +214,11 @@ static inline bool compaction_failed(enum compact_result result) · 59 unmodified lines | |||
| 207 | return false; | 214 | return false; |
| 208 | } | 215 | } |
| 209 | 216 | ||
| 217 | static inline bool compaction_needs_reclaim(enum compact_result result) | ||
| 218 | { | ||
| 219 | return false; | ||
| 220 | } | ||
| 221 | |||
| 210 | static inline bool compaction_withdrawn(enum compact_result result) | 222 | static inline bool compaction_withdrawn(enum compact_result result) |
| 211 | { | 223 | { |
| 212 | return true; | 224 | return true; |
| @@ -3954,15 +3954,23 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, · 3953 unmodified lines | |||
| 3954 | if (compaction_failed(compact_result)) | 3954 | if (compaction_failed(compact_result)) |
| 3955 | goto check_priority; | 3955 | goto check_priority; |
| 3956 | 3956 | ||
| 3957 | /* | ||
| 3958 | * compaction was skipped because there are not enough order-0 pages | ||
| 3959 | * to work with, so we retry only if it looks like reclaim can help. | ||
| 3960 | */ | ||
| 3961 | if (compaction_needs_reclaim(compact_result)) { | ||
| 3962 | ret = compaction_zonelist_suitable(ac, order, alloc_flags); | ||
| 3963 | goto out; | ||
| 3964 | } | ||
| 3965 | |||
| 3957 | /* | 3966 | /* |
| 3958 | * make sure the compaction wasn't deferred or didn't bail out early | 3967 | * make sure the compaction wasn't deferred or didn't bail out early |
| 3959 | * due to locks contention before we declare that we should give up. | 3968 | * due to locks contention before we declare that we should give up. |
| 3960 | * But do not retry if the given zonelist is not suitable for | 3969 | * But the next retry should use a higher priority if allowed, so |
| 3961 | * compaction. | 3970 | * we don't just keep bailing out endlessly. |
| 3962 | */ | 3971 | */ |
| 3963 | if (compaction_withdrawn(compact_result)) { | 3972 | if (compaction_withdrawn(compact_result)) { |
| 3964 | ret = compaction_zonelist_suitable(ac, order, alloc_flags); | 3973 | goto check_priority; |
| 3965 | goto out; | ||
| 3966 | } | 3974 | } |
| 3967 | 3975 | ||
| 3968 | /* | 3976 | /* |