RE: [PATCH v6 1/1] eal/unix: allow creating thread with real-time priority

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Friday, 27 October 2023 10.09
> 
> When adding an API for creating threads,
> the real-time priority has been forbidden on Unix.
> 
> There is a known issue with ring behaviour,
> but it should not be completely forbidden.
> 
> Real-time thread can block some kernel threads on the same core,
> making the system unstable.
> That's why a sleep is added in the test thread,
> and a warning is logged when using real-time priority.
> 
> Fixes: ca04c78b6262 ("eal: get/set thread priority per thread
> identifier")
> Fixes: ce6e911d20f6 ("eal: add thread lifetime API")
> Fixes: a7ba40b2b1bf ("drivers: convert to internal control threads")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> ---

[...]

>  enum rte_thread_priority {
> +	/** Normal thread priority, the default. */
>  	RTE_THREAD_PRIORITY_NORMAL            = 0,
> -	/**< normal thread priority, the default */
> +	/**
> +	 * Highest thread priority, use with caution.
> +	 * WARNING: System may be unstable because of a real-time busy
> loop.
> +	 *          @see rte_thread_yield_realtime().

Please remove the reference to the now non-existing function.

Also, I'd prefer to move the warning comments (about real-time threads having priority over kernel threads, and issues with rte_ring) up here, so it goes into the public API documentation.

> +	 */
>  	RTE_THREAD_PRIORITY_REALTIME_CRITICAL = 1,
> -	/**< highest thread priority allowed */
>  };
> 
>  /**
> diff --git a/lib/eal/unix/rte_thread.c b/lib/eal/unix/rte_thread.c
> index 278d8d342d..17ffb86c17 100644
> --- a/lib/eal/unix/rte_thread.c
> +++ b/lib/eal/unix/rte_thread.c
> @@ -33,6 +33,8 @@ static int
>  thread_map_priority_to_os_value(enum rte_thread_priority eal_pri, int
> *os_pri,
>  	int *pol)
>  {
> +	static bool warned;
> +
>  	/* Clear the output parameters. */
>  	*os_pri = sched_get_priority_min(SCHED_OTHER) - 1;
>  	*pol = -1;
> @@ -51,6 +53,17 @@ thread_map_priority_to_os_value(enum
> rte_thread_priority eal_pri, int *os_pri,
>  			sched_get_priority_max(SCHED_OTHER)) / 2;
>  		break;
>  	case RTE_THREAD_PRIORITY_REALTIME_CRITICAL:
> +		/*
> +		 * WARNING: Real-time busy loop takes priority on kernel
> threads,
> +		 *          making the system unstable.
> +		 *          There is also a known issue when using
> rte_ring.
> +		 */
> +		if (!warned) {
> +			RTE_LOG(NOTICE, EAL,
> +					"Real-time thread is unstable if polling
> without sleep.\n");
> +			warned = true;
> +		}

Is it 100 % certain that the system becomes unstable if not sleeping or using blocking system calls from a real-time thread?
And technically, it's not the thread itself that becomes unstable.

How about:
"System may be unstable unless real-time thread uses blocking system calls or sleeps."
or:
"Real-time thread usually requires the use of blocking system calls or sleeps."
or something else.

My ACK is still valid.