All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michal Hocko <mhocko@kernel.org>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	linux-mm@kvack.org, cgroups@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 2/4] mm: memcontrol: re-use global VM event enum
Date: Fri, 7 Apr 2017 14:47:02 +0200	[thread overview]
Message-ID: <20170407124702.GE16413@dhcp22.suse.cz> (raw)
In-Reply-To: <20170404220148.28338-2-hannes@cmpxchg.org>

I do agree that we should share global and memcg specific events constants
but I am not sure we want to share all of them. Would it make sense to
reorganize the global enum and put those that are shared to the
beginning? We wouldn't need the memcg specific translation then.

Anyway, two comments on the current implementation.

On Tue 04-04-17 18:01:46, Johannes Weiner wrote:
[...]
> +/* Cgroup-specific events, on top of universal VM events */
> +enum memcg_event_item {
> +	MEMCG_LOW = NR_VM_EVENT_ITEMS,
> +	MEMCG_HIGH,
> +	MEMCG_MAX,
> +	MEMCG_OOM,
> +	MEMCG_NR_EVENTS,
> +};

The above should mention that each supported global VM event should
provide the corresponding translation

[...]

here...
> +/* Universal VM events cgroup1 shows, original sort order */
> +unsigned int memcg1_events[] = {
> +	PGPGIN,
> +	PGPGOUT,
> +	PGFAULT,
> +	PGMAJFAULT,
> +};
> +
> +static const char *const memcg1_event_names[] = {
> +	"pgpgin",
> +	"pgpgout",
> +	"pgfault",
> +	"pgmajfault",
> +};

the naming doesn't make it easier to undestand why we need this.
global2memcg_event?

-- 
Michal Hocko
SUSE Labs

WARNING: multiple messages have this Message-ID
From: Michal Hocko <mhocko@kernel.org>
To: Johannes Weiner <hannes@cmpxchg.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Vladimir Davydov <vdavydov.dev@gmail.com>,
	linux-mm@kvack.org, cgroups@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 2/4] mm: memcontrol: re-use global VM event enum
Date: Fri, 7 Apr 2017 14:47:02 +0200	[thread overview]
Message-ID: <20170407124702.GE16413@dhcp22.suse.cz> (raw)
In-Reply-To: <20170404220148.28338-2-hannes@cmpxchg.org>

I do agree that we should share global and memcg specific events constants
but I am not sure we want to share all of them. Would it make sense to
reorganize the global enum and put those that are shared to the
beginning? We wouldn't need the memcg specific translation then.

Anyway, two comments on the current implementation.

On Tue 04-04-17 18:01:46, Johannes Weiner wrote:
[...]
> +/* Cgroup-specific events, on top of universal VM events */
> +enum memcg_event_item {
> +	MEMCG_LOW = NR_VM_EVENT_ITEMS,
> +	MEMCG_HIGH,
> +	MEMCG_MAX,
> +	MEMCG_OOM,
> +	MEMCG_NR_EVENTS,
> +};

The above should mention that each supported global VM event should
provide the corresponding translation

[...]

here...
> +/* Universal VM events cgroup1 shows, original sort order */
> +unsigned int memcg1_events[] = {
> +	PGPGIN,
> +	PGPGOUT,
> +	PGFAULT,
> +	PGMAJFAULT,
> +};
> +
> +static const char *const memcg1_event_names[] = {
> +	"pgpgin",
> +	"pgpgout",
> +	"pgfault",
> +	"pgmajfault",
> +};

the naming doesn't make it easier to undestand why we need this.
global2memcg_event?

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  parent reply	other threads:[~2017-04-07 12:47 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-04 22:01 [PATCH 1/4] mm: memcontrol: clean up memory.events counting function Johannes Weiner
2017-04-04 22:01 ` Johannes Weiner
2017-04-04 22:01 ` [PATCH 2/4] mm: memcontrol: re-use global VM event enum Johannes Weiner
2017-04-04 22:01   ` Johannes Weiner
2017-04-06  8:49   ` Vladimir Davydov
2017-04-06  8:49     ` Vladimir Davydov
2017-04-10 14:17     ` Johannes Weiner
2017-04-10 14:17       ` Johannes Weiner
2017-04-07 12:47   ` Michal Hocko [this message]
2017-04-07 12:47     ` Michal Hocko
2017-04-10 14:13     ` Johannes Weiner
2017-04-10 14:13       ` Johannes Weiner
2017-04-11 12:30       ` Michal Hocko
2017-04-11 12:30         ` Michal Hocko
2017-04-04 22:01 ` [PATCH 3/4] mm: memcontrol: re-use node VM page state enum Johannes Weiner
2017-04-04 22:01   ` Johannes Weiner
2017-04-06  8:59   ` Vladimir Davydov
2017-04-06  8:59     ` Vladimir Davydov
2017-04-04 22:01 ` [PATCH 4/4] mm: memcontrol: use node page state naming scheme for memcg Johannes Weiner
2017-04-04 22:01   ` Johannes Weiner
2017-04-06  9:01   ` Vladimir Davydov
2017-04-06  9:01     ` Vladimir Davydov
2017-04-07 12:54   ` Michal Hocko
2017-04-07 12:54     ` Michal Hocko
2017-04-06  8:31 ` [PATCH 1/4] mm: memcontrol: clean up memory.events counting function Vladimir Davydov
2017-04-06  8:31   ` Vladimir Davydov
2017-04-07 12:20 ` Michal Hocko
2017-04-07 12:20   ` Michal Hocko

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=20170407124702.GE16413@dhcp22.suse.cz \
    --to=mhocko@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=cgroups@vger.kernel.org \
    --cc=hannes@cmpxchg.org \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH 2/4] mm: memcontrol: re-use global VM event enum' \
    /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

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.