On Thu 19-08-21 16:38:59, Johannes Weiner wrote:
> On Thu, Aug 19, 2021 at 05:01:38PM +0200, Michal Hocko wrote:
[...]
> > The logic is spread over 3 different places.
> >
> > Would something like the following be more understandable?
> >
> > /*
> > * Low limit protected memcgs are already excluded at
> > * a higher level (shrink_node_memcgs) but scaling
> > * down the reclaim target can result in hard to
> > * reclaim and premature OOM. We do not have a full
> > * picture here so we cannot really judge this
> > * sutuation here but pro-actively flag this scenario
> > * and let do_try_to_free_pages to retry if
> > * there is no progress.
> > */
>
> I've been drafting around with this, but it seems to say the same
> thing as the comment I put into struct scan_control already:
>
> /*
> * Cgroup memory below memory.low is protected as long as we
> * don't threaten to OOM. If any cgroup is reclaimed at
> * reduced force or passed over entirely due to its memory.low
> * setting (memcg_low_skipped), and nothing is reclaimed as a
> * result, then go back back for one more cycle that reclaims
> * the protected memory (memcg_low_reclaim) to avert OOM.
> */
>
> How about a brief version of this with a pointer to the original?
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 701106e1829c..c32d686719d5 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2580,7 +2580,12 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc,
> unsigned long cgroup_size = mem_cgroup_size(memcg);
> unsigned long protection;
>
> - /* memory.low scaling, make sure we retry before OOM */
> + /*
> + * Soft protection must not cause reclaim failure. Let
> + * the upper level know if we skipped pages during the
> + * first pass, so it can retry if necessary. See the
> + * struct scan_control definition of those flags.
> + */
> if (!sc->memcg_low_reclaim && low > min) {
> protection = low;
> sc->memcg_low_skipped = 1;
> @@ -2853,16 +2858,16 @@ static void shrink_node_memcgs(pg_data_t *pgdat, struct scan_control *sc)
>
> if (mem_cgroup_below_min(memcg)) {
> /*
> - * Hard protection.
> - * If there is no reclaimable memory, OOM.
> + * Hard protection. Always respected. If there is not
> + * enough reclaimable memory elsewhere, it's an OOM.
> */
> continue;
> } else if (mem_cgroup_below_low(memcg)) {
> /*
> - * Soft protection.
> - * Respect the protection only as long as
> - * there is an unprotected supply
> - * of reclaimable memory from other cgroups.
> + * Soft protection must not cause reclaim failure. Let
> + * the upper level know if we skipped pages during the
> + * first pass, so it can retry if necessary. See the
> + * struct scan_control definition of those flags.
> */
> if (!sc->memcg_low_reclaim) {
> sc->memcg_low_skipped = 1;
Yes, this makes the situation more explicit. I still see some advantage
to be explicit about those other layers as this will be easier to follow
the code but I will certainly not insist.
Andrew has already sent your original patch to Linus so this will need
to go as a separate patch. For that
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
Thanks!
--
Michal Hocko
SUSE Labs
