Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Song Liu <songliubraving@fb.com>
To:     Stephane Eranian <eranian@google.com>
CC:     open list <linux-kernel@vger.kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@elte.hu" <mingo@elte.hu>,
        "acme@redhat.com" <acme@redhat.com>,
        "jolsa@redhat.com" <jolsa@redhat.com>,
        "kan.liang@intel.com" <kan.liang@intel.com>,
        "irogers@google.com" <irogers@google.com>
Subject: Re: [PATCH] perf/core: fix multiplexing event scheduling issue
Thread-Topic: [PATCH] perf/core: fix multiplexing event scheduling issue
Thread-Index: AQHVhUsMoVyDSFs26kucCFo0fCE/0adf67AA
Date:   Fri, 18 Oct 2019 06:13:21 +0000
Message-ID: <7B6B74DF-53E0-42DA-97D6-3F04E84D688B@fb.com>
References: <20191018002746.149200-1-eranian@google.com>
In-Reply-To: <20191018002746.149200-1-eranian@google.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <D3BD5F9E019BD74EB5D6C995E5ACD1D1@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 87725262-e7b1-4d98-5ded-08d7539246c4
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Oct 2019 06:13:21.1660
 (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: oJcbt3XuKlKz3vhwzeHXORnQjLI3SnSb1BwcXtcHYaHnPODrI+260CLVR39vrEDdX0uktiwYNF3AVwKKhKS27A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1770
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8
 definitions=2019-10-18_01:2019-10-17,2019-10-18 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 adultscore=0
 malwarescore=0 bulkscore=0 priorityscore=1501 spamscore=0
 lowpriorityscore=0 mlxlogscore=999 impostorscore=0 mlxscore=0
 clxscore=1011 phishscore=0 suspectscore=0 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-1908290000
 definitions=main-1910180060
X-FB-Internal: deliver
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Oct 17, 2019, at 5:27 PM, Stephane Eranian <eranian@google.com> wrote:
>=20
> This patch complements the following commit:
> 7fa343b7fdc4 ("perf/core: Fix corner case in perf_rotate_context()")
>=20
> The fix from Song addresses the consequences of the problem but
> not the cause. This patch fixes the causes and can sit on top of
> Song's patch.
>=20
> This patch fixes a scheduling problem in the core functions of
> perf_events. Under certain conditions, some events would not be
> scheduled even though many counters would be available. This
> is related to multiplexing and is architecture agnostic and
> PMU agnostic (i.e., core or uncore).
>=20
> This problem can easily be reproduced when you have two perf
> stat sessions. The first session does not cause multiplexing,
> let's say it is measuring 1 event, E1. While it is measuring,
> a second session starts and causes multiplexing. Let's say it
> adds 6 events, B1-B6. Now, 7 events compete and are multiplexed.
> When the second session terminates, all 6 (B1-B6) events are
> removed. Normally, you'd expect the E1 event to continue to run
> with no multiplexing. However, the problem is that depending on
> the state Of E1 when B1-B6 are removed, it may never be scheduled
> again. If E1 was inactive at the time of removal, despite the
> multiplexing hrtimer still firing, it will not find any active
> events and will not try to reschedule. This is what Song's patch
> fixes. It forces the multiplexing code to consider non-active events.

Good analysis! I kinda knew the example I had (with pinned event)
was not the only way to trigger this issue. But I didn't think=20
about event remove path.=20

> However, the cause is not addressed. The kernel should not rely on
> the multiplexing hrtimer to unblock inactive events. That timer
> can have abitrary duration in the milliseconds. Until the timer
> fires, counters are available, but no measurable events are using
> them. We do not want to introduce blind spots of arbitrary durations.
>=20
> This patch addresses the cause of the problem, by checking that,
> when an event is disabled or removed and the context was multiplexing
> events, inactive events gets immediately a chance to be scheduled by
> calling ctx_resched(). The rescheduling is done on  event of equal
> or lower priority types.  With that in place, as soon as a counter
> is freed, schedulable inactive events may run, thereby eliminating
> a blind spot.
>=20
> This can be illustrated as follows using Skylake uncore CHA here:
>=20
> $ perf stat --no-merge -a -I 1000 -C 28 -e uncore_cha_0/event=3D0x0/
>    42.007856531      2,000,291,322      uncore_cha_0/event=3D0x0/
>    43.008062166      2,000,399,526      uncore_cha_0/event=3D0x0/
>    44.008293244      2,000,473,720      uncore_cha_0/event=3D0x0/
>    45.008501847      2,000,423,420      uncore_cha_0/event=3D0x0/
>    46.008706558      2,000,411,132      uncore_cha_0/event=3D0x0/
>    47.008928543      2,000,441,660      uncore_cha_0/event=3D0x0/
>=20
> Adding second sessiont with 4 events for 4s
>=20
> $ perf stat -a -I 1000 -C 28 --no-merge -e uncore_cha_0/event=3D0x0/,unco=
re_cha_0/event=3D0x0/,uncore_cha_0/event=3D0x0/,uncore_cha_0/event=3D0x0/ s=
leep 4
>    48.009114643      1,983,129,830      uncore_cha_0/event=3D0x0/        =
                               (99.71%)
>    49.009279616      1,976,067,751      uncore_cha_0/event=3D0x0/        =
                               (99.30%)
>    50.009428660      1,974,448,006      uncore_cha_0/event=3D0x0/        =
                               (98.92%)
>    51.009524309      1,973,083,237      uncore_cha_0/event=3D0x0/        =
                               (98.55%)
>    52.009673467      1,972,097,678      uncore_cha_0/event=3D0x0/        =
                               (97.11%)
>=20
> End of 4s, second session is removed. But the first event does not schedu=
le and never will
> unless new events force multiplexing again.
>=20
>    53.009815999      <not counted>      uncore_cha_0/event=3D0x0/        =
                               (95.28%)
>    54.009961809      <not counted>      uncore_cha_0/event=3D0x0/        =
                               (93.52%)
>    55.010110972      <not counted>      uncore_cha_0/event=3D0x0/        =
                               (91.82%)
>    56.010217579      <not counted>      uncore_cha_0/event=3D0x0/        =
                               (90.18%)

Does this still happen after my patch? I was not able to repro this.=20

> Signed-off-by: Stephane Eranian <eranian@google.com>

Maybe add:
Fixes: 8d5bce0c37fa ("perf/core: Optimize perf_rotate_context() event sched=
uling")

Thanks,
Song=