Define and document expected behavior of API calls in various contexts

[pabigot]

This issue is intended to cover the concerns raised in #1960, #3694, #6184, #17014, and related lack of clarity and consistency.

Proposal: We should have:

• Definitions of terms; see Draft terminology related to API behavior definitions #20959
• A policy for when (kernel and driver) API functions are thread-safe/reentrant. Some are (to varying degrees), some are not.
• At the individual function level we should clearly document:
  • Can a call block?
  • Any situation where the call doesn't block but also will not return for a very long time (e.g. chip-level flash erase)
  • Can the function be invoked from an ISR? What behavior is guaranteed in that situation?
  • Are the operations of the function atomic?
  • The context in which any callback is invoked.
  • Any conditions that must be satisfied before the function is invoked, e.g. holding a lock.

Also are k_is_in_isr() and k_is_preempt_thread() sufficient to determine whether it's safe to proceed, or do we also need k_is_atomic(), #17301, or other technologies?

Example policies might be:

• All driver functions must be invokable from ISRs, but-EWOULDBLOCK shall be returned in situations where the requested operation cannot be successfully completed in that context.
• All GPIO manipulation functions must be atomic.

If we can define the normal behavior at either a system or driver-specific level, only the exceptions need be documented at the individual function level.

[mbolivar]

• thread-safe: (TBD) a function is thread-safe if its behavior is correct when invoked from multiple threads at the same time

• reentrant: (TBD) a function is reentrant if its behavior is correct when it is invoked by (indirect) recursion from the same thread

+1 to these.

• interrupt-safe: (TBD) a function is interrupt-safe if its behavior is not affected by concurrent access from interrupts

No objections, but can you provide an example API function where you might want to use this term? I would expect all the driver APIs I can think of offhand to be interrupt-safe, at least when called from thread context.

* block: (TBD) a call blocks if it can suspend the invoking thread
  while it waits for something to happen

I would suggest "might sleep" (or "may sleep") instead.

This is more familiar from Linux kernel source code (648 matches for "might sleep" in v5.3), and has additional advantages beyond that.

In particular, as discussed for clock_control_on() at today's API meeting, some people (including me) thought "block" might simply mean "don't return until the operation is complete". This would include functions which busy-wait until the job is done (like k_spin_lock() might want to do on SMP).

By contrast, "might sleep" makes it clearer that the caller's thread might get scheduled out, and thus further hints the caller must be running in thread context.

At the individual function level we should clearly document:

Can a call block?

So this would become e.g.:

/**
 * @brief Enable a clock controlled by the device
 *
 * On success, the clock is enabled and ready when this function
 * returns.
 *
 * This function :ref:`might-sleep`.
 *
 * @param dev Device structure whose driver controls the clock.
 * @param sys Opaque data representing the clock.
 * @return 0 on success, negative errno on failure.
 */
static inline int clock_control_on(struct device *dev,
				   clock_control_subsys_t sys)

Where the Sphinx might-sleep ref would link directly to a common definition for the term.

Also are k_is_in_isr() and k_is_preempt_thread() sufficient to determine
whether it's safe to proceed, or do we also need k_is_atomic(), #17301, or other technologies?

+1 to k_is_atomic() instead of k_is_in_isr().

[mbolivar]

+1 to k_is_atomic() instead of k_is_in_isr().

Never mind, I take it back. https://lwn.net/Articles/274695/

Once these mistakes are cleared up, there is still the question of just how kernel code should decide whether it is running in an atomic context or not. The real answer is that it just can't do that. Quoting Andrew Morton again:

The consistent pattern we use in the kernel is that callers keep track of whether they are running in a schedulable context and, if necessary, they will inform callees about that. Callees don't work it out for themselves.

[pabigot]

• interrupt-safe: (TBD) a function is interrupt-safe if its behavior is not affected by concurrent access from interrupts

No objections, but can you provide an example API function where you might want to use this term? I would expect all the driver APIs I can think of offhand to be interrupt-safe, at least when called from thread context.

We need to be able to say succinctly "Unless otherwise specified all API functions are interrupt-safe" and expect people to know what that means. A specific example would be the GPIO API, where it is not clear to implementers that, because GPIO write functions may be invoked from ISRs, read-modify-write code like:

