On 17.08.22 12:40, Mel Gorman wrote:
> On Wed, Aug 17, 2022 at 08:59:11AM +0200, Michal Hocko wrote:
>>> In order to address that, we should either have to call first_zones_zonelist
>>> inside get_page_from_freelist if the zoneref doesn't correspond to a
>>> real zone in the zonelist or we should revisit my older approach
>>> referenced above.
>>
>> Would this work? It is not really great to pay an overhead for unlikely
>> event in the hot path but we might use a similar trick to check_retry_cpuset
>> in the slowpath to detect this situation.
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index b0bcab50f0a3..bce786d7fcb4 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4098,7 +4098,17 @@ get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
>> * See also __cpuset_node_allowed() comment in kernel/cpuset.c.
>> */
>> no_fallback = alloc_flags & ALLOC_NOFRAGMENT;
>> +
>> + /*
>> + * A race with memory offlining could alter zones on the zonelist
>> + * e.g. dropping the top (movable) zone if it gets unpoppulated
>> + * and so preferred_zoneref is not valid anymore
>> + */
>> + if (unlikely(!ac->preferred_zoneref->zone))
>> + ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
>> + ac->highest_zoneidx, ac->nodemask);
>> z = ac->preferred_zoneref;
>> +
>
> ac->preferred_zoneref->zone could still be a valid pointer to a zone,
> but an empty one so that would imply
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index e5486d47406e..38ce123af543 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5191,6 +5191,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> if (check_retry_cpuset(cpuset_mems_cookie, ac))
> goto retry_cpuset;
>
> + /* Hotplug could have drained the preferred zone. */
> + if (!populated_zone(ac->preferred_zoneref->zone))
> + goto retry_cpuset;
> +
> /* Reclaim has failed us, start killing things */
> page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> if (page)
>
> But even that is fragile. If there were multiple zones in the zonelist
> and the preferred zone was further down the list, the zone could still
> be populated but a different zone than expected. It may be better to have
> the same type of seq counter that restarts the allocation attempt if the
> zonelist changes.
>
> So.... this? It is seqcount only with a basic lock as there already is a
> full lock on the writer side and it would appear to be overkill to protect
> the reader side with read_seqbegin_or_lock as it complicates the writer side.
>
> (boot tested only)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index e5486d47406e..158954b10724 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4708,6 +4708,22 @@ void fs_reclaim_release(gfp_t gfp_mask)
> EXPORT_SYMBOL_GPL(fs_reclaim_release);
> #endif
>
> +/*
> + * Zonelists may change due to hotplug during allocation. Detect when zonelists
> + * have been rebuilt so allocation retries.
> + */
> +static seqcount_t zonelist_update_seq = SEQCNT_ZERO(zonelist_update_seq);
> +
> +static unsigned int zonelist_update_begin(void)
> +{
> + return read_seqcount_begin(&zonelist_update_seq);
> +}
> +
> +static unsigned int zonelist_update_retry(unsigned int seq)
> +{
> + return read_seqcount_retry(&zonelist_update_seq, seq);
> +}
> +
> /* Perform direct synchronous page reclaim */
> static unsigned long
> __perform_reclaim(gfp_t gfp_mask, unsigned int order,
> @@ -5001,6 +5017,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> int compaction_retries;
> int no_progress_loops;
> unsigned int cpuset_mems_cookie;
> + unsigned int zonelist_update_cookie;
> int reserve_flags;
>
> /*
> @@ -5016,6 +5033,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> no_progress_loops = 0;
> compact_priority = DEF_COMPACT_PRIORITY;
> cpuset_mems_cookie = read_mems_allowed_begin();
> + zonelist_update_cookie = zonelist_update_begin();
>
> /*
> * The fast path uses conservative alloc_flags to succeed only until
> @@ -5191,6 +5209,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> if (check_retry_cpuset(cpuset_mems_cookie, ac))
> goto retry_cpuset;
>
> + if (zonelist_update_retry(zonelist_update_cookie))
> + goto retry_cpuset;
> +
> /* Reclaim has failed us, start killing things */
> page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> if (page)
> @@ -6517,6 +6538,7 @@ static void __build_all_zonelists(void *data)
> static DEFINE_SPINLOCK(lock);
>
> spin_lock(&lock);
> + write_seqcount_begin(&zonelist_update_seq);
>
> #ifdef CONFIG_NUMA
> memset(node_load, 0, sizeof(node_load));
> @@ -6553,6 +6575,7 @@ static void __build_all_zonelists(void *data)
> #endif
> }
>
> + write_seqcount_end(&zonelist_update_seq);
> spin_unlock(&lock);
Do we want to get rid of the static lock by using a seqlock_t instead of
a seqcount_t?
--
Thanks,
David / dhildenb
