linux-kernel - Re: [PATCH 2/4] mm: memcontrol: re-use global VM event enum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170407124702.GE16413@dhcp22.suse.cz>
Date:   Fri, 7 Apr 2017 14:47:02 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>, linux-mm@...ck.org,
        cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com
Subject: Re: [PATCH 2/4] mm: memcontrol: re-use global VM event enum

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