From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 176ECC43334 for ; Tue, 28 Jun 2022 14:54:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347460AbiF1OyI (ORCPT ); Tue, 28 Jun 2022 10:54:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32836 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347455AbiF1OyB (ORCPT ); Tue, 28 Jun 2022 10:54:01 -0400 Received: from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com [IPv6:2607:f8b0:4864:20::b2d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E3ED739A for ; Tue, 28 Jun 2022 07:53:59 -0700 (PDT) Received: by mail-yb1-xb2d.google.com with SMTP id x184so19899436ybg.12 for ; Tue, 28 Jun 2022 07:53:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vsLZ4itLb8yCme9zL9SXTWJXCTaMaHUvxCRr4pVmzPo=; b=hr+LOJOCejSbhs13LtB4ad2iVHUb+RAG9UvlGywImgCuWnbv9Uz0CuXpr7kdL2EXyM Zl3qZVecs/jCPbL36y5mfL0HhG/kKAAg5CMELOsO8zieEy+pumtY6i3i9lUzK6EDUaJc ij34SuQoLj/NiOxDwRIPbpShAuux9rUl4DMeRDPsX6BzEAEbfgiBkZR8fpdCYwEniTFB rUqH8uIe91EnJHMjEePoJ34++jQzFYkaVhwsw6SpYElF68QAdJJOFJ35ZmrerpdmqSoc KvqV3wMzW9sxWtFKFxX5f0P0Jvv4ZBX10XEVhIHB536OA3Q6fXumnmTBqPHUsHo1ZiVe 4+eQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vsLZ4itLb8yCme9zL9SXTWJXCTaMaHUvxCRr4pVmzPo=; b=xMc8W1OEDq6sFNoTYQZ93lVssisf1McB6AQcqIi4cHbFDnHRIyEbv/FWTHeZ673QqI uL5GQvkr0zOneGn0H6L7k6GIQ+ppPhPj134NM74qVTTpLiKye7TjshyHTlFNW6Ib5HXr YXdlthq08iizgGkcM5ip2+vPitrBChvf5Q4u3QTf7rQ0+plpOgZOD2e5xZSDELZjcN1K XWGwlMV5tn/MU1pBWu0w43VTnHCMIPsunAaw+ERBOxiHez1pu/PO05rxbhbyUmNNCs7M 3oxxihXubA4CBaQ2Uyl+NPSzLKx1H1QKnyzlnafobTOyjWjo06LGGqjPjPyHKuJ2COmg Ms5g== X-Gm-Message-State: AJIora8VEcpMVhhHzS0BUROPDw6yP79euWc8T7c86lHG3p2roPMwFyqF 6XvGRD7nLyf1xMJBpAdkxBdPDjfLc05LED9LrZG5pw== X-Google-Smtp-Source: AGRyM1vqDPdKqBHAN5BWgtuoqgS2YkXMiHwIjI7rhq0LUMqvW//SUblW3opNkhuDW9IWNxEP1SI0g+pLXiP+fSqQZuA= X-Received: by 2002:a5b:74e:0:b0:66c:df8d:12f6 with SMTP id s14-20020a5b074e000000b0066cdf8d12f6mr9546552ybq.609.1656428038960; Tue, 28 Jun 2022 07:53:58 -0700 (PDT) MIME-Version: 1.0 References: <20220628095833.2579903-1-elver@google.com> <20220628095833.2579903-4-elver@google.com> In-Reply-To: From: Marco Elver Date: Tue, 28 Jun 2022 16:53:22 +0200 Message-ID: Subject: Re: [PATCH v2 03/13] perf/hw_breakpoint: Optimize list of per-task breakpoints To: Dmitry Vyukov Cc: Peter Zijlstra , Frederic Weisbecker , Ingo Molnar , Thomas Gleixner , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Michael Ellerman , linuxppc-dev@lists.ozlabs.org, linux-perf-users@vger.kernel.org, x86@kernel.org, linux-sh@vger.kernel.org, kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org On Tue, 28 Jun 2022 at 15:08, Dmitry Vyukov wrote: > > On Tue, 28 Jun 2022 at 11:59, Marco Elver wrote: > > > > On a machine with 256 CPUs, running the recently added perf breakpoint > > benchmark results in: > > > > | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64 > > | # Running 'breakpoint/thread' benchmark: > > | # Created/joined 30 threads with 4 breakpoints and 64 parallelism > > | Total time: 236.418 [sec] > > | > > | 123134.794271 usecs/op > > | 7880626.833333 usecs/op/cpu > > > > The benchmark tests inherited breakpoint perf events across many > > threads. > > > > Looking at a perf profile, we can see that the majority of the time is > > spent in various hw_breakpoint.c functions, which execute within the > > 'nr_bp_mutex' critical sections which then results in contention on that > > mutex as well: > > > > 37.27% [kernel] [k] osq_lock > > 34.92% [kernel] [k] mutex_spin_on_owner > > 12.15% [kernel] [k] toggle_bp_slot > > 11.90% [kernel] [k] __reserve_bp_slot > > > > The culprit here is task_bp_pinned(), which has a runtime complexity of > > O(#tasks) due to storing all task breakpoints in the same list and > > iterating through that list looking for a matching task. Clearly, this > > does not scale to thousands of tasks. > > > > Instead, make use of the "rhashtable" variant "rhltable" which stores > > multiple items with the same key in a list. This results in average > > runtime complexity of O(1) for task_bp_pinned(). > > > > With the optimization, the benchmark shows: > > > > | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64 > > | # Running 'breakpoint/thread' benchmark: > > | # Created/joined 30 threads with 4 breakpoints and 64 parallelism > > | Total time: 0.208 [sec] > > | > > | 108.422396 usecs/op > > | 6939.033333 usecs/op/cpu > > > > On this particular setup that's a speedup of ~1135x. > > > > While one option would be to make task_struct a breakpoint list node, > > this would only further bloat task_struct for infrequently used data. > > Furthermore, after all optimizations in this series, there's no evidence > > it would result in better performance: later optimizations make the time > > spent looking up entries in the hash table negligible (we'll reach the > > theoretical ideal performance i.e. no constraints). > > > > Signed-off-by: Marco Elver > > --- > > v2: > > * Commit message tweaks. > > --- > > include/linux/perf_event.h | 3 +- > > kernel/events/hw_breakpoint.c | 56 ++++++++++++++++++++++------------- > > 2 files changed, 37 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index 01231f1d976c..e27360436dc6 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -36,6 +36,7 @@ struct perf_guest_info_callbacks { > > }; > > > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > > +#include > > #include > > #endif > > > > @@ -178,7 +179,7 @@ struct hw_perf_event { > > * creation and event initalization. > > */ > > struct arch_hw_breakpoint info; > > - struct list_head bp_list; > > + struct rhlist_head bp_list; > > }; > > #endif > > struct { /* amd_iommu */ > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c > > index 1b013968b395..add1b9c59631 100644 > > --- a/kernel/events/hw_breakpoint.c > > +++ b/kernel/events/hw_breakpoint.c > > @@ -26,10 +26,10 @@ > > #include > > #include > > #include > > -#include > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -54,7 +54,13 @@ static struct bp_cpuinfo *get_bp_info(int cpu, enum bp_type_idx type) > > } > > > > /* Keep track of the breakpoints attached to tasks */ > > -static LIST_HEAD(bp_task_head); > > +static struct rhltable task_bps_ht; > > +static const struct rhashtable_params task_bps_ht_params = { > > + .head_offset = offsetof(struct hw_perf_event, bp_list), > > + .key_offset = offsetof(struct hw_perf_event, target), > > + .key_len = sizeof_field(struct hw_perf_event, target), > > + .automatic_shrinking = true, > > +}; > > > > static int constraints_initialized; > > > > @@ -103,17 +109,23 @@ static unsigned int max_task_bp_pinned(int cpu, enum bp_type_idx type) > > */ > > static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type) > > { > > - struct task_struct *tsk = bp->hw.target; > > + struct rhlist_head *head, *pos; > > struct perf_event *iter; > > int count = 0; > > > > - list_for_each_entry(iter, &bp_task_head, hw.bp_list) { > > - if (iter->hw.target == tsk && > > - find_slot_idx(iter->attr.bp_type) == type && > > + rcu_read_lock(); > > + head = rhltable_lookup(&task_bps_ht, &bp->hw.target, task_bps_ht_params); > > + if (!head) > > + goto out; > > + > > + rhl_for_each_entry_rcu(iter, pos, head, hw.bp_list) { > > + if (find_slot_idx(iter->attr.bp_type) == type && > > (iter->cpu < 0 || cpu == iter->cpu)) > > count += hw_breakpoint_weight(iter); > > } > > > > +out: > > + rcu_read_unlock(); > > return count; > > } > > > > @@ -186,7 +198,7 @@ static void toggle_bp_task_slot(struct perf_event *bp, int cpu, > > /* > > * Add/remove the given breakpoint in our constraint table > > */ > > -static void > > +static int > > toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > int weight) > > { > > @@ -199,7 +211,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > /* Pinned counter cpu profiling */ > > if (!bp->hw.target) { > > get_bp_info(bp->cpu, type)->cpu_pinned += weight; > > - return; > > + return 0; > > } > > > > /* Pinned counter task profiling */ > > @@ -207,9 +219,9 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > toggle_bp_task_slot(bp, cpu, type, weight); > > > > if (enable) > > - list_add_tail(&bp->hw.bp_list, &bp_task_head); > > + return rhltable_insert(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params); > > else > > - list_del(&bp->hw.bp_list); > > + return rhltable_remove(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params); > > } > > > > __weak int arch_reserve_bp_slot(struct perf_event *bp) > > @@ -307,9 +319,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type) > > if (ret) > > return ret; > > > > - toggle_bp_slot(bp, true, type, weight); > > - > > - return 0; > > + return toggle_bp_slot(bp, true, type, weight); > > } > > > > int reserve_bp_slot(struct perf_event *bp) > > @@ -334,7 +344,7 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type) > > > > type = find_slot_idx(bp_type); > > weight = hw_breakpoint_weight(bp); > > - toggle_bp_slot(bp, false, type, weight); > > + WARN_ON(toggle_bp_slot(bp, false, type, weight)); > > } > > > > void release_bp_slot(struct perf_event *bp) > > @@ -678,7 +688,7 @@ static struct pmu perf_breakpoint = { > > int __init init_hw_breakpoint(void) > > { > > int cpu, err_cpu; > > - int i; > > + int i, ret; > > > > for (i = 0; i < TYPE_MAX; i++) > > nr_slots[i] = hw_breakpoint_slots(i); > > @@ -689,18 +699,24 @@ int __init init_hw_breakpoint(void) > > > > info->tsk_pinned = kcalloc(nr_slots[i], sizeof(int), > > GFP_KERNEL); > > - if (!info->tsk_pinned) > > - goto err_alloc; > > + if (!info->tsk_pinned) { > > + ret = -ENOMEM; > > + goto err; > > + } > > } > > } > > > > + ret = rhltable_init(&task_bps_ht, &task_bps_ht_params); > > + if (ret) > > + goto err; > > + > > constraints_initialized = 1; > > > > perf_pmu_register(&perf_breakpoint, "breakpoint", PERF_TYPE_BREAKPOINT); > > > > return register_die_notifier(&hw_breakpoint_exceptions_nb); > > It seems there is a latent bug here: > if register_die_notifier() fails we also need to execute the err: label code. I think we should ignore it, because it's just a notifier when the kernel dies. I'd rather have working breakpoints (which we have if we made it to this point) when the kernel is live, and sacrifice some bad behaviour when the kernel dies. > Otherwise the patch looks good. > > Reviewed-by: Dmitry Vyukov Thanks, -- Marco From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D16F3C433EF for ; Tue, 28 Jun 2022 14:54:40 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4LXSL73ntxz3f2c for ; Wed, 29 Jun 2022 00:54:39 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=hr+LOJOC; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=google.com (client-ip=2607:f8b0:4864:20::b31; helo=mail-yb1-xb31.google.com; envelope-from=elver@google.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.a=rsa-sha256 header.s=20210112 header.b=hr+LOJOC; dkim-atps=neutral Received: from mail-yb1-xb31.google.com (mail-yb1-xb31.google.com [IPv6:2607:f8b0:4864:20::b31]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4LXSKQ3jbQz3c1b for ; Wed, 29 Jun 2022 00:54:02 +1000 (AEST) Received: by mail-yb1-xb31.google.com with SMTP id v185so13119455ybe.8 for ; Tue, 28 Jun 2022 07:54:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=vsLZ4itLb8yCme9zL9SXTWJXCTaMaHUvxCRr4pVmzPo=; b=hr+LOJOCejSbhs13LtB4ad2iVHUb+RAG9UvlGywImgCuWnbv9Uz0CuXpr7kdL2EXyM Zl3qZVecs/jCPbL36y5mfL0HhG/kKAAg5CMELOsO8zieEy+pumtY6i3i9lUzK6EDUaJc ij34SuQoLj/NiOxDwRIPbpShAuux9rUl4DMeRDPsX6BzEAEbfgiBkZR8fpdCYwEniTFB rUqH8uIe91EnJHMjEePoJ34++jQzFYkaVhwsw6SpYElF68QAdJJOFJ35ZmrerpdmqSoc KvqV3wMzW9sxWtFKFxX5f0P0Jvv4ZBX10XEVhIHB536OA3Q6fXumnmTBqPHUsHo1ZiVe 4+eQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=vsLZ4itLb8yCme9zL9SXTWJXCTaMaHUvxCRr4pVmzPo=; b=hEAWQ3tvVA9aTUKKeExsbF1iq7ncR7H+TQ9xrFG13umYbNnRZ8psu6hEBa+MamvAVN 7y6LYD/eJX6YFcTlTtbBlHtZOOXpGOlohmC+2ihuUl0Y0gvkb1AQ3uIfRjRUSNdii8Ev Yl9A6cHT2Lt5q2EP04mVl5GiaNUBxMeeyJMpQwtfSHhX4bHy4veK03MNgbUYfNa2sM01 PQTo4uEN7QYfw/eBtvTCV28xSw+rnbbnogNo/jcP8/XdtzkLNO3TBPN/L/kkzGTEOawa QIpp5wxEh5mkgOmmYSMiC2Zl3JFkC+YAz4B/Ky0VCAa8UpXuqOI5nLIbzhlED8k5pOML ZM8Q== X-Gm-Message-State: AJIora8YPH4NNQM69er5Et4M8398kwx0RGbv7q7aOqE08W+jnmivOu6q 1SbTKCYYLgkYvVxdEN5JlcM3lzUaDkRSn5IcNCNyxw== X-Google-Smtp-Source: AGRyM1vqDPdKqBHAN5BWgtuoqgS2YkXMiHwIjI7rhq0LUMqvW//SUblW3opNkhuDW9IWNxEP1SI0g+pLXiP+fSqQZuA= X-Received: by 2002:a5b:74e:0:b0:66c:df8d:12f6 with SMTP id s14-20020a5b074e000000b0066cdf8d12f6mr9546552ybq.609.1656428038960; Tue, 28 Jun 2022 07:53:58 -0700 (PDT) MIME-Version: 1.0 References: <20220628095833.2579903-1-elver@google.com> <20220628095833.2579903-4-elver@google.com> In-Reply-To: From: Marco Elver Date: Tue, 28 Jun 2022 16:53:22 +0200 Message-ID: Subject: Re: [PATCH v2 03/13] perf/hw_breakpoint: Optimize list of per-task breakpoints To: Dmitry Vyukov Content-Type: text/plain; charset="UTF-8" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , linux-sh@vger.kernel.org, Peter Zijlstra , Frederic Weisbecker , x86@kernel.org, linuxppc-dev@lists.ozlabs.org, Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, Alexander Shishkin , kasan-dev@googlegroups.com, Namhyung Kim , Thomas Gleixner , Jiri Olsa , Ingo Molnar Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Tue, 28 Jun 2022 at 15:08, Dmitry Vyukov wrote: > > On Tue, 28 Jun 2022 at 11:59, Marco Elver wrote: > > > > On a machine with 256 CPUs, running the recently added perf breakpoint > > benchmark results in: > > > > | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64 > > | # Running 'breakpoint/thread' benchmark: > > | # Created/joined 30 threads with 4 breakpoints and 64 parallelism > > | Total time: 236.418 [sec] > > | > > | 123134.794271 usecs/op > > | 7880626.833333 usecs/op/cpu > > > > The benchmark tests inherited breakpoint perf events across many > > threads. > > > > Looking at a perf profile, we can see that the majority of the time is > > spent in various hw_breakpoint.c functions, which execute within the > > 'nr_bp_mutex' critical sections which then results in contention on that > > mutex as well: > > > > 37.27% [kernel] [k] osq_lock > > 34.92% [kernel] [k] mutex_spin_on_owner > > 12.15% [kernel] [k] toggle_bp_slot > > 11.90% [kernel] [k] __reserve_bp_slot > > > > The culprit here is task_bp_pinned(), which has a runtime complexity of > > O(#tasks) due to storing all task breakpoints in the same list and > > iterating through that list looking for a matching task. Clearly, this > > does not scale to thousands of tasks. > > > > Instead, make use of the "rhashtable" variant "rhltable" which stores > > multiple items with the same key in a list. This results in average > > runtime complexity of O(1) for task_bp_pinned(). > > > > With the optimization, the benchmark shows: > > > > | $> perf bench -r 30 breakpoint thread -b 4 -p 64 -t 64 > > | # Running 'breakpoint/thread' benchmark: > > | # Created/joined 30 threads with 4 breakpoints and 64 parallelism > > | Total time: 0.208 [sec] > > | > > | 108.422396 usecs/op > > | 6939.033333 usecs/op/cpu > > > > On this particular setup that's a speedup of ~1135x. > > > > While one option would be to make task_struct a breakpoint list node, > > this would only further bloat task_struct for infrequently used data. > > Furthermore, after all optimizations in this series, there's no evidence > > it would result in better performance: later optimizations make the time > > spent looking up entries in the hash table negligible (we'll reach the > > theoretical ideal performance i.e. no constraints). > > > > Signed-off-by: Marco Elver > > --- > > v2: > > * Commit message tweaks. > > --- > > include/linux/perf_event.h | 3 +- > > kernel/events/hw_breakpoint.c | 56 ++++++++++++++++++++++------------- > > 2 files changed, 37 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > index 01231f1d976c..e27360436dc6 100644 > > --- a/include/linux/perf_event.h > > +++ b/include/linux/perf_event.h > > @@ -36,6 +36,7 @@ struct perf_guest_info_callbacks { > > }; > > > > #ifdef CONFIG_HAVE_HW_BREAKPOINT > > +#include > > #include > > #endif > > > > @@ -178,7 +179,7 @@ struct hw_perf_event { > > * creation and event initalization. > > */ > > struct arch_hw_breakpoint info; > > - struct list_head bp_list; > > + struct rhlist_head bp_list; > > }; > > #endif > > struct { /* amd_iommu */ > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c > > index 1b013968b395..add1b9c59631 100644 > > --- a/kernel/events/hw_breakpoint.c > > +++ b/kernel/events/hw_breakpoint.c > > @@ -26,10 +26,10 @@ > > #include > > #include > > #include > > -#include > > #include > > #include > > #include > > +#include > > #include > > #include > > > > @@ -54,7 +54,13 @@ static struct bp_cpuinfo *get_bp_info(int cpu, enum bp_type_idx type) > > } > > > > /* Keep track of the breakpoints attached to tasks */ > > -static LIST_HEAD(bp_task_head); > > +static struct rhltable task_bps_ht; > > +static const struct rhashtable_params task_bps_ht_params = { > > + .head_offset = offsetof(struct hw_perf_event, bp_list), > > + .key_offset = offsetof(struct hw_perf_event, target), > > + .key_len = sizeof_field(struct hw_perf_event, target), > > + .automatic_shrinking = true, > > +}; > > > > static int constraints_initialized; > > > > @@ -103,17 +109,23 @@ static unsigned int max_task_bp_pinned(int cpu, enum bp_type_idx type) > > */ > > static int task_bp_pinned(int cpu, struct perf_event *bp, enum bp_type_idx type) > > { > > - struct task_struct *tsk = bp->hw.target; > > + struct rhlist_head *head, *pos; > > struct perf_event *iter; > > int count = 0; > > > > - list_for_each_entry(iter, &bp_task_head, hw.bp_list) { > > - if (iter->hw.target == tsk && > > - find_slot_idx(iter->attr.bp_type) == type && > > + rcu_read_lock(); > > + head = rhltable_lookup(&task_bps_ht, &bp->hw.target, task_bps_ht_params); > > + if (!head) > > + goto out; > > + > > + rhl_for_each_entry_rcu(iter, pos, head, hw.bp_list) { > > + if (find_slot_idx(iter->attr.bp_type) == type && > > (iter->cpu < 0 || cpu == iter->cpu)) > > count += hw_breakpoint_weight(iter); > > } > > > > +out: > > + rcu_read_unlock(); > > return count; > > } > > > > @@ -186,7 +198,7 @@ static void toggle_bp_task_slot(struct perf_event *bp, int cpu, > > /* > > * Add/remove the given breakpoint in our constraint table > > */ > > -static void > > +static int > > toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > int weight) > > { > > @@ -199,7 +211,7 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > /* Pinned counter cpu profiling */ > > if (!bp->hw.target) { > > get_bp_info(bp->cpu, type)->cpu_pinned += weight; > > - return; > > + return 0; > > } > > > > /* Pinned counter task profiling */ > > @@ -207,9 +219,9 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type, > > toggle_bp_task_slot(bp, cpu, type, weight); > > > > if (enable) > > - list_add_tail(&bp->hw.bp_list, &bp_task_head); > > + return rhltable_insert(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params); > > else > > - list_del(&bp->hw.bp_list); > > + return rhltable_remove(&task_bps_ht, &bp->hw.bp_list, task_bps_ht_params); > > } > > > > __weak int arch_reserve_bp_slot(struct perf_event *bp) > > @@ -307,9 +319,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type) > > if (ret) > > return ret; > > > > - toggle_bp_slot(bp, true, type, weight); > > - > > - return 0; > > + return toggle_bp_slot(bp, true, type, weight); > > } > > > > int reserve_bp_slot(struct perf_event *bp) > > @@ -334,7 +344,7 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type) > > > > type = find_slot_idx(bp_type); > > weight = hw_breakpoint_weight(bp); > > - toggle_bp_slot(bp, false, type, weight); > > + WARN_ON(toggle_bp_slot(bp, false, type, weight)); > > } > > > > void release_bp_slot(struct perf_event *bp) > > @@ -678,7 +688,7 @@ static struct pmu perf_breakpoint = { > > int __init init_hw_breakpoint(void) > > { > > int cpu, err_cpu; > > - int i; > > + int i, ret; > > > > for (i = 0; i < TYPE_MAX; i++) > > nr_slots[i] = hw_breakpoint_slots(i); > > @@ -689,18 +699,24 @@ int __init init_hw_breakpoint(void) > > > > info->tsk_pinned = kcalloc(nr_slots[i], sizeof(int), > > GFP_KERNEL); > > - if (!info->tsk_pinned) > > - goto err_alloc; > > + if (!info->tsk_pinned) { > > + ret = -ENOMEM; > > + goto err; > > + } > > } > > } > > > > + ret = rhltable_init(&task_bps_ht, &task_bps_ht_params); > > + if (ret) > > + goto err; > > + > > constraints_initialized = 1; > > > > perf_pmu_register(&perf_breakpoint, "breakpoint", PERF_TYPE_BREAKPOINT); > > > > return register_die_notifier(&hw_breakpoint_exceptions_nb); > > It seems there is a latent bug here: > if register_die_notifier() fails we also need to execute the err: label code. I think we should ignore it, because it's just a notifier when the kernel dies. I'd rather have working breakpoints (which we have if we made it to this point) when the kernel is live, and sacrifice some bad behaviour when the kernel dies. > Otherwise the patch looks good. > > Reviewed-by: Dmitry Vyukov Thanks, -- Marco