u32_t out = gpio->OUT;
gpio->OUT ^= (out & ~mask) | (value & mask);

must be wrapped in a spin-lock to be interrupt-safe. Most implementations don't do that, even in the ongoing GPIO rewrite, in part because we couldn't get agreement in #19252 on what "interrupt-safe" meant and whether it was important to document that expectation before people started implementing drivers.

* block: (TBD) a call blocks if it can suspend the invoking thread
  while it waits for something to happen

I would suggest "might sleep" (or "may sleep") instead.
This is more familiar from Linux kernel source code (648 matches for "might sleep" in v5.3), and has additional advantages beyond that.

In particular, as discussed for clock_control_on() at today's API meeting, some people (including me) thought "block" might simply mean "don't return until the operation is complete". This would include functions which busy-wait until the job is done (like k_spin_lock() might want to do on SMP).

My intent was to identify functions that will not transfer control from a cooperative thread. "blocking" is a poorly chosen term. Scrap it and start over.

Zephyr uses several terms including "Waiting", "Suspended", and "Ready" to identify started non-terminated non-running threads. The transitions we're interested in are "suspend" and "wait". Let's use "yield" for "suspends or waits".

There are three cases:

• The thread invoking a given function will not yield;
• The thread invoking a given function may yield;
• The thread invoking a given function will yield.

So we can do:

• yields (thread): A thread yields when it invokes an operation that transitions it to Suspended or Waiting.

• pre-empted: A thread is pre-empted when it transitions to Ready without having yielded.

• yielding (function): A function is yielding when it invokes (directly or indirectly) an operation that causes the executing thread to yield.

• non-yielding (function): A function is non-yielding when it is not yielding.

It's non-yielding that we care about, because those functions guarantee, when invoked by a cooperative thread, that no other cooperative thread will be allowed to run before the function returns. (Absent bugs like #20255.)

I'm not sure exactly how functions should be annotated with respect to their behavior, but I would want it to be terse and consistent.

+1 to k_is_atomic() instead of k_is_in_isr().

Never mind, I take it back. https://lwn.net/Articles/274695/

Yes, but that relies on having API to provide information about context to callees, which we probably don't have. To be considered later.

[mbolivar]

A specific example would be the GPIO API

Excellent example, thank you.

Summarizing the status:

• thread-safe: OK as-is
• re-entrant: OK as-is
• interrupt-safe: OK as-is
• atomic: OK as-is
• blocking: scrap it, need a different term
• k_is_atomic(): no, not yet (and to clarify: in my mind, not ever, being convinced by the LWN article)

There are three cases:

* The thread invoking a given function will not yield;
* The thread invoking a given function may yield;
* The thread invoking a given function will yield.

Did you mean "The function invoked by a given thread" instead of "The thread invoking a given function"?

• yielding (function): A function is yielding when it invokes (directly or indirectly) an operation that causes the executing thread to yield.
• non-yielding (function): A function is non-yielding when it is not yielding.

With the given definition of "yield" ("suspends or waits"), I agree these are the desired semantics.

I also agree that defining terms in accordance with documented thread states and related APIs is a great approach.

However, I'm still bullish on "might sleep" terminology-wise. I worry "yielding" is too easy to confuse with "may call k_yield()".

[pabigot]

I've integrated the discussion so far into this document which is available for review through #20959 so people can comment on specific concerns in context. I think that'll be a bit more convenient then chained comments on this issue.

There are three cases:

* The thread invoking a given function will not yield;
* The thread invoking a given function may yield;
* The thread invoking a given function will yield.

Did you mean "The function invoked by a given thread" instead of "The thread invoking a given function"?

No. The operation of yielding is something that directly affects threads. The impact on functions is derived.

However, I'm still bullish on "might sleep" terminology-wise. I worry "yielding" is too easy to confuse with "may call k_yield()".

A problem is that traditionally Zephyr has used "sleep" to specifically mean "wait for a timeout to occur". I would rather that "suspend" was more general, but currently it specifically references waiting without an event.