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;
From:   Song Liu <songliubraving@fb.com>
To:     linux-kernel <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>
CC:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Kernel Team <Kernel-team@fb.com>,
        Stephane Eranian <eranian@google.com>,
        Lucian Grijincu <lucian@fb.com>
Subject: Re: [PATCH v2] perf/core: fix userpage->time_enabled of inactive
 events
Thread-Topic: [PATCH v2] perf/core: fix userpage->time_enabled of inactive
 events
Thread-Index: AQHXsON8cxUzcBLit0S7boefzLsslKu4GRYA
Date:   Mon, 27 Sep 2021 16:33:43 +0000
Message-ID: <A38DD9FF-E142-4BD0-89C4-2B1B699812D7@fb.com>
References: <20210924012800.2461781-1-songliubraving@fb.com>
In-Reply-To: <20210924012800.2461781-1-songliubraving@fb.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?+XcFdy61nneR3Pr71uqGB3ijULyyu3EuufPql5KKmPO2ZnMiFPMMGlOjUfso?=
 =?us-ascii?Q?x+vMO2QKouwNGOvjm2fHaTVTB1QV7a03MFHfilEopN/SiOLPWoYRJ2rSeaH1?=
 =?us-ascii?Q?Ur3W0Q1E1joTV0k2zuV6aEuZj7IVnVVtJpbfU2bXeQTEdRge7FCV606Xcipn?=
 =?us-ascii?Q?XmVQbChGpS668CkoZNlXQpldjAFMZWy5pf7uyd3w0tQtWlzhAiM7Ws9Xp13K?=
 =?us-ascii?Q?g0Q4la+ffZlIEGROu/HI+cA1VXMKfRVgE1wo7KQcCXenl/7zvXVb8QAU0x3Z?=
 =?us-ascii?Q?sHMDzTh0x0l1pJzPlxXFQVCDcUU6lBtXUogIU/VLAx4C8A8APurSkRUp02g6?=
 =?us-ascii?Q?zJC0YkEkMw97f0IdljkN3EfJqbN5S5EqhEQ0watjAXerwdtQewdR66QXPyDV?=
 =?us-ascii?Q?Ghapd5o5zWflg9IAdmzIF6O8ZapPc54QQxg5hTK8okl7TQzBYzSq/EzVgOr7?=
 =?us-ascii?Q?hiM2JpMaufrAUdqan8hL+sWdb9TathsVWEn8wmwp6B+y/ROyVgMYBPi3DHtF?=
 =?us-ascii?Q?SLnpOFIVQBRPb8chYEz+DV7m0ChYgb3DwLqtrua08KiqyP3x4owQSQF6Ccbl?=
 =?us-ascii?Q?oARESCTl31zLir7DF7FKA8EkdzSmhJeERIWzyE+LpA23qJ1xrH1z9u6bIDys?=
 =?us-ascii?Q?MHQwfSfGl+TUIO2YFhuPsuAbCKgoi6TAbL5/cTvS/aUr54QGIDeqpkk5NB8T?=
 =?us-ascii?Q?DDuHXcFFp976JKoVbHXdhszIu5Uo8FFDu+zcefMCtmDEG7QlXBIk7kBo3cA6?=
 =?us-ascii?Q?WUJtv/EyNoU8SedzNCJ/3S9lJwbMmv1vNMIGjvwFHBxXawHeA6bU+pnOGY9S?=
 =?us-ascii?Q?nW6KZoFVYrvO7hbMf2Z0lAyhGbvI1iYkHdFnuyQpNdG2HDERDsCm8Ux5CTlN?=
 =?us-ascii?Q?BD24A0fJdP08tDVWc68cjXis7IuluwjxL1YpCh0NJ9aUdFKn7gSahyi6OKEz?=
 =?us-ascii?Q?SJZx/DCDdopPKDib2Rc5gCuH278D3XdjR9R/j0C2cAP28Wlsd3iHRCbvufbE?=
 =?us-ascii?Q?0xEBgIslBGg9hDLLu1aSGyocyvNjtptQdCGYv0nH6ltqGQmVbzMGSyBKhJ0I?=
 =?us-ascii?Q?qapfJDIJzvxnFvZzgMt0ueVXF+g9GPwwMX96gFgBUxLcmVRrTW2F8Z7EP/oF?=
 =?us-ascii?Q?PxTNo1YOCG5gubxsJmCDdtzcTVm4JrTFhlcfeBQ9tkNRCu6/lR8cmhMbU1jn?=
 =?us-ascii?Q?qIQ+sZbWKSNBIZ/KKtcACbj/Va2CfcgDTbtV/YkOJZWvNty3MAB/m+dVlys8?=
 =?us-ascii?Q?TsbdmuNBNFU5WbwCHXIjWw30WVftxnM3149xJWFPYS/796Yq4aORTyMJ7yVH?=
 =?us-ascii?Q?sKGROwrJyshm9/cmtVapTiJO+fAzxHAVumBcIlHAMXbHo0I/RSHgpBtj8K3v?=
 =?us-ascii?Q?gViYMqU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <6B6BC0625B037C4FB98E06E1D382958B@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 89b12a4b-7214-4749-ad1d-08d981d4920f
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Sep 2021 16:33:43.2499
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: FVti+mBS536FWZpWrJuP7YP+vvPcwfAxtmxXJ/yrvJfVpeed0kEmUfGpBEidnbvvuHt6TUD61bmaScgLg81VhA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB5064
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: EsEvqxRSDKiLL7JH72V8pBAqlnxQVXH4
X-Proofpoint-ORIG-GUID: EsEvqxRSDKiLL7JH72V8pBAqlnxQVXH4
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475
 definitions=2021-09-27_06,2021-09-24_02,2020-04-07_01
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 adultscore=0
 clxscore=1015 mlxlogscore=999 priorityscore=1501 spamscore=0
 impostorscore=0 suspectscore=0 malwarescore=0 mlxscore=0 phishscore=0
 bulkscore=0 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2109230001 definitions=main-2109270112
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Peter, 

