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 6BBE1C4332F for ; Wed, 23 Nov 2022 14:12:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238000AbiKWOMN (ORCPT ); Wed, 23 Nov 2022 09:12:13 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35488 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236038AbiKWOMJ (ORCPT ); Wed, 23 Nov 2022 09:12:09 -0500 Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 72AC3238 for ; Wed, 23 Nov 2022 06:12:08 -0800 (PST) Received: by mail-wm1-x332.google.com with SMTP id t1so13136249wmi.4 for ; Wed, 23 Nov 2022 06:12:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=arista.com; s=google; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Bws7Utd85LMsHk1OFS5y1Jt3azqf33BIHbl8d6KTHuQ=; b=D+wYqLDP1MEA7jCYynzAsSVMjisFupGWEdxCzQFgAuLl/j7P7VMONsmjag6HspHkDv 2Hb7jLfBYBKn7EXSFEe3S39UkuY40vBMpDWg4yQJ4DUQJ2YzXHc5fdHK/JikDPxeLKxO echf0BlGjUZhObFuWaVKRFMbuGyKOaQyUfzZ5RSdHkDArwX/vjxyuFeGEnfPpd2yOQz5 s6JTEbHITuLdXFNGDf8zpWu7spCPT+9jtpiRGHW4pmypT2XUM/EKUobQXJYypI6Vm0Ol b0rOtA2VPArOraLFkReBtfw21uDCV+3qXlv6/BoK+zYCKDvGg/JYisGHDWZ3Nx59gFVi 1P0A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Bws7Utd85LMsHk1OFS5y1Jt3azqf33BIHbl8d6KTHuQ=; b=gxEbtK21uxeCoaN8W8VelV3AvqeP1RALvFcsENKHfQRIzTydQJUVU1947DQ3kySSD0 bbg2UjMh78gPE1+Qf+3V8iSFPqnwOBuhrgZSB2Ru6raeifi3UkvcwCn0PeiudSyUn4MO 5MIRSuVUZ9rsnbHaZ0RsGDvQo1mjzupi9jZ5HHiYW72FZ//R2+y+fuHPuLYj2o4CD+lg zH9npBvFjJQQGzmuoLKIP7KE+nkXq16akfjsAAcjp+3CKbn6X0J1NhZekQmjb9YJaraE OIQ8w+B+oouFGINi0CTKOjiV4vg/VDHTw0nXuU2McsKwUhJPGKfR5MtxKUEtpDUKtloU kDsg== X-Gm-Message-State: ANoB5pmLrpZfn3kB3CJUi3mVif8hLDWE9vhcycHpJ0d5P8d7IKqKxNEy E0lvJ7rDBD/JDyeXFRV+XcVMVw== X-Google-Smtp-Source: AA0mqf6Y0ju2xImGL+9QBlfOHXw/7y97CmSePiN/N1bGOjf5pSx013+27gKxLKN4IqD5oXnpuuoyFA== X-Received: by 2002:a05:600c:2302:b0:3cf:a3c4:59b3 with SMTP id 2-20020a05600c230200b003cfa3c459b3mr12643453wmo.198.1669212726904; Wed, 23 Nov 2022 06:12:06 -0800 (PST) Received: from [10.83.37.24] ([217.173.96.166]) by smtp.gmail.com with ESMTPSA id y7-20020a1c4b07000000b003b4c979e6bcsm2345676wma.10.2022.11.23.06.12.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 23 Nov 2022 06:12:06 -0800 (PST) Message-ID: <9e730aaf-9bbb-38d4-c26f-dfc58c4a9352@arista.com> Date: Wed, 23 Nov 2022 14:11:59 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH v5 1/5] jump_label: Prevent key->enabled int overflow Content-Language: en-US To: Peter Zijlstra Cc: linux-kernel@vger.kernel.org, David Ahern , Eric Dumazet , Ard Biesheuvel , Bob Gilligan , "David S. Miller" , Dmitry Safonov <0x7f454c46@gmail.com>, Francesco Ruggeri , Hideaki YOSHIFUJI , Jakub Kicinski , Jason Baron , Josh Poimboeuf , Paolo Abeni , Salam Noureddine , Steven Rostedt , netdev@vger.kernel.org References: <20221122185534.308643-1-dima@arista.com> <20221122185534.308643-2-dima@arista.com> From: Dmitry Safonov In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/23/22 09:55, Peter Zijlstra wrote: > On Tue, Nov 22, 2022 at 06:55:30PM +0000, Dmitry Safonov wrote: > >> +/*** >> + * static_key_fast_inc_not_negative - adds a user for a static key >> + * @key: static key that must be already enabled >> + * >> + * The caller must make sure that the static key can't get disabled while >> + * in this function. It doesn't patch jump labels, only adds a user to >> + * an already enabled static key. >> + * >> + * Returns true if the increment was done. >> + */ > > I don't normally do kerneldoc style comments, and this is the first in > the whole file. The moment I get a docs person complaining about some > markup issue I just take the ** off. The only reason I used kerneldoc style is that otherwise usually someone would come and complain. I'll convert it to a regular comment. > One more thing; it might be useful to point out that unlike refcount_t > this thing does not saturate but will fail to increment on overflow. Will add it as well. > >> +static bool static_key_fast_inc_not_negative(struct static_key *key) >> { >> + int v; >> + >> STATIC_KEY_CHECK_USE(key); >> + /* >> + * Negative key->enabled has a special meaning: it sends >> + * static_key_slow_inc() down the slow path, and it is non-zero >> + * so it counts as "enabled" in jump_label_update(). Note that >> + * atomic_inc_unless_negative() checks >= 0, so roll our own. >> + */ >> + v = atomic_read(&key->enabled); >> + do { >> + if (v <= 0 || (v + 1) < 0) >> + return false; >> + } while (!likely(atomic_try_cmpxchg(&key->enabled, &v, v + 1))); >> + >> + return true; >> +} > > ( vexing how this function and the JUMP_LABEL=n static_key_slow_inc() are > only a single character different ) Yeah, also another reason for it was that when JUMP_LABEL=y jump_label.h doesn't include and because of the inclusion hell: commit 1f69bf9c6137 ("jump_label: remove bug.h, atomic.h dependencies for HAVE_JUMP_LABEL") and I can't move JUMP_LABEL=n version of static_key_slow_inc() to jump_label.c as it is not being built without the config set. So, in result I was looking into macro-define for both cases, but that adds quite some ugliness and has no type checks for just reusing 10 lines, where 1 differs... > So while strictly accurate, I dislike this name (and I see I was not > quick enough responding to your earlier suggestion :/). The whole > negative thing is an implementation detail that should not spread > outside of jump_label.c. > > Since you did not like the canonical _inc_not_zero(), how about > inc_not_disabled() ? Ok, that sounds good, I'll rename in v6. > Also, perhaps expose this function in this patch, instead of hiding that > in patch 3? Will do. > Otherwise, things look good. > > Thanks! Thanks again for the review, Dmitry