27/10/2023 10:45, Morten Brørup:
> > From: Thomas Monjalon [mailto:tho...@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: sta...@dpdk.org
> >
> > Signed-off-by: Thomas Monjalon <tho...@monjalon.net>
> > Acked-by: Morten Brørup <m...@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.
Yes OK, thanks for the careful review.
>
> > + */
> > 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.
Yes something like that looks better.
I will try to find a short sentence.
>
> My ACK is still valid.
>
>
