[PATCH v7 1/5] Documentation: Add Thread safety page

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Thu, Aug 08, 2024 at 03:09:44PM +0100, Daniel Scally wrote:
> Move the section of the Thread support page dealing with thread safety
> to a dedicated .dox file at Documentation/. This is done to support the
> splitting of the Documentation into a public and internal version. With
> a separate page, references can be made to thread safety without having
> to include the Thread class in the doxygen run. Some sections of the new
> page are still specific to internal implementations and so are hidden
> with the \internal flag and an internal section which is conditionally
> included. For now, hardcode it to be included in the Doxyfile.
> 
> Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> ---
> Changes since v6:
> 
> Conditionally included any reference to thread-bound behind either the \internal
> directive or and internal section.
> 
> Changes since v5:
> 
> - Much of the content that dealt with internal implementation was moved
>   back to its place against the Thread class. The thread safety section
>   is retained in a separate page, with a single reference to the main
>   thread support page who's display is conditional on the doxygen
>   INTERNAL_DOCS directive.
> 
> Given the scope of the changes, I dropped the R-b tags the patch had
> accumulated
> 
> Changes since v1:
> 
> - New patch
> 
>  Documentation/Doxyfile.in       |  5 +++-
>  Documentation/thread-safety.dox | 48 +++++++++++++++++++++++++++++++++
>  src/libcamera/base/thread.cpp   | 36 -------------------------
>  3 files changed, 52 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/thread-safety.dox
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 62e63968..203f8e67 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -17,13 +17,15 @@ EXTENSION_MAPPING      = h=C++
>  
>  TOC_INCLUDE_HEADINGS   = 0
>  
> +ENABLED_SECTIONS       = internal
>  INTERNAL_DOCS          = YES
>  CASE_SENSE_NAMES       = YES
>  
>  QUIET                  = YES
>  WARN_AS_ERROR          = @WARN_AS_ERROR@
>  
> -INPUT                  = "@TOP_SRCDIR@/include/libcamera" \
> +INPUT                  = "@TOP_SRCDIR@/Documentation" \
> +                         "@TOP_SRCDIR@/include/libcamera" \
>                           "@TOP_SRCDIR@/src/ipa/ipu3" \
>                           "@TOP_SRCDIR@/src/ipa/libipa" \
>                           "@TOP_SRCDIR@/src/libcamera" \
> @@ -32,6 +34,7 @@ INPUT                  = "@TOP_SRCDIR@/include/libcamera" \
>  
>  FILE_PATTERNS          = *.c \
>                           *.cpp \
> +                         *.dox \
>                           *.h
>  
>  RECURSIVE              = YES
> diff --git a/Documentation/thread-safety.dox b/Documentation/thread-safety.dox
> new file mode 100644
> index 00000000..1a610035
> --- /dev/null
> +++ b/Documentation/thread-safety.dox
> @@ -0,0 +1,48 @@
> +/**
> + * \page thread-safety Reentrancy and Thread-Safety
> + *
> + * Through the documentation, several terms are used to define how classes and
> + * their member functions can be used from multiple threads.
> + *
> + * - A **reentrant** function may be called simultaneously from multiple
> + *   threads if and only if each invocation uses a different instance of the
> + *   class. This is the default for all member functions not explictly marked
> + *   otherwise.
> + *
> + * - \anchor thread-safe A **thread-safe** function may be called
> + *   simultaneously from multiple threads on the same instance of a class. A
> + *   thread-safe function is thus reentrant. Thread-safe functions may also be
> + *   called simultaneously with any other reentrant function of the same class
> + *   on the same instance.
> + *
> + * \internal
> + * - \anchor thread-bound A **thread-bound** function may be called only from
> + *   the thread that the class instances lives in (see section \ref
> + *   thread-objects). For instances of classes that do not derive from the
> + *   Object class, this is the thread in which the instance was created. A
> + *   thread-bound function is not thread-safe, and may or may not be reentrant.
> + * \endinternal
> + *
> + * Neither reentrancy nor thread-safety, in this context, mean that a function
> + * may be called simultaneously from the same thread, for instance from a
> + * callback invoked by the function. This may deadlock and isn't allowed unless
> + * separately documented.
> + *
> + * \if internal
> + * A class is defined as reentrant, thread-safe or thread-bound if all its
> + * member functions are reentrant, thread-safe or thread-bound respectively.
> + * Some member functions may additionally be documented as having additional
> + * thread-related attributes.
> + *
> + * \else
> + *
> + * A class is defined as reentrant or thread-safe if all its member functions
> + * are reentrant or thread-safe respectively. Some member functions may
> + * additionally be documented as having additional thread-related attributes.
> + *
> + * \endif

As the second sentence is common, I think you can share it:

 * \if internal
 * A class is defined as reentrant, thread-safe or thread-bound if all its
 * member functions are reentrant, thread-safe or thread-bound respectively.
 * \else
 * A class is defined as reentrant or thread-safe if all its member functions
 * are reentrant or thread-safe respectively.
 * \endif
 * Some member functions may additionally be documented as having additional
 * thread-related attributes.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> + *
> + * Most classes are reentrant but not thread-safe, as making them fully
> + * thread-safe would incur locking costs considered prohibitive for the
> + * expected use cases.
> + */
> diff --git a/src/libcamera/base/thread.cpp b/src/libcamera/base/thread.cpp
> index 72733431..8735670b 100644
> --- a/src/libcamera/base/thread.cpp
> +++ b/src/libcamera/base/thread.cpp
> @@ -64,42 +64,6 @@
>   * receiver's event loop, running in the receiver's thread. This mechanism can
>   * be overridden by selecting a different connection type when calling
>   * Signal::connect().
> - *
> - * \section thread-reentrancy Reentrancy and Thread-Safety
> - *
> - * Through the documentation, several terms are used to define how classes and
> - * their member functions can be used from multiple threads.
> - *
> - * - A **reentrant** function may be called simultaneously from multiple
> - *   threads if and only if each invocation uses a different instance of the
> - *   class. This is the default for all member functions not explictly marked
> - *   otherwise.
> - *
> - * - \anchor thread-safe A **thread-safe** function may be called
> - *   simultaneously from multiple threads on the same instance of a class. A
> - *   thread-safe function is thus reentrant. Thread-safe functions may also be
> - *   called simultaneously with any other reentrant function of the same class
> - *   on the same instance.
> - *
> - * - \anchor thread-bound A **thread-bound** function may be called only from
> - *   the thread that the class instances lives in (see section \ref
> - *   thread-objects). For instances of classes that do not derive from the
> - *   Object class, this is the thread in which the instance was created. A
> - *   thread-bound function is not thread-safe, and may or may not be reentrant.
> - *
> - * Neither reentrancy nor thread-safety, in this context, mean that a function
> - * may be called simultaneously from the same thread, for instance from a
> - * callback invoked by the function. This may deadlock and isn't allowed unless
> - * separately documented.
> - *
> - * A class is defined as reentrant, thread-safe or thread-bound if all its
> - * member functions are reentrant, thread-safe or thread-bound respectively.
> - * Some member functions may additionally be documented as having additional
> - * thread-related attributes.
> - *
> - * Most classes are reentrant but not thread-safe, as making them fully
> - * thread-safe would incur locking costs considered prohibitive for the
> - * expected use cases.
>   */
>  
>  /**

-- 
Regards,

Laurent Pinchart