From: Johannes Weiner <hannes@cmpxchg.org>
To: Michal Hocko <mhocko@kernel.org>
Cc: mateusznosek0@gmail.com, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, akpm@linux-foundation.org
Subject: Re: [PATCH] mm/vmscan.c: Clean code by removing unnecessary assignment
Date: Fri, 20 Mar 2020 11:55:05 -0400 [thread overview]
Message-ID: <20200320155505.GA204561@cmpxchg.org> (raw)
In-Reply-To: <20200319171334.GK20800@dhcp22.suse.cz>
On Thu, Mar 19, 2020 at 06:13:34PM +0100, Michal Hocko wrote:
> It is usually preferable to Cc author of the code (added Johannes)
>
> On Thu 19-03-20 17:59:38, mateusznosek0@gmail.com wrote:
> > From: Mateusz Nosek <mateusznosek0@gmail.com>
> >
> > Previously 0 was assigned to 'sc->skipped_deactivate'. It could happen only
> > if 'sc->skipped_deactivate' was 0 so the assignment is unnecessary and can
> > be removed.
>
> The above wording was a bit hard to understdand for me. I would go with
> "
> sc->memcg_low_skipped resets skipped_deactivate to 0 but this is not
> needed as this code path is never reachable with skipped_deactivate != 0
> due to previous sc->skipped_deactivate branch.
> "
Yeah that sounds good.
> > Signed-off-by: Mateusz Nosek <mateusznosek0@gmail.com>
>
> The patch is correct. I am not sure it results in a better code though.
> I will defer to Johannes here. I suspect he simply wanted to express
> that skipped_deactivate should be always reset when retrying the direct
> reclaim. After this patch this could be lost in future changes so the
> code would be more subtle. But I am only guessing here.
It's a valid concern, but I think in this case specifically we're very
unlikely to change the ordering here - violate memory.low before going
after active pages of unprotected cgroups.
I indeed just kept it stupid: reset everything, then retry. But it
appears that the unnecessary assignment trips people up and wastes
their time, so I'm in favor of removing it.
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
next prev parent reply other threads:[~2020-03-20 15:55 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 16:59 [PATCH] mm/vmscan.c: Clean code by removing unnecessary assignment mateusznosek0
2020-03-19 17:13 ` Michal Hocko
2020-03-20 15:55 ` Johannes Weiner [this message]
-- strict thread matches above, loose matches on Subject: below --
2020-02-29 21:40 mateusznosek0
2020-03-01 0:11 ` Matthew Wilcox
2020-03-02 13:54 ` Wei Yang
2020-03-03 8:59 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200320155505.GA204561@cmpxchg.org \
--to=hannes@cmpxchg.org \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mateusznosek0@gmail.com \
--cc=mhocko@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.