Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933693AbdDGMrX (ORCPT ); Fri, 7 Apr 2017 08:47:23 -0400 Received: from mx2.suse.de ([195.135.220.15]:59934 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932905AbdDGMrM (ORCPT ); Fri, 7 Apr 2017 08:47:12 -0400 Date: Fri, 7 Apr 2017 14:47:02 +0200 From: Michal Hocko To: Johannes Weiner Cc: Andrew Morton , Vladimir Davydov , 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 Message-ID: <20170407124702.GE16413@dhcp22.suse.cz> References: <20170404220148.28338-1-hannes@cmpxchg.org> <20170404220148.28338-2-hannes@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170404220148.28338-2-hannes@cmpxchg.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1104 Lines: 45 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