Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Song Liu <songliubraving@meta.com>
To:     Andrew Morton <akpm@linux-foundation.org>
CC:     Song Liu <song@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Kernel Team <kernel-team@meta.com>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH] watchdog: Prefer use "ref-cycles" for NMI watchdog
Thread-Topic: [PATCH] watchdog: Prefer use "ref-cycles" for NMI watchdog
Thread-Index: AQHZgsQGTR6REoP6vEGzK72DJVVZQK9XUNwAgAAGZAA=
Date:   Sat, 13 May 2023 00:03:58 +0000
Message-ID: <53D16C28-5489-4DFD-B8B3-6B20517A941D@fb.com>
References: <20230509221700.859865-1-song@kernel.org>
 <20230512164056.8f1e4e23032f7f7f5cb69df0@linux-foundation.org>
In-Reply-To: <20230512164056.8f1e4e23032f7f7f5cb69df0@linux-foundation.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Mppq9V0zxh3EPYnLTK1wY1lQhR60iSAYYYXYE+y8f8V4VzR48j6ZThHyX9Fp?=
 =?us-ascii?Q?5O5+/OeqoK0pX6DoI41Khx2gC+O37s6oQuCayRZbBt0HDmcvDiRi2CU8JfYh?=
 =?us-ascii?Q?iUtOAxFhhNQ0dbkvCyC4koigDIHqh+9nXx1z4ItaHAQH6ayUPzdmBtxVP+uV?=
 =?us-ascii?Q?FDijg7mej8k7uAEJjNwRPVbYtDXEUJoira6UQztkmpWYkOHk6N66yC5T6V2p?=
 =?us-ascii?Q?ZW0yJUkSbglKquC/l/DAWspnVnqPAqmhXCKWLxAFwS72ww9SqB1iekUhs1W2?=
 =?us-ascii?Q?ESrKwKttZReq0JOCE7SCOwcW8LuhgiQyGK3L/kycAIJY7cbtLMszt1gVoWuy?=
 =?us-ascii?Q?kx+G2NsObeye7nlir3BbudZFwGNmRV5cbdv08TqTzgT6PsXGenT7rK5nIVGU?=
 =?us-ascii?Q?2ih5KUvuN+2sAwdZUmNgMKGfnM85RNVkmi/e04r2daAx+d7njOYfEpLUMqUR?=
 =?us-ascii?Q?4Tmk7ntk4+IdEvZR9OMvbxw2j/n6dXyCzDOjliNLe2QEHpkSYivBTquc01gi?=
 =?us-ascii?Q?T5BW9QCiTra4oDbn7XW6PXPsUsRZdSA4wf4RNW0bLkLsOoRvWGrXoWvI1MGF?=
 =?us-ascii?Q?8SDfvWvnhXdK0VV2+GOO2r8YyYYcH0wMEGhVUH7ir6Wlu7kfga7trZy5BV6y?=
 =?us-ascii?Q?5utEY4HJX4GaVMHLb+yoEoAMGa8zjPef/Ug4iYHYWircfjDx92qgaC+632tt?=
 =?us-ascii?Q?/HXewxeQCU8Mdr0lei3DY8jg+/Ix4vmlf6pcv0LIqb4fKrDiK+Ca7grA4StV?=
 =?us-ascii?Q?OR4TUuokRaaxpKun0EuvYZuVIqNDizauLsfIm9strRqkad9R4bAOcV9olhY1?=
 =?us-ascii?Q?yAc4yjizgg9uh17eLUIqjTQ2khJ7gwB4AYy/2hqCxCXDeXNADUTSN31tIOiA?=
 =?us-ascii?Q?ljJL1HXGoHqPu+2tSjbRlsMhbgGdo8zomsIsN+8bR4ayM7rGgxmKg4g2DY3o?=
 =?us-ascii?Q?Iz6v7G+FStRnqECbx5dchhIE9lX5v3TSiK7m2xke0Q0MkaeEFOv8/uHynEJe?=
 =?us-ascii?Q?C06LdjPibCKwsYciLgWf+3gr7sfj4ljhw7P752PmivpCf3uRNeSMb+smkP6D?=
 =?us-ascii?Q?/LLl+plIGl/arBulvvJjy/yMOZfEWOfO1a2g2Chc+hDDtPFPrtTSu03mt6as?=
 =?us-ascii?Q?B5i5a4NSIMCKksvct6OrHw2BUkkLNhIkUkS6gPyvvZeZHrgrfcQPzbcSTTU4?=
 =?us-ascii?Q?R+gAY+3G7Yn8/IHLPc++Oc4ChVyTmWk7WND8cAeU5SMyk5IYPT/YaQIbPG8W?=
 =?us-ascii?Q?i369L5riuzuroCDcIhePYZc+sXB2fqaeTNYg1uOp7INDjQNitIAyJgm23LN4?=
 =?us-ascii?Q?SLj89w44AbihTVFWmQZil3FGdHwQu4bz5frZq0uN8pYbemezMu59uF6ebhbq?=
 =?us-ascii?Q?dlVh/GRnQwgVkFdmc5dK5acnFzTXOLD/ydXxZVkyPLmXiZmyC3NdXT/QjEzK?=
 =?us-ascii?Q?PiLCIXB+pb560iJu0h+y2tuBnIqdnBqOC+WHeBQusnFHCTQSIDg5bAgKPEv5?=
 =?us-ascii?Q?B0CNIFnlWZPL9i+IAd3r7HRVEC5imJqwwl6E1Fm3JDMxsXLz6+R8O0hhkWv3?=
 =?us-ascii?Q?jUk1lKnzQjHoak30j2Tlfp8soFeI9f271iIg3hRv4JOzkIDK3joKrbxfznYq?=
 =?us-ascii?Q?/V1MAf/nX1TDl/8Y1IGOf5Y=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <A91E00C91E0C95448256A6DCFB751138@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c7397700-9cb4-46c4-dae0-08db53458d12
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 May 2023 00:03:58.7281
 (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: d8gSaS44TRr7of73w0VL8TnoemHo8vPn46dGd7NlFIe4vs9oPMoXpR4pSynw9dVoDv/XeJWBdexpJOWzvhn15A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR15MB5860
X-Proofpoint-ORIG-GUID: p_G2eUW5LEqSKmVQ8Gte1adYVSI41ikg
X-Proofpoint-GUID: p_G2eUW5LEqSKmVQ8Gte1adYVSI41ikg
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-12_16,2023-05-05_01,2023-02-09_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On May 12, 2023, at 4:40 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Tue, 9 May 2023 15:17:00 -0700 Song Liu <song@kernel.org> wrote:
> 
>> NMI watchdog permanently consumes one hardware counters per CPU on the
>> system. For systems that use many hardware counters, this causes more
>> aggressive time multiplexing of perf events.
>> 
>> OTOH, some CPUs (mostly Intel) support "ref-cycles" event, which is rarely
>> used. Try use "ref-cycles" for the watchdog. If the CPU supports it, so
>> that one more hardware counter is available to the user. If the CPU doesn't
>> support "ref-cycles", fall back to "cycles".
>> 
>> The downside of this change is that users of "ref-cycles" need to disable
>> nmi_watchdog.
>> 
>> ...
>> 
>> @@ -286,6 +286,12 @@ int __init hardlockup_detector_perf_init(void)
>> {
>> int ret = hardlockup_detector_event_create();
>> 
>> + if (ret) {
> 
> If we get here, hardlockup_detector_event_create() has sent a scary
> pr_debug message.
> 
>> + /* Failed to create "ref-cycles", try "cycles" instead */
>> + wd_hw_attr.config = PERF_COUNT_HW_CPU_CYCLES;
>> + ret = hardlockup_detector_event_create();
> 
> So it would be good to emit a followup message here telling users that
> things are OK.  Or tell the user we're retrying with a different
> counter, etc.

How about we ask hardlockup_detector_event_create() not to send pr_debug
message in the first try (something like below)?

Also, I think Peter's concern is valid. If some user daemon monitors 
ref-cycles in the background (I am not aware of such use cases though), 
this could be a real issue. 

Thanks,
Song


diff --git i/kernel/watchdog_hld.c w/kernel/watchdog_hld.c
index f77109d98641..a1d2a43ea31f 100644
--- i/kernel/watchdog_hld.c
+++ w/kernel/watchdog_hld.c
@@ -163,7 +163,7 @@ static void watchdog_overflow_callback(struct perf_event *event,
        return;
 }

-static int hardlockup_detector_event_create(void)
+static int hardlockup_detector_event_create(bool send_warning)
 {
        unsigned int cpu = smp_processor_id();
        struct perf_event_attr *wd_attr;
@@ -176,8 +176,10 @@ static int hardlockup_detector_event_create(void)
        evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
                                               watchdog_overflow_callback, NULL);
        if (IS_ERR(evt)) {
-               pr_debug("Perf event create on CPU %d failed with %ld\n", cpu,
-                        PTR_ERR(evt));
+               if (send_warning) {
+                       pr_debug("Perf event create on CPU %d failed with %ld\n", cpu,
+                                PTR_ERR(evt));
+               }
                return PTR_ERR(evt);
        }
        this_cpu_write(watchdog_ev, evt);
@@ -189,7 +191,7 @@ static int hardlockup_detector_event_create(void)
  */
 void hardlockup_detector_perf_enable(void)
 {
-       if (hardlockup_detector_event_create())
+       if (hardlockup_detector_event_create(true))
                return;

        /* use original value for check */
@@ -284,12 +286,12 @@ void __init hardlockup_detector_perf_restart(void)
  */
 int __init hardlockup_detector_perf_init(void)
 {
-       int ret = hardlockup_detector_event_create();
+       int ret = hardlockup_detector_event_create(false);

        if (ret) {
                /* Failed to create "ref-cycles", try "cycles" instead */
                wd_hw_attr.config = PERF_COUNT_HW_CPU_CYCLES;
-               ret = hardlockup_detector_event_create();
+               ret = hardlockup_detector_event_create(true);
        }

        if (ret) {