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:     Peter Zijlstra <peterz@infradead.org>
CC:     Alexei Starovoitov <ast@fb.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        Kernel Team <Kernel-team@fb.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>
Subject: Re: [PATCH bpf-next 2/2] bpf/stackmap: fix A-A deadlock in
 bpf_get_stack()
Thread-Topic: [PATCH bpf-next 2/2] bpf/stackmap: fix A-A deadlock in
 bpf_get_stack()
Thread-Index: AQHVfzKraNK9YMcIS0WwHSwrlFbkdqdTfF0AgACi24CAAAefAIAABZIAgAWzPYCAALmAgA==
Date:   Mon, 14 Oct 2019 20:12:59 +0000
Message-ID: <048ED376-246D-4895-B5DA-FB3158C99122@fb.com>
References: <20191010061916.198761-1-songliubraving@fb.com>
 <20191010061916.198761-3-songliubraving@fb.com>
 <20191010073608.GO2311@hirez.programming.kicks-ass.net>
 <a1d30b11-2759-0293-5612-48150db92775@fb.com>
 <20191010174618.GT2328@hirez.programming.kicks-ass.net>
 <4865df4d-7d13-0655-f3b4-5d025aaa1edb@fb.com>
 <20191014090903.GA2328@hirez.programming.kicks-ass.net>
In-Reply-To: <20191014090903.GA2328@hirez.programming.kicks-ass.net>
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: <E1B63940F9EEE443B9F123C048498F46@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b4624f09-0376-481e-58a8-08d750e2e8d9
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Oct 2019 20:12:59.4064
 (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: 02p02APrS160hJ9JCgo3Z/m4bBwxRYZrA8ZtXuRuUnqHN1B43OqzexHNzDWPeWrAGJ3w5Iw6bB2B8zDdPm6Geg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1182
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.95,1.0.8
 definitions=2019-10-14_10:2019-10-11,2019-10-14 signatures=0
X-Proofpoint-Spam-Details: rule=fb_default_notspam policy=fb_default score=0 lowpriorityscore=0
 bulkscore=0 priorityscore=1501 adultscore=0 mlxlogscore=999 spamscore=0
 mlxscore=0 suspectscore=0 clxscore=1015 phishscore=0 impostorscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-1908290000 definitions=main-1910140168
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

Thanks Peter!

> On Oct 14, 2019, at 2:09 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
> On Thu, Oct 10, 2019 at 06:06:14PM +0000, Alexei Starovoitov wrote:
>> On 10/10/19 10:46 AM, Peter Zijlstra wrote:
>=20
>>> All of stack_map_get_build_id_offset() is just disguisting games; I did
>>> tell you guys how to do lockless vma lookups a few years ago -- and yes=
,
>>> that is invasive core mm surgery. But this is just disguisting hacks fo=
r
>>> not wanting to do it right.
>>=20
>> you mean speculative page fault stuff?
>> That was my hope as well and I offered Laurent all the help to land it.
>> Yet after a year since we've talked the patches are not any closer
>> to landing.
>> Any other 'invasive mm surgery' you have in mind?
>=20
> Indeed that series. It had RCU managed VMAs and lockless VMA lookups,
> which is exactly what you need here.

Lockless VMA lookups will be really useful. It would resolve all the=20
pains we are having here.=20

I remember Google folks also mentioned in LPC that they would like=20
better mechanism to confirm build-id in perf.=20

>=20
>>> Basically the only semi-sane thing to do with that trainwreck is
>>> s/in_nmi()/true/ and pray.
>>>=20
>>> On top of that I just hate buildids in general.
>>=20
>> Emotions aside... build_id is useful and used in production.
>> It's used widely because it solves real problems.
>=20
> AFAIU it solves the problem of you not knowing what version of the
> binary runs where; which I was hoping your cloud infrastructure thing
> would actually know already.
>=20
> Anyway, I know what it does, I just don't nessecarily agree it is the
> right way around that particular problem (also, the way I'm personally
> affected is that perf-record is dead slow by default due to built-id
> post processing).
>=20
> And it obviously leads to horrible hacks like the code currently under
> discussion :/
>=20
>> This dead lock is from real servers and not from some sanitizer wannabe.
>=20
> If you enable CFS bandwidth control and run this function on the
> trace_hrtimer_start() tracepoint, you should be able to trigger a real
> AB-BA lockup.
>=20
>> Hence we need to fix it as cleanly as possible and quickly.
>> s/in_nmi/true/ is certainly an option.
>=20
> That is the best option; because tracepoints / perf-overflow handlers
> really should not be taking any locks.
>=20
>> I'm worried about overhead of doing irq_work_queue() all the time.
>> But I'm not familiar with mechanism enough to justify the concerns.
>> Would it make sense to do s/in_nmi/irgs_disabled/ instead?
>=20
> irqs_disabled() should work in this particular case because rq->lock
> (and therefore all it's nested locks) are IRQ-safe.

We worry about the overhead of irq_work for every single stackmap=20
lookup. So we would like to go with the irqs_disabled() check. I just=20
sent v2 of the patch.=20

Thanks again,
Song