> On Sep 23, 2021, at 6:28 PM, Song Liu <songliubraving@fb.com> wrote:
> 
> Users of rdpmc rely on the mmapped user page to calculate accurate
> time_enabled. Currently, userpage->time_enabled is only updated when the
> event is added to the pmu. As a result, inactive event (due to counter
> multiplexing) does not have accurate userpage->time_enabled. This can
> be reproduced with something like:
> 
>   /* open 20 task perf_event "cycles", to create multiplexing */
> 
>   fd = perf_event_open();  /* open task perf_event "cycles" */
>   userpage = mmap(fd);     /* use mmap and rdmpc */
> 
>   while (true) {
>     time_enabled_mmap = xxx; /* use logic in perf_event_mmap_page */
>     time_enabled_read = read(fd).time_enabled;
>     if (time_enabled_mmap > time_enabled_read)
>         BUG();
>   }
> 
> Fix this by updating userpage for inactive events in merge_sched_in.
> 
> Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> Reported-and-tested-by: Lucian Grijincu <lucian@fb.com>
> Signed-off-by: Song Liu <songliubraving@fb.com>

Could you please share your comments on this version?

Thanks,
Song

> ---
> include/linux/perf_event.h |  4 +++-
> kernel/events/core.c       | 49 ++++++++++++++++++++++++++++++++++----
> 2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 2d510ad750edc..4aa52f7a48c16 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -683,7 +683,9 @@ struct perf_event {
> 	/*
> 	 * timestamp shadows the actual context timing but it can
> 	 * be safely used in NMI interrupt context. It reflects the
> -	 * context time as it was when the event was last scheduled in.
> +	 * context time as it was when the event was last scheduled in,
> +	 * or when ctx_sched_in failed to schedule the event because we
> +	 * run out of PMC.
> 	 *
> 	 * ctx_time already accounts for ctx->timestamp. Therefore to
> 	 * compute ctx_time for a sample, simply add perf_clock().
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 1cb1f9b8392e2..d73f986eef7b3 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3707,6 +3707,46 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx,
> 	return 0;
> }
> 
> +static inline bool event_update_userpage(struct perf_event *event)
> +{
> +	/*
> +	 * Checking mmap_count to avoid unnecessary work. This does leave a
> +	 * corner case: if the event is enabled before mmap(), the first
> +	 * time the event gets scheduled is via:
> +	 *
> +	 *  __perf_event_enable (or __perf_install_in_context)
> +	 *      -> ctx_resched
> +	 *         -> perf_event_sched_in
> +	 *            -> ctx_sched_in
> +	 *
> +	 * with mmap_count of 0, so we will skip here. As a result,
> +	 * userpage->offset is not accurate after mmap and before the
> +	 * first rotation.
> +	 *
> +	 * To avoid the discrepancy of this window, the user space should
> +	 * mmap the event before enabling it.
> +	 */
> +	if (likely(!atomic_read(&event->mmap_count)))
> +		return false;
> +
> +	perf_event_update_time(event);
> +	perf_set_shadow_time(event, event->ctx);
> +	perf_event_update_userpage(event);
> +
> +	return true;
> +}
> +
> +static inline void group_update_userpage(struct perf_event *group_event)
> +{
> +	struct perf_event *event;
> +
> +	if (!event_update_userpage(group_event))
> +		return;
> +
> +	for_each_sibling_event(event, group_event)
> +		event_update_userpage(event);
> +}
> +
> static int merge_sched_in(struct perf_event *event, void *data)
> {
> 	struct perf_event_context *ctx = event->ctx;
> @@ -3725,14 +3765,15 @@ static int merge_sched_in(struct perf_event *event, void *data)
> 	}
> 
> 	if (event->state == PERF_EVENT_STATE_INACTIVE) {
> +		*can_add_hw = 0;
> 		if (event->attr.pinned) {
> 			perf_cgroup_event_disable(event, ctx);
> 			perf_event_set_state(event, PERF_EVENT_STATE_ERROR);
> +		} else {
> +			ctx->rotate_necessary = 1;
> +			perf_mux_hrtimer_restart(cpuctx);
> +			group_update_userpage(event);
> 		}
> -
> -		*can_add_hw = 0;
> -		ctx->rotate_necessary = 1;
> -		perf_mux_hrtimer_restart(cpuctx);
> 	}
> 
> 	return 0;
> -- 
> 2.30.2
>