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/0adf67AAgAABuQCAAAn8AA==
Date:   Fri, 18 Oct 2019 06:55:14 +0000
Message-ID: <32FCDA83-9888-480A-9A77-AEB37FD004CE@fb.com>
References: <20191018002746.149200-1-eranian@google.com>
 <7B6B74DF-53E0-42DA-97D6-3F04E84D688B@fb.com>
 <CABPqkBT40-wVWq7K93QJc1r_1=R0WQuoa_SHebWApPEstCWeNg@mail.gmail.com>
In-Reply-To: <CABPqkBT40-wVWq7K93QJc1r_1=R0WQuoa_SHebWApPEstCWeNg@mail.gmail.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: <1A09FA85496B174A90A7C5BC124A883A@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f759ca0a-3899-461e-c7ff-08d7539820e1
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Oct 2019 06:55:14.6957
 (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: /xUwyRog+Fq2TT/k6q3BxQnAwt0fQwiafbul/ngNh/9FSIcLbq68YerGiPEhNybF/rvJv6jaGSLDmTyBRGnCYg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1199
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8
 definitions=2019-10-18_02: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=1015 phishscore=0 suspectscore=0 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-1908290000
 definitions=main-1910180065
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 11:19 PM, Stephane Eranian <eranian@google.com> wrote=
:
>=20
> On Thu, Oct 17, 2019 at 11:13 PM Song Liu <songliubraving@fb.com> wrote:
>>=20
>>=20
>>=20
>>> On Oct 17, 2019, at 5:27 PM, Stephane Eranian <eranian@google.com> wrot=
e:
>>>=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.
>>=20
>> 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
>> about event remove path.
>>=20
> I was pursuing this bug without knowledged of your patch. Your patch
> makes it harder to see. Clearly in my test case, it disappears, but it is
> just because of the multiplexing interval. If we get to the rotate code
> and we have no active events yet some inactive, there is something
> wrong because we are wasting counters. When I tracked the bug,
> I started from the remove_context code, then realized there was also
> the disable case. I fixed both and they I discovered your patch which
> is fixing it at the receiving end. Hopefully, there aren't any code path
> that can lead to this situation.

Thanks for the explanation. Agreed that blind spot has bigger impact=20
with longer rotation interval.=20

[...]

>>> Signed-off-by: Stephane Eranian <eranian@google.com>
>>=20
>> Maybe add:
>> Fixes: 8d5bce0c37fa ("perf/core: Optimize perf_rotate_context() event sc=
heduling")
>>=20
> It does not really fix your patch, I think we can keep it as a double
> precaution. It fixes
> the causes. I think it is useful to check beyond the active in the
> rotate code as well.

Also agreed, this is not really fixing that specific commit.=20

Acked-by: Song Liu <songliubraving@fb.com>

Thanks,
Song