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