[prev in list] [next in list] [prev in thread] [next in thread] 

List:       freebsd-arch
Subject:    Re: callout_stop() return value
From:       Hans Petter Selasky <hps () selasky ! org>
Date:       2018-05-03 7:24:06
Message-ID: 1d28ee66-ee17-4515-b855-f33db9e8bbda () selasky ! org
[Download RAW message or body]

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
_______________________________________________
freebsd-arch@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@freebsd.org"
[prev in list] [next in list] [prev in thread] [next in thread]