Date: Thu, 3 May 2018 09:24:06 +0200 From: Hans Petter Selasky <hps@selasky.org> To: Mark Johnston <markj@FreeBSD.org>, freebsd-arch@FreeBSD.org Subject: Re: callout_stop() return value Message-ID: <1d28ee66-ee17-4515-b855-f33db9e8bbda@selasky.org> In-Reply-To: <20180502152024.GA24397@raichu> References: <20180502152024.GA24397@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1d28ee66-ee17-4515-b855-f33db9e8bbda>