From owner-freebsd-arch@freebsd.org Mon Apr 30 16:36:07 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 531C3FB00FB for ; Mon, 30 Apr 2018 16:36:07 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id E345A6AAED for ; Mon, 30 Apr 2018 16:36:06 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: by mailman.ysv.freebsd.org (Postfix) id A71E4FB00F9; Mon, 30 Apr 2018 16:36:06 +0000 (UTC) Delivered-To: arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 94F38FB00F8 for ; Mon, 30 Apr 2018 16:36:06 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from smtp.freebsd.org (unknown [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "smtp.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 472D96AAD5; Mon, 30 Apr 2018 16:36:06 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (ralph.baldwin.cx [66.234.199.215]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 21BEA21FC8; Mon, 30 Apr 2018 16:36:06 +0000 (UTC) (envelope-from jhb@freebsd.org) From: John Baldwin To: Warner Losh Cc: "freebsd-arch@freebsd.org" Subject: Re: LIBC_SCCS Date: Mon, 30 Apr 2018 08:28:24 -0700 Message-ID: <10594521.E4XjG3cQAG@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.1-STABLE; KDE/4.14.30; amd64; ; ) In-Reply-To: References: <1711113.VelFtdTVS7@ralph.baldwin.cx> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 30 Apr 2018 16:36:07 -0000 On Friday, April 27, 2018 05:36:56 PM Warner Losh wrote: > On Fri, Apr 27, 2018 at 4:19 PM, John Baldwin wrote: > > > I suspect no one cares, but for whatever reason our current handling of the > > LIBC_SCCS macro in some of our libraries annoys me. In theory it seems > > like > > LIBC_SCCS's purpose is to control whether or not old SCCS IDs from Berkeley > > are included in libc's sources when libc is built. (Similar to how macros > > control the behavior of __FBSDID().) However, we use an odd construct in > > the tree. First, we define LIBC_SCCS by default in the CFLAGS of various > > libraries (libkvm, libutil, libthr, libc, etc.) which in theory would > > enable > > the IDs, but then we explicitly wrap them in #if 0, e.g.: > > > > #if defined(LIBC_SCCS) && !defined(lint) > > #if 0 > > static char sccsid[] = "@(#)kvm_hp300.c 8.1 (Berkeley) 6/4/93"; > > #endif > > #endif /* LIBC_SCCS and not lint */ > > > > I'd rather that we make LIBC_SCCS actually work by removing the #if 0 (and > > perhaps the lint baggage) but then remove it from the default CFLAGS to > > preserve the existing behavior by default. Does anyone else care if I do > > this? > > > > I'm cool with it. Why not do __SCCS_ID( "@(#)kvm_hp300.c 8.1 (Berkeley) > 6/4/93");? I probably would use that, though perhaps still wrapped in the #ifdef so we don't enable them by default as __SCCS_ID is enabled by default. Alternatively we could drop LIBC_SCCS entirely and define NO__SCCSID by default for the affected libraries? > I don't know if we need a separate #ifdef for each SCCS_ID subtree in our > build. Either it's there, or it's not. Default: not. It would also let us > put them in separate sections ala our freebsd id macros. > > I'm slightly against removing it altogether, though I can see a good case > for it. I have a visceral reaction that puts me in the 'against complete > removal' camp, but only just. I probably lean towards not outright removing them, but I don't feel too strongly. -- John Baldwin From owner-freebsd-arch@freebsd.org Tue May 1 17:38:22 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 75108FB1964 for ; Tue, 1 May 2018 17:38:22 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 056F37CB22 for ; Tue, 1 May 2018 17:38:22 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-io0-x229.google.com with SMTP id z4-v6so14464486iof.5 for ; Tue, 01 May 2018 10:38:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=3f+VC7234ELXxi15r/cbzaPwTJ3rHTaJvcMmx3j/dps=; b=Zb54VfrmQWzUgPtZEtsYRliJtFjaI7jN74yyZ7HSg+lIu25K1BaWwccYWYYLGTEk8/ GMiW9p5jUZ7rz5CEcgWHwwffYoGfyUeHXZ6ZAwSDr7Nne1KIFeEK8QMHP58sW1SgF162 B2v8+qycd2qADv4XydPcM2U4Amyi/9VbqepIu7qvMlGHDNxg+zEufpEqSDMtXmJSSy9p Yi57HtsQIuo1ZdfK+ttf8Bdhk8fJH2epWR5y7C/HeCvLwrEVy8Ze/G6g/LTkWh56Xcki tZDgyFvAqLUopqaZhSgdiA4MggjHe9+BpZ6NO//8EdMhm+VmquVcXdL2oN9hFEvX3n7m oXqQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=3f+VC7234ELXxi15r/cbzaPwTJ3rHTaJvcMmx3j/dps=; b=pD/DQ4aWxtcXa8QwSK2ONVo9oIJHgQsZ27dRmEuaxwYbrbxGskBkQfQyUqD75qC5IX b7JyPyJ+XX2azicvjSGXRxp9JKxsJTj4IpQWNQCsOE2faBjVa7Y1/CaaeBk2DEDxWyvv AOh/GAFjHEInoEUtVP2k/CJ9Ow+XM9ws+ajBUesl5tlzoMxIIpmUl8vU8VNQ/dAVSni0 vwhuQTqzPia6h3ZURXu1NzGedoT38dt0Hfsh1OshDBNqjBOzB8I4jOeb7XNHIy7gyJmW /aEVkfJ4cEVbwemYAb3FFR+uAqKnzUR+7FyUwDbGsWh8dzyqQYm4ffDss/6tD0QmdVNK MMNQ== X-Gm-Message-State: ALQs6tBnILCNrFP5yw2ajdJBXMDeKBUcNyA69NryZccoWWcj6vTtMMPs ce3X8IrdhaROQ2/spM0nhAD4qQ== X-Google-Smtp-Source: AB8JxZrNt9nwxMf0Kug0xHCECia0Cxn9tYyjqSMaebEMOCQT3FxE3RNaoxDDkUMCz5Kt7d0uiVvabg== X-Received: by 2002:a6b:fd0:: with SMTP id 77-v6mr18423402iop.108.1525196300995; Tue, 01 May 2018 10:38:20 -0700 (PDT) Received: from raichu (toroon0560w-lp130-04-184-145-252-74.dsl.bell.ca. [184.145.252.74]) by smtp.gmail.com with ESMTPSA id e18-v6sm4884237itc.3.2018.05.01.10.38.19 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 01 May 2018 10:38:20 -0700 (PDT) Sender: Mark Johnston Date: Tue, 1 May 2018 13:38:17 -0400 From: Mark Johnston To: freebsd-arch@FreeBSD.org Subject: Re: CFT: netdump Message-ID: <20180501173817.GB47315@raichu> References: <20180411210445.GC43015@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180411210445.GC43015@raichu> User-Agent: Mutt/1.9.5 (2018-04-13) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 May 2018 17:38:22 -0000 On Wed, Apr 11, 2018 at 05:04:45PM -0400, Mark Johnston wrote: > Hi, > > In user/markj/netdump I've been working on a feature which allows the > kernel to transmit kernel dumps over a network rather than writing to a > local storage device. This is convenient for VMs, diskless or embedded > devices, and devices with limited storage space. The server component of > netdump is a UDP daemon which is in ports as ftp/netdumpd. > > The original code has been around for a long time and wasn't authored by > me. However, I've fixed a number of bugs, made it more reliable, and > integrated it with dumpon(8). I also largely rewrote the userland > daemon. At this point I'd like to propose committing it to HEAD. > > The netdump(4) man page included in the patch lists the drivers > currently supported by netdump[*]. To be supported, a driver must be > augmented with a set of routines for use by the generic code. These > allow the generic code to transmit packets and poll for completions. I > added a mechanism to overwrite the mbuf/cluster UMA zone pointers at > panic time so that they instead fetch mbufs from a pre-allocated pool. > This way, existing driver code can be used without modification, and we > don't impose any runtime overhead in the regular mbuf allocation path. > > Of course, the netdump data path is not fully reliable since the panic > may have interrupted a thread as it was processing tx/rx ring entries > and thus left them in an inconsistent state. In my experience this > hasn't proved to be an issue, and when testing I will typically have > several iperf streams running over the interface at panic time. That > said, the same issue exists in principle with local storage devices > as well. > > A kernel dump will generally contain sensitive data. Thus, netdump > should only be used on trusted networks. It is possible to use the > kernel dump encryption feature (dumpon -k) with netdump, however I > am not certain that the encryption scheme used is well-suited for > this situation. Any feedback on this would be appreciated. Please note > that you need netdumpd-20180411 or later when using this feature, as the > daemon needs to accept and save the symmetric key in addition to the > dump itself and support for this isn't present in earlier versions. > > A single large patch for the src tree is available here: > https://people.freebsd.org/~markj/patches/netdump/netdump-20180411.diff > Of course, the user/markj/netdump branch in SVN or its clone in the > GitHub mirror may be used as well. Documentation for configuration is in > the dumpon(8) man page. The gist of it is that one specifies some > additional network parameters to dumpon(8), along with an interface > name. For instance, > > # dumpon -c 192.168.2.11 -s 192.168.2.1 vtnet0 > > tells the kernel to transmit to netdumpd at 192.168.2.1 using a source > address of 192.168.2.11. If there is a router between the two, the > gateway IP for the client must be specified as well, with -g. > > I'm interested in any testing reports that folks may have, especially > with ixgbe or bnxt, or with NICs under load. If you have a favourite > NIC driver which does not yet support netdump, I'm happy to help add > support or review patches. Depending on how testing goes, I plan to > create reviews for various components of the patch over the next several > days. I've created reviews for various components of the change: https://reviews.freebsd.org/D15251 (post-panic mbuf allocator) https://reviews.freebsd.org/D15252 (kernel dump refactoring for netdump) https://reviews.freebsd.org/D15253 (core netdump client) https://reviews.freebsd.org/D15254 (dumpon(8) extension for netdump) https://reviews.freebsd.org/D15255 (alc(4) hooks) https://reviews.freebsd.org/D15256 (bge(4) hooks) https://reviews.freebsd.org/D15257 (bxe(4) hooks) https://reviews.freebsd.org/D15258 (cxgb(4) hooks) https://reviews.freebsd.org/D15259 (mlxen(4) hooks) https://reviews.freebsd.org/D15260 (re(4) hooks) https://reviews.freebsd.org/D15261 (vtnet(4) hooks) https://reviews.freebsd.org/D15262 (iflib hooks) If anyone is interested in reviewing some or all of these diffs, please feel free to add yourself to the corresponding review. I plan to add driver maintainers as reviewers to the corresponding driver changes. From owner-freebsd-arch@freebsd.org Tue May 1 21:53:12 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id D62B6FB905F for ; Tue, 1 May 2018 21:53:11 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 792347C2A2 for ; Tue, 1 May 2018 21:53:11 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by mailman.ysv.freebsd.org (Postfix) id 35A12FB905E; Tue, 1 May 2018 21:53:11 +0000 (UTC) Delivered-To: arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 242BBFB905C for ; Tue, 1 May 2018 21:53:11 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id BD1567C2A0; Tue, 1 May 2018 21:53:10 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id AE1CC5A9F12; Tue, 1 May 2018 21:53:03 +0000 (UTC) Date: Tue, 1 May 2018 21:53:03 +0000 From: Brooks Davis To: Bruce Evans Cc: John Baldwin , arch@freebsd.org Subject: Re: LIBC_SCCS Message-ID: <20180501215303.GA4870@spindle.one-eyed-alien.net> References: <1711113.VelFtdTVS7@ralph.baldwin.cx> <20180428110152.Q4737@besplex.bde.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qDbXVdCdHGoSgWSk" Content-Disposition: inline In-Reply-To: <20180428110152.Q4737@besplex.bde.org> User-Agent: Mutt/1.9.4 (2018-02-28) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 May 2018 21:53:12 -0000 --qDbXVdCdHGoSgWSk Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Apr 28, 2018 at 11:39:02AM +1000, Bruce Evans wrote: > On Fri, 27 Apr 2018, John Baldwin wrote: >=20 > > I suspect no one cares, but for whatever reason our current handling of= the > > LIBC_SCCS macro in some of our libraries annoys me. In theory it seems= like > > LIBC_SCCS's purpose is to control whether or not old SCCS IDs from Berk= eley > > are included in libc's sources when libc is built. (Similar to how mac= ros > > control the behavior of __FBSDID().) However, we use an odd construct = in > > the tree. First, we define LIBC_SCCS by default in the CFLAGS of vario= us > > libraries (libkvm, libutil, libthr, libc, etc.) which in theory would e= nable > > the IDs, but then we explicitly wrap them in #if 0, e.g.: > > > > #if defined(LIBC_SCCS) && !defined(lint) > > #if 0 > > static char sccsid[] =3D "@(#)kvm_hp300.c 8.1 (Berkeley) 6/4/93"; > > #endif > > #endif /* LIBC_SCCS and not lint */ >=20 > Most aren't actually wrapped with '#if 0'. E.g., in libc/*/*.c there are > 839 files but only 47 of these have any '#if 0' at all. SO this can be > fixed without much churn. >=20 > I thought there is a problem with the above not actually compiling if > LIBC_SCCS is defined, but WARNS is only 2 for libc and it takes WARNS >= =3D 4 > to give -Wwrite-strings. At higher WARNS levels the ones without #if 0 also warn about unused static variable. If we're going to keep them, using an __FBSDID()-like macro seems like the best option so that's easy to make correct and doesn't require churn if compilers change. -- Brooks --qDbXVdCdHGoSgWSk Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJa6OG/AAoJEKzQXbSebgfAqt4IAJIrvHBMRE6LWfgozri+nN3U i42IdwC6xt/n/FScwe5beM6uaOoIFxwLSZOAZENXqwqVPWt1l1QnVqQ4TJxsTNuZ aAIg/QamwW+0ruHPThezgU1zPD2oRgnTKcXOf0REXRPRm681IwHAVHGNi6lzFq8t AFGwkHJ6AXW/37k4NG/EQfSREMa2n7zWfwEF1YQ8emsITprK0BNXu7zbJp9S+iKY vTkEg94vukCPSLajyGPovxLPpn8r/tn1fyaKec6MlasihM+hyYdT7Lz+gKNZdyzP 0V7nxg/VlInAJBCF+DEkMab0mHT25d8wXl/nbKCL4YuLfOnPLcBxb4PNT48W7NQ= =qa0X -----END PGP SIGNATURE----- --qDbXVdCdHGoSgWSk-- From owner-freebsd-arch@freebsd.org Wed May 2 15:20:31 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 64438FAC7FA for ; Wed, 2 May 2018 15:20:31 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-it0-x243.google.com (mail-it0-x243.google.com [IPv6:2607:f8b0:4001:c0b::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id EC1CB71E59 for ; Wed, 2 May 2018 15:20:30 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-it0-x243.google.com with SMTP id q4-v6so9560707ite.3 for ; Wed, 02 May 2018 08:20:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:subject:message-id:mime-version :content-disposition:user-agent; bh=RfCEF1LUvqXnGJYnBhsnrvBqkfxNOwtZaDiVEVkuVe4=; b=bl5UK91g1rVRHLwqzWGJwlNb7RSGRy3rGdwfgs+riK3UPPAMzvXQ5A/715N2n3u3zE 5jgk//Om87CRtAbyVuERU7dpC5IBk/rYK9vgJwwhgNSyLy2e+tLuCqzzmW92tTCuD2jO gsLxd4YhtXsPn6nYOlngcKEZ/MEFd0+nQZ/mrxafKV4LL1CA0mCko5ux+R3MDfVa80gI qruA5K9t94yVkSrM1aJNcG3NTPAuHCvXfQL0/UQPp4fXUfkcTfDEGdH4Gfn23tpUHG46 JuVZom5utLYJugwv9307XCqEyjsHascrk3NPlbWxymnbaUrjh+z38PBkSSVOjQFhMuPG 7h7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:subject:message-id :mime-version:content-disposition:user-agent; bh=RfCEF1LUvqXnGJYnBhsnrvBqkfxNOwtZaDiVEVkuVe4=; b=LdmWouOO9UKOPgnSLtJ6lYgdv0qDaBVHE932/DMGtgzMJ0bBRozMat8Zq0lbiv6Gum HXO+Fqw/Mwlcp315JeLrn3fieVqWuube4IrsrldkI+YCFkMR7CQfWPrscLas+YZkyfGX /kcHGRM0oCQdysVCNS/sATVkEfRaVYBoDrcmoSml7XVJfHAS08ztRsCLOUxgMX1cI9l6 WVfJnY/U8g33eQZ3XXXM4sgztmcWG8AYzHk6Y+7BdpbVRS7HDDYnRZlIN/dueTCdGg2E u9tIOJkJBrEI7mn8lrGe0Ei3FAz5VybtCmewaaEZwfthM6VzJ52nH0sE79ilB2VQkRxO O7bg== X-Gm-Message-State: ALQs6tDdANZyqsq8mi2VvP4IJ9cFRjQ87NwkvZoRibGvWwXMkXn/gcKy rpL1lqQdznzS8ytybM+HbakKQQ== X-Google-Smtp-Source: AB8JxZoAFU15qXKmhjzlUaHOP2RPsZIp8NkVEJFoJeqGzgkrCRpnPzFPQ8u7Hh3pPwSzTQbLVF3tRQ== X-Received: by 2002:a24:1d93:: with SMTP id 141-v6mr14258435itj.41.1525274430140; Wed, 02 May 2018 08:20:30 -0700 (PDT) Received: from raichu (toroon0560w-lp130-04-184-145-252-74.dsl.bell.ca. [184.145.252.74]) by smtp.gmail.com with ESMTPSA id k3-v6sm3529984ioc.44.2018.05.02.08.20.28 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 May 2018 08:20:29 -0700 (PDT) Sender: Mark Johnston Date: Wed, 2 May 2018 11:20:24 -0400 From: Mark Johnston To: freebsd-arch@FreeBSD.org Subject: callout_stop() return value Message-ID: <20180502152024.GA24397@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.9.5 (2018-04-13) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 15:20:31 -0000 Hi, We have a few pieces of code that follow a pattern: a thread allocates a resource and schedules a callout. The callout handler releases the resource and never reschedules itself. In the common case, a thread will cancel the callout before it executes. To know whether it must release the resource associated with the callout, this thread must know whether the pending callout was cancelled. Before r302350, our code happened to work; it could use the return value of callout_stop() to correctly determine whether to release the resource. Now, however, it cannot: the return value does not contain sufficient information. In particular, if the return value is 0, we do not know whether a future invocation of the callout was cancelled. The resource's lifetime is effectively tied to the scheduling of the callout, so I think it's preferable to address this by extending the callout API rather than by adding some extra state to the structure containing the callout. Does anyone have any opinions on adding a new flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When this flag is specified, _callout_stop_safe() will return 1 if a future invocation was cancelled, even if the callout was running at the time of the call. The patch below implements this suggestion, but I haven't yet tested it and was wondering if anyone had opinions on how to handle this scenario. If I end up going ahead with this approach, I'll be sure to update the callout man page with a description of CS_EXECUTING and the new flag. Thanks in advance. diff --git a/sys/kern/kern_timeout.c b/sys/kern/kern_timeout.c index 81b4a14ecf08..23c8c7dc3675 100644 --- a/sys/kern/kern_timeout.c +++ b/sys/kern/kern_timeout.c @@ -1183,6 +1183,8 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *)) int direct, sq_locked, use_lock; int cancelled, not_on_a_list; + KASSERT((flags & (CS_EXECUTING | CS_DESCHEDULED)) != + (CS_EXECUTING | CS_DESCHEDULED), ("invalid flags %#x", flags)); if ((flags & CS_DRAIN) != 0) WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock, "calling %s", __func__); @@ -1391,7 +1393,7 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *)) KASSERT(!sq_locked, ("sleepqueue chain still locked")); cancelled = ((flags & CS_EXECUTING) != 0); } else - cancelled = 1; + cancelled = ((flags & CS_DESCHEDULED) == 0); if (sq_locked) sleepq_release(&cc_exec_waiting(cc, direct)); @@ -1422,6 +1424,8 @@ _callout_stop_safe(struct callout *c, int flags, void (*drain)(void *)) } else { TAILQ_REMOVE(&cc->cc_expireq, c, c_links.tqe); } + if ((flags & CS_DESCHEDULED) != 0) + cancelled = 1; } callout_cc_del(c, cc); CC_UNLOCK(cc); diff --git a/sys/sys/callout.h b/sys/sys/callout.h index e4d91c69da3f..5142b3255122 100644 --- a/sys/sys/callout.h +++ b/sys/sys/callout.h @@ -70,6 +70,9 @@ struct callout_handle { #define CS_DRAIN 0x0001 /* callout_drain(), wait allowed */ #define CS_EXECUTING 0x0002 /* Positive return value indicates that the callout was executing */ +#define CS_DESCHEDULED 0x0004 /* Positive return value indicates that + a future invocation of the callout was + cancelled. */ #ifdef _KERNEL /* From owner-freebsd-arch@freebsd.org Wed May 2 15:35:08 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id CD4C0FACDE5 for ; Wed, 2 May 2018 15:35:07 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from pmta2.delivery6.ore.mailhop.org (pmta2.delivery6.ore.mailhop.org [54.200.129.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 468447674A for ; Wed, 2 May 2018 15:35:07 +0000 (UTC) (envelope-from ian@freebsd.org) X-MHO-User: 1e83742b-4e1e-11e8-b951-f99fef315fd9 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound2.ore.mailhop.org (Halon) with ESMTPSA id 1e83742b-4e1e-11e8-b951-f99fef315fd9; Wed, 02 May 2018 15:33:10 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id w42FYvqI042115; Wed, 2 May 2018 09:34:57 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1525275297.57768.202.camel@freebsd.org> Subject: Re: callout_stop() return value From: Ian Lepore To: Mark Johnston , freebsd-arch@FreeBSD.org Date: Wed, 02 May 2018 09:34:57 -0600 In-Reply-To: <20180502152024.GA24397@raichu> References: <20180502152024.GA24397@raichu> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.1 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 15:35:08 -0000 On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > Hi, > > We have a few pieces of code that follow a pattern: a thread > allocates a > resource and schedules a callout. The callout handler releases the > resource and never reschedules itself. In the common case, a thread > will cancel the callout before it executes. To know whether it must > release the resource associated with the callout, this thread must > know > whether the pending callout was cancelled. > It seems to me a better solution would be to track the state / lifetime of the resource separately rather than trying to infer the state of the resource from the state of the callout as viewed through a semi-opaque interface. If the original thread that schedules the callout keeps enough information about the resource to free it after cancelling, then it is already maintaining some kind of sideband info about the resource, even if it's just a pointer to be freed. Couldn't the callout routine manipulate this resource tracking info (null out the pointer or set a bool or whatever), then after cancelling you don't really care whether the callout ran or not, you just examine the pointer/bool/whatever and free or not based on that. -- Ian > Before r302350, our code happened to work; it could use the return > value > of callout_stop() to correctly determine whether to release the > resource. Now, however, it cannot: the return value does not contain > sufficient information. In particular, if the return value is 0, we > do > not know whether a future invocation of the callout was cancelled. > > The resource's lifetime is effectively tied to the scheduling of the > callout, so I think it's preferable to address this by extending the > callout API rather than by adding some extra state to the structure > containing the callout. Does anyone have any opinions on adding a new > flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When > this > flag is specified, _callout_stop_safe() will return 1 if a future > invocation was cancelled, even if the callout was running at the time > of > the call. > > The patch below implements this suggestion, but I haven't yet tested > it and was wondering if anyone had opinions on how to handle this > scenario. If I end up going ahead with this approach, I'll be sure to > update the callout man page with a description of CS_EXECUTING and > the > new flag. Thanks in advance. > > diff --git a/sys/kern/kern_timeout.c b/sys/kern/kern_timeout.c > index 81b4a14ecf08..23c8c7dc3675 100644 > --- a/sys/kern/kern_timeout.c > +++ b/sys/kern/kern_timeout.c > @@ -1183,6 +1183,8 @@ _callout_stop_safe(struct callout *c, int > flags, void (*drain)(void *)) >   int direct, sq_locked, use_lock; >   int cancelled, not_on_a_list; >   > + KASSERT((flags & (CS_EXECUTING | CS_DESCHEDULED)) != > +     (CS_EXECUTING | CS_DESCHEDULED), ("invalid flags %#x", > flags)); >   if ((flags & CS_DRAIN) != 0) >   WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock, >       "calling %s", __func__); > @@ -1391,7 +1393,7 @@ _callout_stop_safe(struct callout *c, int > flags, void (*drain)(void *)) >   KASSERT(!sq_locked, ("sleepqueue chain still > locked")); >   cancelled = ((flags & CS_EXECUTING) != 0); >   } else > - cancelled = 1; > + cancelled = ((flags & CS_DESCHEDULED) == 0); >   >   if (sq_locked) >   sleepq_release(&cc_exec_waiting(cc, direct)); > @@ -1422,6 +1424,8 @@ _callout_stop_safe(struct callout *c, int > flags, void (*drain)(void *)) >   } else { >   TAILQ_REMOVE(&cc->cc_expireq, c, > c_links.tqe); >   } > + if ((flags & CS_DESCHEDULED) != 0) > + cancelled = 1; >   } >   callout_cc_del(c, cc); >   CC_UNLOCK(cc); > diff --git a/sys/sys/callout.h b/sys/sys/callout.h > index e4d91c69da3f..5142b3255122 100644 > --- a/sys/sys/callout.h > +++ b/sys/sys/callout.h > @@ -70,6 +70,9 @@ struct callout_handle { >  #define CS_DRAIN 0x0001 /* callout_drain(), > wait allowed */ >  #define CS_EXECUTING 0x0002 /* Positive return > value indicates that >     the callout was executing > */ > +#define CS_DESCHEDULED 0x0004 /* Positive > return value indicates that > +   a future invocation of the > callout was > +   cancelled. */ >   >  #ifdef _KERNEL >  /*  > _______________________________________________ > freebsd-arch@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.or > g" From owner-freebsd-arch@freebsd.org Wed May 2 15:42:42 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E06BAFAD06E for ; Wed, 2 May 2018 15:42:41 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6ADB677E8E; Wed, 2 May 2018 15:42:41 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-it0-x244.google.com with SMTP id t7-v6so8655073itf.0; Wed, 02 May 2018 08:42:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=o/85Bhk1oENGy81CAGVx1qSrmho4L2CpKiPJ89tNtAg=; b=ZUYyThI+qLPaUD7EUEX47Vd7j3Vct6yMBCFWQgdz/fYSh6oy0AC1cXeZkXB+C/O8Qv bofxEmcSOwvwehaCkaYJazOmnbbyX8Zpn+kuSI9BRiOXpnaJYkyJ7MjaKjgtx9s3EL6o pLUBFw+1f3eWIBZfBzjmxFmO95d3I0UbEMYvhWOG71tcNzQ4DIUocijinHruenDNu8ru OPx9bc1Y/wL31ejM/S0EMW9nEsvO2NwHRhAuMnRXb2XfSO/jBfCMgugO2Y1pXc+jTlC/ 3N16zS9bIbP+bERpuV5snXa608pjsjnDzkZyiD5n+MslxkwykgfA/uQnvgVe+u/zobai twSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=o/85Bhk1oENGy81CAGVx1qSrmho4L2CpKiPJ89tNtAg=; b=RBZEe9GoRbbqj7HIy5jQS7DxgTcrPyMDkffmNUuCOYjT/eYHoypIvBE5aTaPW1+j5j QXWsiv+NEXD67JgPf8QmrF5B8xczuOSGns0QhdvLRH5GPABCw2/9b4quOcszeUmJf/Oh oKcGulWzpmcAq1B+U4ixn9k7aLUCPDzTWKMIFETyI+Qm2l3YAP7J9x70/6ZwtxLHFbYG o8yLRc2OVBpjtODn4sH5JxbvQyMrBLMm6VNWK0xl9DjjOOfXaNsP9yuE+2m2WhDMuUTB 7HHmBmIGgi99Q99jhPy40wqXsi0UT/5vaoCOjXLd93aGt8JStZ8rFActFMxA8gyyv/Oj bG8w== X-Gm-Message-State: ALQs6tCSSFw9GxapS78LQbV/WfqIfN/cR6nTcKJelNwB42HlaxpoGGmF 4PIUeauLUP90DkbngQiUu8AwMQ== X-Google-Smtp-Source: AB8JxZrajLGFpkpIP8tTshZhW087xSxOgB4ZgTjZleU8qH52ZHF0gtg0kD8MlqiSukdr7vlwhuoeyg== X-Received: by 2002:a24:1acc:: with SMTP id 195-v6mr21037267iti.48.1525275760537; Wed, 02 May 2018 08:42:40 -0700 (PDT) Received: from raichu (toroon0560w-lp130-04-184-145-252-74.dsl.bell.ca. [184.145.252.74]) by smtp.gmail.com with ESMTPSA id k130-v6sm5993029itb.0.2018.05.02.08.42.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 May 2018 08:42:39 -0700 (PDT) Sender: Mark Johnston Date: Wed, 2 May 2018 11:42:37 -0400 From: Mark Johnston To: Ian Lepore Cc: freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <20180502154237.GB24397@raichu> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1525275297.57768.202.camel@freebsd.org> User-Agent: Mutt/1.9.5 (2018-04-13) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 15:42:42 -0000 On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > Hi, > > > > We have a few pieces of code that follow a pattern: a thread > > allocates a > > resource and schedules a callout. The callout handler releases the > > resource and never reschedules itself. In the common case, a thread > > will cancel the callout before it executes. To know whether it must > > release the resource associated with the callout, this thread must > > know > > whether the pending callout was cancelled. > > > > It seems to me a better solution would be to track the state / lifetime > of the resource separately rather than trying to infer the state of the > resource from the state of the callout as viewed through a semi-opaque > interface. > > If the original thread that schedules the callout keeps enough > information about the resource to free it after cancelling, then it is > already maintaining some kind of sideband info about the resource, even > if it's just a pointer to be freed. Couldn't the callout routine > manipulate this resource tracking info (null out the pointer or set a > bool or whatever), then after cancelling you don't really care whether > the callout ran or not, you just examine the pointer/bool/whatever and > free or not based on that. I'd considered that. It's not quite as elegant a solution as you suggest, since in my case the resource is embedded in an opaque structure, so I need to add an extra field beside the callout to track state that's already tracked by the callout subsystem. That plus the fact that we have multiple instances of this bug make me want to fix it in a general way, though I recognize that the callout API is already overly complicated. From owner-freebsd-arch@freebsd.org Wed May 2 16:24:24 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 843D4FAE274 for ; Wed, 2 May 2018 16:24:24 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id C5E4D823AE; Wed, 2 May 2018 16:24:23 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id w42GOCPb078866 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 2 May 2018 19:24:16 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua w42GOCPb078866 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id w42GOCIm078865; Wed, 2 May 2018 19:24:12 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 2 May 2018 19:24:12 +0300 From: Konstantin Belousov To: Mark Johnston Cc: Ian Lepore , freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <20180502162412.GA6887@kib.kiev.ua> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> <20180502154237.GB24397@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180502154237.GB24397@raichu> User-Agent: Mutt/1.9.5 (2018-04-13) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 16:24:24 -0000 On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote: > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > > Hi, > > > > > > We have a few pieces of code that follow a pattern: a thread > > > allocates a > > > resource and schedules a callout. The callout handler releases the > > > resource and never reschedules itself. In the common case, a thread > > > will cancel the callout before it executes. To know whether it must > > > release the resource associated with the callout, this thread must > > > know > > > whether the pending callout was cancelled. > > > > > > > It seems to me a better solution would be to track the state / lifetime > > of the resource separately rather than trying to infer the state of the > > resource from the state of the callout as viewed through a semi-opaque > > interface. > > > > If the original thread that schedules the callout keeps enough > > information about the resource to free it after cancelling, then it is > > already maintaining some kind of sideband info about the resource, even > > if it's just a pointer to be freed. Couldn't the callout routine > > manipulate this resource tracking info (null out the pointer or set a > > bool or whatever), then after cancelling you don't really care whether > > the callout ran or not, you just examine the pointer/bool/whatever and > > free or not based on that. > > I'd considered that. It's not quite as elegant a solution as you > suggest, since in my case the resource is embedded in an opaque > structure, so I need to add an extra field beside the callout to track > state that's already tracked by the callout subsystem. That plus the > fact that we have multiple instances of this bug make me want to fix it > in a general way, though I recognize that the callout API is already > overly complicated. I gave up on trying to get this fixed. r302350 was not the first time the return value was broken. I had to do r303426 exactly for this reason. From owner-freebsd-arch@freebsd.org Wed May 2 16:40:07 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DCD02FAE55F for ; Wed, 2 May 2018 16:40:06 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-it0-x242.google.com (mail-it0-x242.google.com [IPv6:2607:f8b0:4001:c0b::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 645528516E; Wed, 2 May 2018 16:40:06 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-it0-x242.google.com with SMTP id j186-v6so17812717ita.5; Wed, 02 May 2018 09:40:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=d0Eri+baLE72cyCr/DWcOFPes00uqBZwpsdTpHVI/DU=; b=mFN3AwO+ypoknbP1Ab9atFx0vsR2HimWMsvfjdlnONlhKktIVss/9yLrbHSEGP/VSy nbmhlzwX3wCdoB8PyPWlry1238Kzs/ZEQj0rtmUJCqKdbxIocqwheXy8+KDYilP7GVw6 MkSWRySzwdfLsd5UE+SkwC1kDYPDpnMxzfq0/UFA7kX7jMBVYleuZcTraL0u+S+XDJQS 6J78Cjl08NMMCdAVtahWUszlPnPk8SzbeIwzmE5j2Yy2+5Z+WBeafs6PZ+ChUcj+UvfM ImlzPHIa8VWQ6dJzEu70SEgItkZkd8AzU6dz435rIKqyVHWTBJLFlQQGnh4/OPOWY2Tq VjEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=d0Eri+baLE72cyCr/DWcOFPes00uqBZwpsdTpHVI/DU=; b=jEpufRV49IRK3Xd6AXIFTlRSgvoNHc+5W1UXaipq/NFzyEB2/r//N/1GVbYZstrjzj QUItzdJR8uGJ5XGme2ow9Da+Aqh/AiCquAh71h9f7gH+yZXYDPMJe32x7KPLCq7QYK+9 8CXCFIwUdF1lbNyzbEjFCDketiE2KeHajIyZ6ZvU1+QWVTatSyaptp84fUYCJp6G7gnj cHJa5UiJ5jH9f8XhQN9dxehROshspdZB1qKHI6R3IIUWuRcJ4mBDqq6gLCBV0jTJwI7u NK3ugi6u5wgZnFfsH7XzDuYsFi+U66Lzzd4NRDhHvkNkYtzcGZO+1XxUkEK97BH5jC1F bcvg== X-Gm-Message-State: ALQs6tBsly8YhLQeFqeV4UVody0sVg8fHaG5TZFfu6iSQHNPn9NgjDQ6 p6PzAsPk9gQoV4O54PvbqKA= X-Google-Smtp-Source: AB8JxZogl5/XnS/1/LovaK6xODwJZSZ6DDotEKEP88HRiLbl6kv9SxQIIlKDSP4UwQ1zPgmROnkiRg== X-Received: by 2002:a24:694b:: with SMTP id e72-v6mr21725496itc.38.1525279205676; Wed, 02 May 2018 09:40:05 -0700 (PDT) Received: from raichu (toroon0560w-lp130-04-184-145-252-74.dsl.bell.ca. [184.145.252.74]) by smtp.gmail.com with ESMTPSA id f4-v6sm5766795ioc.47.2018.05.02.09.40.04 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 May 2018 09:40:04 -0700 (PDT) Sender: Mark Johnston Date: Wed, 2 May 2018 12:40:02 -0400 From: Mark Johnston To: Konstantin Belousov Cc: Ian Lepore , freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <20180502164002.GC24397@raichu> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> <20180502154237.GB24397@raichu> <20180502162412.GA6887@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180502162412.GA6887@kib.kiev.ua> User-Agent: Mutt/1.9.5 (2018-04-13) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 16:40:07 -0000 On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote: > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote: > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > > > Hi, > > > > > > > > We have a few pieces of code that follow a pattern: a thread > > > > allocates a > > > > resource and schedules a callout. The callout handler releases the > > > > resource and never reschedules itself. In the common case, a thread > > > > will cancel the callout before it executes. To know whether it must > > > > release the resource associated with the callout, this thread must > > > > know > > > > whether the pending callout was cancelled. > > > > > > > > > > It seems to me a better solution would be to track the state / lifetime > > > of the resource separately rather than trying to infer the state of the > > > resource from the state of the callout as viewed through a semi-opaque > > > interface. > > > > > > If the original thread that schedules the callout keeps enough > > > information about the resource to free it after cancelling, then it is > > > already maintaining some kind of sideband info about the resource, even > > > if it's just a pointer to be freed. Couldn't the callout routine > > > manipulate this resource tracking info (null out the pointer or set a > > > bool or whatever), then after cancelling you don't really care whether > > > the callout ran or not, you just examine the pointer/bool/whatever and > > > free or not based on that. > > > > I'd considered that. It's not quite as elegant a solution as you > > suggest, since in my case the resource is embedded in an opaque > > structure, so I need to add an extra field beside the callout to track > > state that's already tracked by the callout subsystem. That plus the > > fact that we have multiple instances of this bug make me want to fix it > > in a general way, though I recognize that the callout API is already > > overly complicated. I forgot to add that in some cases, extra synchronization would be needed to maintain this hypothetical flag. Specifically, some of these callouts do not have an associated lock, so the callout handler doesn't have an easy way to mark the resource as consumed. > I gave up on trying to get this fixed. r302350 was not the first time > the return value was broken. > > I had to do r303426 exactly for this reason. What kind of fix did you envision? The problem seems to be that callout_stop()'s return value simply does not contain enough information for certain use cases. From owner-freebsd-arch@freebsd.org Wed May 2 17:00:50 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 605FFFAEB0C for ; Wed, 2 May 2018 17:00:50 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D20596AC1A; Wed, 2 May 2018 17:00:49 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id w42H0d9o087162 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 2 May 2018 20:00:42 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua w42H0d9o087162 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id w42H0dSK087161; Wed, 2 May 2018 20:00:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 2 May 2018 20:00:39 +0300 From: Konstantin Belousov To: Mark Johnston Cc: Ian Lepore , freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <20180502170038.GB6887@kib.kiev.ua> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> <20180502154237.GB24397@raichu> <20180502162412.GA6887@kib.kiev.ua> <20180502164002.GC24397@raichu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180502164002.GC24397@raichu> User-Agent: Mutt/1.9.5 (2018-04-13) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 17:00:50 -0000 On Wed, May 02, 2018 at 12:40:02PM -0400, Mark Johnston wrote: > On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote: > > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote: > > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > > > > Hi, > > > > > > > > > > We have a few pieces of code that follow a pattern: a thread > > > > > allocates a > > > > > resource and schedules a callout. The callout handler releases the > > > > > resource and never reschedules itself. In the common case, a thread > > > > > will cancel the callout before it executes. To know whether it must > > > > > release the resource associated with the callout, this thread must > > > > > know > > > > > whether the pending callout was cancelled. > > > > > > > > > > > > > It seems to me a better solution would be to track the state / lifetime > > > > of the resource separately rather than trying to infer the state of the > > > > resource from the state of the callout as viewed through a semi-opaque > > > > interface. > > > > > > > > If the original thread that schedules the callout keeps enough > > > > information about the resource to free it after cancelling, then it is > > > > already maintaining some kind of sideband info about the resource, even > > > > if it's just a pointer to be freed. Couldn't the callout routine > > > > manipulate this resource tracking info (null out the pointer or set a > > > > bool or whatever), then after cancelling you don't really care whether > > > > the callout ran or not, you just examine the pointer/bool/whatever and > > > > free or not based on that. > > > > > > I'd considered that. It's not quite as elegant a solution as you > > > suggest, since in my case the resource is embedded in an opaque > > > structure, so I need to add an extra field beside the callout to track > > > state that's already tracked by the callout subsystem. That plus the > > > fact that we have multiple instances of this bug make me want to fix it > > > in a general way, though I recognize that the callout API is already > > > overly complicated. > > I forgot to add that in some cases, extra synchronization would be > needed to maintain this hypothetical flag. Specifically, some of these > callouts do not have an associated lock, so the callout handler doesn't > have an easy way to mark the resource as consumed. > > > I gave up on trying to get this fixed. r302350 was not the first time > > the return value was broken. > > > > I had to do r303426 exactly for this reason. > > What kind of fix did you envision? The problem seems to be that > callout_stop()'s return value simply does not contain enough information > for certain use cases. The fix is really social, to make people who change the callout_stop() KPI to care about non-tcp stack uses. I tried to add a flag to _callout_stop_safe() to make return values useful for sleepqueues, see r296320. But as I said, it was broken again and I gave up. From owner-freebsd-arch@freebsd.org Wed May 2 17:02:13 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 29EC1FAF088 for ; Wed, 2 May 2018 17:02:13 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from pmta2.delivery6.ore.mailhop.org (pmta2.delivery6.ore.mailhop.org [54.200.129.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 9FF976B017 for ; Wed, 2 May 2018 17:02:12 +0000 (UTC) (envelope-from ian@freebsd.org) X-MHO-User: 4cfc2d1e-4e2a-11e8-b951-f99fef315fd9 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound2.ore.mailhop.org (Halon) with ESMTPSA id 4cfc2d1e-4e2a-11e8-b951-f99fef315fd9; Wed, 02 May 2018 17:00:22 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id w42H292w042387; Wed, 2 May 2018 11:02:09 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1525280529.57768.230.camel@freebsd.org> Subject: Re: callout_stop() return value From: Ian Lepore To: Mark Johnston , Konstantin Belousov Cc: freebsd-arch@FreeBSD.org Date: Wed, 02 May 2018 11:02:09 -0600 In-Reply-To: <20180502164002.GC24397@raichu> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> <20180502154237.GB24397@raichu> <20180502162412.GA6887@kib.kiev.ua> <20180502164002.GC24397@raichu> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.1 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 17:02:13 -0000 On Wed, 2018-05-02 at 12:40 -0400, Mark Johnston wrote: > On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote: > > > > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote: > > > > > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > > > > > > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > > > > > > > > > Hi, > > > > > > > > > > We have a few pieces of code that follow a pattern: a thread > > > > > allocates a > > > > > resource and schedules a callout. The callout handler releases the > > > > > resource and never reschedules itself. In the common case, a thread > > > > > will cancel the callout before it executes. To know whether it must > > > > > release the resource associated with the callout, this thread must > > > > > know > > > > > whether the pending callout was cancelled. > > > > > > > > > It seems to me a better solution would be to track the state / lifetime > > > > of the resource separately rather than trying to infer the state of the > > > > resource from the state of the callout as viewed through a semi-opaque > > > > interface. > > > > > > > > If the original thread that schedules the callout keeps enough > > > > information about the resource to free it after cancelling, then it is > > > > already maintaining some kind of sideband info about the resource, even > > > > if it's just a pointer to be freed. Couldn't the callout routine > > > > manipulate this resource tracking info (null out the pointer or set a > > > > bool or whatever), then after cancelling you don't really care whether > > > > the callout ran or not, you just examine the pointer/bool/whatever and > > > > free or not based on that. > > > I'd considered that. It's not quite as elegant a solution as you > > > suggest, since in my case the resource is embedded in an opaque > > > structure, so I need to add an extra field beside the callout to track > > > state that's already tracked by the callout subsystem. That plus the > > > fact that we have multiple instances of this bug make me want to fix it > > > in a general way, though I recognize that the callout API is already > > > overly complicated. > I forgot to add that in some cases, extra synchronization would be > needed to maintain this hypothetical flag. Specifically, some of these > callouts do not have an associated lock, so the callout handler doesn't > have an easy way to mark the resource as consumed. > Wouldn't there be implied temporal synchronization? The callout will free the resource and manipulate the sideband info to say so, knowing that nothing else will modify it or even examine it at the same time because the callout would be cancelled and drained before the resource is manipulated by someone else. Conversely, the someone-else doesn't examine or modify the resource or sideband info until the callout is drained (whether cancelled or it just runs to normal completion). Only if it were impossible to reliably cancel the callout and reach a point where you know it's not running would there be any chance of a race that needs explicit locking. -- Ian > > > > I gave up on trying to get this fixed.  r302350 was not the first time > > the return value was broken. > > > > I had to do r303426 exactly for this reason. > What kind of fix did you envision? The problem seems to be that > callout_stop()'s return value simply does not contain enough information > for certain use cases. From owner-freebsd-arch@freebsd.org Wed May 2 21:43:08 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4963FFB5513 for ; Wed, 2 May 2018 21:43:08 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-it0-x229.google.com (mail-it0-x229.google.com [IPv6:2607:f8b0:4001:c0b::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id CA17B6B74C; Wed, 2 May 2018 21:43:07 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-it0-x229.google.com with SMTP id n202-v6so18754239ita.1; Wed, 02 May 2018 14:43:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=ZkluGu76sp/XCUV/0Y64jqCmvAdK79q5q1o59u8Ul2c=; b=dWVECj1tUWtuhB4Z2WiH2y7azeFCS/L3xWP6Ec0UUAFzHlwqmNqoYmg8bwdADXrV0O TDLhc/gYKzyXV5iwiPYuA36/1bZL8XfgBp5qntyvk/6OeJ9fW5qxFGrv3WB3FhUuSiSo 1W6nFP71fnV1l7co6dUWNSZSNHwK4FtdVvB1RW+ARMdrkGpSU0stXV7c8L8DGk+9Wk4a o4C+QPDzq9QxIeuJ293HJIWEjx61j0fS1aphcdkzFwqTxViYuF+hkectLPa9Gj8eUXYa rYNe1oZJEoGQunvMvysyKevK0Tk3bOHtyalKPjrmMO7uVQdjyAjYYm1D/JlWdOP7NBvG d0Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=ZkluGu76sp/XCUV/0Y64jqCmvAdK79q5q1o59u8Ul2c=; b=DsTjHWUkATNFGzjxayxl4y6QUuNMyA86SW2b9YmZS6SfreIxfPqmYfBvS7m29MTRMx Wy+PnKjzNEYWMbz/aUnF0IqkfzPqZc3vPI5LsPFgp6asdNaPirFRCZ34vg2ZbcAdYA4L dV3dcmf7PINH6iHYBBjsDgKPx1UalyHY8gFakqBwFah84eIGRhM5LkrfLKfuBKU8xy6Z Od1xsQHOjehv93B+0MTr/8bFME1C9UR+xk3GJMH9ls9ZK606T1sIKbpx7V4QH+Nt0A2H +NnoJOPmlm60CDsAg82d4xWvzNlfBOL8gfbdZCVhfBva4YKKI4jI52/Ebgu2gMFMpIRV renw== X-Gm-Message-State: ALQs6tAc20tjR0A6u53dDAYpatPPGJz93tqEZ0LgF20xlNZoKGbxAZSP EPch7640isVy0NKhfH0rnbtF8A== X-Google-Smtp-Source: AB8JxZqcHFdnvOtyqdNB5w2BF8Gxnztlz/fK2pzM1xmiPXbXm2J0sZ+Q0ZkHmSbKrWaPuQD5PECniw== X-Received: by 2002:a24:97c4:: with SMTP id k187-v6mr8023011ite.115.1525297386916; Wed, 02 May 2018 14:43:06 -0700 (PDT) Received: from raichu (toroon0560w-lp130-04-184-145-252-74.dsl.bell.ca. [184.145.252.74]) by smtp.gmail.com with ESMTPSA id o1-v6sm4562821ite.37.2018.05.02.14.43.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 May 2018 14:43:06 -0700 (PDT) Sender: Mark Johnston Date: Wed, 2 May 2018 17:43:03 -0400 From: Mark Johnston To: Ian Lepore Cc: Konstantin Belousov , freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <20180502214303.GE24397@raichu> References: <20180502152024.GA24397@raichu> <1525275297.57768.202.camel@freebsd.org> <20180502154237.GB24397@raichu> <20180502162412.GA6887@kib.kiev.ua> <20180502164002.GC24397@raichu> <1525280529.57768.230.camel@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1525280529.57768.230.camel@freebsd.org> User-Agent: Mutt/1.9.5 (2018-04-13) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 02 May 2018 21:43:08 -0000 On Wed, May 02, 2018 at 11:02:09AM -0600, Ian Lepore wrote: > On Wed, 2018-05-02 at 12:40 -0400, Mark Johnston wrote: > > On Wed, May 02, 2018 at 07:24:12PM +0300, Konstantin Belousov wrote: > > > > > > On Wed, May 02, 2018 at 11:42:37AM -0400, Mark Johnston wrote: > > > > > > > > On Wed, May 02, 2018 at 09:34:57AM -0600, Ian Lepore wrote: > > > > > > > > > > On Wed, 2018-05-02 at 11:20 -0400, Mark Johnston wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > We have a few pieces of code that follow a pattern: a thread > > > > > > allocates a > > > > > > resource and schedules a callout. The callout handler releases the > > > > > > resource and never reschedules itself. In the common case, a thread > > > > > > will cancel the callout before it executes. To know whether it must > > > > > > release the resource associated with the callout, this thread must > > > > > > know > > > > > > whether the pending callout was cancelled. > > > > > > > > > > > It seems to me a better solution would be to track the state / lifetime > > > > > of the resource separately rather than trying to infer the state of the > > > > > resource from the state of the callout as viewed through a semi-opaque > > > > > interface. > > > > > > > > > > If the original thread that schedules the callout keeps enough > > > > > information about the resource to free it after cancelling, then it is > > > > > already maintaining some kind of sideband info about the resource, even > > > > > if it's just a pointer to be freed. Couldn't the callout routine > > > > > manipulate this resource tracking info (null out the pointer or set a > > > > > bool or whatever), then after cancelling you don't really care whether > > > > > the callout ran or not, you just examine the pointer/bool/whatever and > > > > > free or not based on that. > > > > I'd considered that. It's not quite as elegant a solution as you > > > > suggest, since in my case the resource is embedded in an opaque > > > > structure, so I need to add an extra field beside the callout to track > > > > state that's already tracked by the callout subsystem. That plus the > > > > fact that we have multiple instances of this bug make me want to fix it > > > > in a general way, though I recognize that the callout API is already > > > > overly complicated. > > I forgot to add that in some cases, extra synchronization would be > > needed to maintain this hypothetical flag. Specifically, some of these > > callouts do not have an associated lock, so the callout handler doesn't > > have an easy way to mark the resource as consumed. > > > > Wouldn't there be implied temporal synchronization? The callout will > free the resource and manipulate the sideband info to say so, knowing > that nothing else will modify it or even examine it at the same time > because the callout would be cancelled and drained before the resource > is manipulated by someone else. Conversely, the someone-else doesn't > examine or modify the resource or sideband info until the callout is > drained (whether cancelled or it just runs to normal completion). In one case, the callout isn't drained but simply stopped. Only if callout_stop() returns 1 (i.e., we cancelled a future invocation AND the callout wasn't running at the time of the call) do we release the resource. However, if the call returns 0 we don't know whether to release the resource. In this case though, I think it's sufficient to check whether callout_pending() is true before calling callout_stop(). I will spend some time trying to fix these issues locally before trying to push this topic further. > Only if it were impossible to reliably cancel the callout and reach a > point where you know it's not running would there be any chance of a > race that needs explicit locking. From owner-freebsd-arch@freebsd.org Thu May 3 07:24:16 2018 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9C9F7FC72D3 for ; Thu, 3 May 2018 07:24:16 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [IPv6:2a01:4f8:c17:6c4b::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2EBDD6B2EE; Thu, 3 May 2018 07:24:15 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [62.141.128.70]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 26591260E3E; Thu, 3 May 2018 09:24:14 +0200 (CEST) Subject: Re: callout_stop() return value To: Mark Johnston , freebsd-arch@FreeBSD.org References: <20180502152024.GA24397@raichu> From: Hans Petter Selasky Message-ID: <1d28ee66-ee17-4515-b855-f33db9e8bbda@selasky.org> Date: Thu, 3 May 2018 09:24:06 +0200 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180502152024.GA24397@raichu> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 May 2018 07:24:16 -0000 On 05/02/18 17:20, Mark Johnston wrote: > Hi, > > We have a few pieces of code that follow a pattern: a thread allocates a > resource and schedules a callout. The callout handler releases the > resource and never reschedules itself. In the common case, a thread > will cancel the callout before it executes. To know whether it must > release the resource associated with the callout, this thread must know > whether the pending callout was cancelled. > > Before r302350, our code happened to work; it could use the return value > of callout_stop() to correctly determine whether to release the > resource. Now, however, it cannot: the return value does not contain > sufficient information. In particular, if the return value is 0, we do > not know whether a future invocation of the callout was cancelled. > > The resource's lifetime is effectively tied to the scheduling of the > callout, so I think it's preferable to address this by extending the > callout API rather than by adding some extra state to the structure > containing the callout. Does anyone have any opinions on adding a new > flag to _callout_stop_safe(), somewhat akin to CS_EXECUTING? When this > flag is specified, _callout_stop_safe() will return 1 if a future > invocation was cancelled, even if the callout was running at the time of > the call. > > The patch below implements this suggestion, but I haven't yet tested > it and was wondering if anyone had opinions on how to handle this > scenario. If I end up going ahead with this approach, I'll be sure to > update the callout man page with a description of CS_EXECUTING and the > new flag. Thanks in advance. > Hi, You cannot add nor change the current callout_xxx() return values! There is a lot of code in the kernel (TCP stack for example) which simply compares this value with < > != and == ! Be warned ! To force all callers to re-evaluate the return value, I suggest to return the callout state as a bitmap structure. See: https://svnweb.freebsd.org/base/projects/hps_head > 55 /* return value for all callout_xxx() functions */ > 56 typedef union callout_ret { > 57 struct { > 58 unsigned cancelled : 1; > 59 unsigned draining : 1; > 60 unsigned reserved : 30; > 61 } bit; > 62 unsigned value; > 63 } callout_ret_t; The clang compiler treats this structure as an integer. Example: > if (callout_async_drain(t_callout, tcp_timer_discard).bit.draining) { --HPS