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:     Athira Rajeev <atrajeev@linux.vnet.ibm.com>
CC:     LKML <linux-kernel@vger.kernel.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Kernel Team <Kernel-team@fb.com>,
        Stephane Eranian <eranian@google.com>,
        Lucian Grijincu <lucian@fb.com>,
        Madhavan Srinivasan <maddy@linux.vnet.ibm.com>,
        kajoljain <kjain@linux.ibm.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: AQHXsON8cxUzcBLit0S7boefzLsslKu7RmiAgAArwgA=
Date:   Wed, 29 Sep 2021 19:41:19 +0000
Message-ID: <4AB79DA5-A8AF-456D-A009-3F24EEDA20F2@fb.com>
References: <20210924012800.2461781-1-songliubraving@fb.com>
 <EEA8D03E-14A7-4A8B-8C99-B68CACEC25D2@linux.vnet.ibm.com>
In-Reply-To: <EEA8D03E-14A7-4A8B-8C99-B68CACEC25D2@linux.vnet.ibm.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?pRiDwbxv6mlZ/qlxgqgjLyHKxF0Zv6qfKeF78UdEtE3GWNv/NHXS38hiJqmr?=
 =?us-ascii?Q?EbCeaxtUtTW6Uf5WQRlDRsIPT6adDj9RIRiG0zsWWjdo9AUEU03A8mUYjJGx?=
 =?us-ascii?Q?AJnInJGjudRt/+97Oy5GTck04KkQc317Do3Scv1QbUhsHi7HycnwOk0AHlhh?=
 =?us-ascii?Q?XkwOUUqEc+DuEvprBAMTjBb9B6g5JJ85hqDsUttYAjh6SysH4EN7HrP2HYH5?=
 =?us-ascii?Q?9rPySP4gjwN9rR1v8LrFp1ItvG9WPESq/e9m8wS1twt+hBzunULQBEgS7vGG?=
 =?us-ascii?Q?osOIie2HGNNsCN1MssSpO9fhESVHDVC9YixGiK/tOrX0+yvd2M1Ix877UNj4?=
 =?us-ascii?Q?1InakuWoeCHXGpintsMU6BGHB6RXqk5MGcR0Zz4yHnLb1UHqNyRqBPi9yQYK?=
 =?us-ascii?Q?QEQp3xvoVgSr92L9WZcMCQ7O1ZSSpTRkECHRHKLQU8of9m4vXkOYc+56NQSn?=
 =?us-ascii?Q?g02+KQlDqQmG2jcuV7unIRsDP4lVs1glFMWozOlhdlInh+V3G9RIpiASdYne?=
 =?us-ascii?Q?wKXHJPkpg0M7C4GXR2cusTCMBqVop+VK1aaBB9k5lmI/HDX4HGE4pArBVzFo?=
 =?us-ascii?Q?/FJi6Q8clqzx2pwkVO9XkfIfSn08FOS+nacM8h1Li9uI5SEvn5P2tCU0YKgi?=
 =?us-ascii?Q?QHkOi1KMYSVZWlcFvzKpXrw0y6Y08lmGLUsfDrgHWmD5g3sLvcaVkxN1f4wN?=
 =?us-ascii?Q?DV7hsWE7Zl+DuNTMnalkTcTr2FtMxMQZsLoNmVrFMZCE2V6OpDLn8JIogZaD?=
 =?us-ascii?Q?MSAjDvpxxInSe2O6SynpdXVi9s2o/VvPRS+ZTqJ/T5FcLBgNSayCMReNxeXQ?=
 =?us-ascii?Q?RvopDYaRoJ1UTUFMQ9vv+S3fXACJDBlrmvGI3k+4yKnaj47E6kPhp2em/GEj?=
 =?us-ascii?Q?sSU3ukcElfN8itnDUvF32PhzyLiV/815N/HcrYynVnJcM13wBR1i2uEC9QLj?=
 =?us-ascii?Q?mDJST1Hdcl5wh5S2ZloUqgYTsC6zPhe0AGgCztUYLlHTCuimQlhsde9KAgCE?=
 =?us-ascii?Q?DheTmhVdjDsS6vfWIn1GKveOJb0cljFQ38qnxpLs/DdciiJiOgvzjQthmQec?=
 =?us-ascii?Q?/mCeAyc6UC2Y6A04x3ZlwjdHVfKgZtP1VYZ11Bqnuzhg9VN8ceGeYTadsMcu?=
 =?us-ascii?Q?6g0uerPtzWBCGXX1izGqzqmFhHCuhVzvYqJUj/M+brDrwXLz3r6wX65O8LGx?=
 =?us-ascii?Q?8MF5rwa/uXnBRV+EBqrEsvV3maddMyes4u/z5GnZLPT0m3aFQuR3Lu3fNI/Y?=
 =?us-ascii?Q?uNQg4kKf0EANabuAeg+mPr977rlWFH4OanCUM3z3S/nom0AYJTG+dQsdeEk4?=
 =?us-ascii?Q?F5E1zUmw+fiXJ5F7XVGllC0XI0Qn8T5MdRtztYBBC7YihqZGTzqLoU2xh1tr?=
 =?us-ascii?Q?7sIARIU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D2F3BE52D2AD8D47854BEA9BA2F39A92@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: bb161613-8e03-4b1e-1863-08d983811be9
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Sep 2021 19:41:19.0622
 (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: ug2cYEOMsMwrTXOu7q6mUSL4cCw3Qu0TkafKlfFHt25Q7P1DES70HHDhxVOxZq7BtLtvkvT2AbJnRriE42JY0A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB5095
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: nggSdQNEWRas7aK59zXiJYxVHQgVIqeX
X-Proofpoint-ORIG-GUID: nggSdQNEWRas7aK59zXiJYxVHQgVIqeX
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-29_08,2021-09-29_01,2020-04-07_01
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 mlxscore=0
 priorityscore=1501 lowpriorityscore=0 phishscore=0 mlxlogscore=999
 malwarescore=0 suspectscore=0 impostorscore=0 adultscore=0 bulkscore=0
 clxscore=1015 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2109290114
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Sep 29, 2021, at 10:04 AM, Athira Rajeev <atrajeev@linux.vnet.ibm.com> wrote:
> 
> 
> 
>> On 24-Sep-2021, at 6:58 AM, 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>
>> ---
>> 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);
>> 	}
> 
> Another optimisation that is possible in merge_sched_in is we can avoid calling "perf_mux_hrtimer_restart" multiple times if already rotate_necessary is set for that context. Even though "perf_mux_hrtimer_restart" will just return if hrtimer is already active, we could avoid the overhead of calling this function multiple times if there are many groups. 
> 
> Something like below:
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 0c000cb01eeb..26eae79bd723 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -3731,8 +3731,10 @@ static int merge_sched_in(struct perf_event *event, void *data)
>                }
> 
>                *can_add_hw = 0;
> -               ctx->rotate_necessary = 1;
> -               perf_mux_hrtimer_restart(cpuctx);
> +               if (!ctx->rotate_necessary) {
> +                       ctx->rotate_necessary = 1;
> +                       perf_mux_hrtimer_restart(cpuctx);
> +               }
>        }
> 
>        return 0;

Yeah, this makes sense. Do you plan to send a patch for this? 

Thanks,
Song