Re: [PATCH 2/2 RESEND] mm: vmpressure: fix sending wrong events on underflow

[Michal Hocko]

On Mon 06-02-17 20:05:21, vinayak menon wrote:
[...]
> By scan I meant pages scanned by shrink_node_memcg/shrink_list
> which is passed as nr_scanned to vmpressure.  The calculation of
> pressure for tree is done at the end of vmpressure_win and it is
> that calculation which underflows. With this patch we want only the
> underflow to be avoided. But if we make (reclaimed = scanned) in
> vmpressure(), we change the vmpressure value even when there is no
> underflow right ?
>
> Rewriting the above e.g again.  First call to vmpressure with
> nr_scanned=1 and nr_reclaimed=512 (THP) Second call to vmpressure
> with nr_scanned=511 and nr_reclaimed=0 In the second call
> vmpr->tree_scanned becomes equal to vmpressure_win and the work
> is scheduled and it will calculate the vmpressure as 0 because
> tree_reclaimed = 512
>
> Similarly, if scanned is made equal to reclaimed in vmpressure()
> itself as you had suggested, First call to vmpressure with
> nr_scanned=1 and nr_reclaimed=512 (THP) And in vmpressure, we
> make nr_scanned=1 and nr_reclaimed=1 Second call to vmpressure
> with nr_scanned=511 and nr_reclaimed=0 In the second call
> vmpr->tree_scanned becomes equal to vmpressure_win and the work is
> scheduled and it will calculate the vmpressure as critical, because
> tree_reclaimed = 1
> 
> So it makes a difference, no?

OK, I see what you meant. Thanks for the clarification. And you are
right that normalizing nr_reclaimed to nr_scanned is a wrong thing to
do because that just doesn't aggregate the real work done. Normalizing
nr_scanned to nr_reclaimed should be better - or it would be even better
to count the scanned pages properly...

My main concern of doing this normalization late on aggregated numbers
is just weird. We are mixing numbers from parallel reclaimers and that
might just add more confusion. It is better to do the fixup as soon as
possible when we still have at least an idea that this was a THP page
scanned and reclaimed.

If we get back to your example it works as you expect just due to good
luck. Just make your nr_scanned=511 and nr_reclaimed=0 be a separate
event and you have your critical event. You have no real control over
when a new event is fired because parallel reclaimers are basically
unpredictable.
-- 
Michal Hocko
SUSE Labs