Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 25 Mar 2021 09:57:29 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Song Liu <songliubraving@fb.com>
Cc:     Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>, Jiri Olsa <jolsa@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Stephane Eranian <eranian@google.com>,
        Andi Kleen <ak@linux.intel.com>,
        Ian Rogers <irogers@google.com>, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH 1/2] perf/core: Share an event with multiple cgroups
Message-ID: <YFyIuZWs+GINtoTY@kernel.org>
References: <20210323162156.1340260-1-namhyung@kernel.org>
 <20210323162156.1340260-2-namhyung@kernel.org>
 <C0AF9F1F-F525-4047-AD89-F75E3FEFC215@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <C0AF9F1F-F525-4047-AD89-F75E3FEFC215@fb.com>
Precedence: bulk

Em Thu, Mar 25, 2021 at 12:55:50AM +0000, Song Liu escreveu:
> > On Mar 23, 2021, at 9:21 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> > #ifdef CONFIG_SECURITY
> > @@ -780,6 +792,14 @@ struct perf_event {
> > #endif /* CONFIG_PERF_EVENTS */
> > };

> > +struct perf_cgroup_node {
> > +	struct hlist_node		node;
> > +	u64				id;
> > +	u64				count;
> > +	u64				time_enabled;
> > +	u64				time_running;
> > +	u64				padding[2];
> 
> Do we really need the padding? For cache line alignment? 

I guess so, to get it to 64 bytes, then having it as:

struct perf_cgroup_node {
	struct hlist_node		node;
	u64				id;
	u64				count;
	u64				time_enabled;
	u64				time_running;
} ____cacheline_aligned;

Seems better :-)

Testing:

[acme@five c]$ cat cacheline_aligned.c
#ifndef ____cacheline_aligned
#define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
#endif

// from ../build/v5.12.0-rc4+/include/generated/autoconf.h
#define CONFIG_X86_L1_CACHE_SHIFT 6

#define L1_CACHE_SHIFT  (CONFIG_X86_L1_CACHE_SHIFT)
#define L1_CACHE_BYTES  (1 << L1_CACHE_SHIFT)

#ifndef SMP_CACHE_BYTES
#define SMP_CACHE_BYTES L1_CACHE_BYTES
#endif

typedef long long unsigned int u64;

struct hlist_node {
	struct hlist_node *        next;                 /*     0     8 */
	struct hlist_node * *      pprev;                /*     8     8 */

	/* size: 16, cachelines: 1, members: 2 */
	/* last cacheline: 16 bytes */
};

struct perf_cgroup_node {
        struct hlist_node               node;
        u64                             id;
        u64                             count;
        u64                             time_enabled;
        u64                             time_running;
} ____cacheline_aligned foo;

[acme@five c]$ cc  -g  -c -o cacheline_aligned.o cacheline_aligned.c
[acme@five c]$ pahole cacheline_aligned.o
struct hlist_node {
	struct hlist_node *        next;                 /*     0     8 */
	struct hlist_node * *      pprev;                /*     8     8 */

	/* size: 16, cachelines: 1, members: 2 */
	/* last cacheline: 16 bytes */
};
struct perf_cgroup_node {
	struct hlist_node          node;                 /*     0    16 */
	u64                        id;                   /*    16     8 */
	u64                        count;                /*    24     8 */
	u64                        time_enabled;         /*    32     8 */
	u64                        time_running;         /*    40     8 */

	/* size: 64, cachelines: 1, members: 5 */
	/* padding: 16 */
} __attribute__((__aligned__(64)));
[acme@five c]$

- Arnaldo