callout_stop() return value

[Mark Johnston]

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.