linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Chris Down <chris@chrisdown.name>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Tejun Heo <tj@kernel.org>, Roman Gushchin <guro@fb.com>,
	linux-kernel@vger.kernel.org, cgroups@vger.kernel.org,
	linux-mm@kvack.org, kernel-team@fb.com
Subject: Re: [PATCH] mm: Move maxable seq_file logic into a single place
Date: Thu, 24 Jan 2019 11:56:34 -0500	[thread overview]
Message-ID: <20190124165634.GA13549@chrisdown.name> (raw)
In-Reply-To: <20190124160935.GB12436@cmpxchg.org>

Johannes Weiner writes:
>I think this increases complexity more than it saves LOC,
>unfortunately.
>
>The current situation is a bit repetitive, but much more obviously
>correct. And we're not planning on adding many more of those memcg
>interface files, so I this doesn't seem to be an improvement re:
>maintainability and future extensibility of the code.

Hmm, my main goal in the patch was not really reduction of LOC, but more around 
centralising logic so that it's clear where these functions are unique and 
where they are completely the same. My initial reaction upon reading these was 
mostly to feel like I'm missing something due to the large amount of similarity 
between them, wondering if the change in mem_cgroup/page_counter access is 
really the only thing to look at, so my goal was primarily to reduce confusion 
(although of course this is subjective, I can also see your point about how 
having "memory.low" and the like without context can also be confusing).

I think using a macro is not ideal, but unfortunately without it a lot of the 
complexity can't really be abstracted easily.

If not this, what would you think about a patch that adds two new functions:

1. mem_cgroup_from_seq
2. mem_cgroup_write_max_or_val

This won't be able to reduce as much of the overlap as the macro, but it still 
will centralise a lot of the logic.

  reply	other threads:[~2019-01-24 16:56 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24  6:17 [PATCH] mm: Move maxable seq_file logic into a single place Chris Down
2019-01-24  8:58 ` Michal Hocko
2019-01-24 16:09 ` Johannes Weiner
2019-01-24 16:56   ` Chris Down [this message]
2019-01-24 19:25     ` Chris Down

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=20190124165634.GA13549@chrisdown.name \
    --to=chris@chrisdown.name \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=tj@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).