On Fri, Apr 24, 2020 at 8:18 PM Chris Down chris@chrisdown.name wrote:
Yafang Shao writes:
If the author can't understand deeply in the code worte by himself/herself, I think the author should do more test on his/her patches. Regarding the issue in this case, my understanding is you know the benefit of proportional reclaim, but I'm wondering that do you know the loss if the proportional is not correct ? I don't mean to affend you, while I just try to explain how the community should cooperate.
I'm pretty sure that since multiple people on mm list have already expressed confusion at this patch, this isn't a question of testing, but of lack of clarity in usage :-)
Promoting "testing" as a panacea for this issue misses a significant part of the real problem: that the intended semantics and room for allowed races is currently unclear, which is why there is a general sense of confusion around your proposed patch and what it solves. If more testing would help, then the benefit of your patch should be patently obvious -- but it isn't.
I have shown a testcase in my commit log. Bellow is the result without my patch,
[ 601.811428] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811429] vmscan: [ 601.811429] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811430] vmscan: [ 601.811430] vmscan: protection 1048576 memcg /foo target memcg /foo [ 601.811431] vmscan: [ 602.452791] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452795] vmscan: [ 602.452796] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452805] vmscan: [ 602.452805] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452806] vmscan: [ 602.452807] vmscan: protection 1048576 memcg /foo target memcg /foo [ 602.452808] vmscan:
Here's patch to print the above info.
diff --git a/mm/vmscan.c b/mm/vmscan.c index b06868fc4926..7525665d7cec 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -2344,10 +2344,18 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, unsigned long lruvec_size; unsigned long scan; unsigned long protection; + struct mem_cgroup *target = sc->target_mem_cgroup;
lruvec_size = lruvec_lru_size(lruvec, lru, sc->reclaim_idx); protection = mem_cgroup_protection(memcg, sc->memcg_low_reclaim); + if (memcg && memcg != root_mem_cgroup && target) { + pr_info("protection %lu memcg ", protection); + pr_cont_cgroup_path(memcg->css.cgroup); + pr_cont(" target memcg "); + pr_cont_cgroup_path(target->css.cgroup); + pr_info("\n"); + }
if (protection) {
So my question is that do you think the protection in these log is okay ? Can you answer me ?
Hint: what should protection be if memcg is the target memcg ?