[dpdk-dev] [PATCH v5 02/21] eal: list acceptable init priorities

Neil Horman nhorman at tuxdriver.com
Mon Apr 16 13:31:53 CEST 2018


On Sun, Apr 15, 2018 at 05:13:13PM +0200, Gaëtan Rivet wrote:
> Hello Neil,
> 
> On Sat, Apr 14, 2018 at 02:45:45PM -0400, Neil Horman wrote:
> > On Fri, Apr 13, 2018 at 02:55:11PM +0200, Gaëtan Rivet wrote:
> > > Hi Shreyansh,
> > > 
> > > On Fri, Apr 13, 2018 at 06:22:43PM +0530, Shreyansh Jain wrote:
> > > > On Friday 13 April 2018 05:12 PM, Neil Horman wrote:
> > > > > On Thu, Apr 12, 2018 at 11:57:47PM +0200, Gaëtan Rivet wrote:
> > > > > > Hello Neil,
> > > > > > 
> > > > > > On Thu, Apr 12, 2018 at 07:28:26AM -0400, Neil Horman wrote:
> > > > > > > On Wed, Apr 11, 2018 at 02:04:03AM +0200, Gaetan Rivet wrote:
> > > > > > > > Build a central list to quickly see each used priorities for
> > > > > > > > constructors, allowing to verify that they are both above 100 and in the
> > > > > > > > proper order.
> > > > > > > > 
> > > > > > > > Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> > > > > > > > Acked-by: Neil Horman <nhorman at tuxdriver.com>
> > > > > > > > Acked-by: Shreyansh Jain <shreyansh.jain at nxp.com>
> > > > > > > > ---
> > > > > > > >   lib/librte_eal/common/eal_common_log.c     | 2 +-
> > > > > > > >   lib/librte_eal/common/include/rte_bus.h    | 2 +-
> > > > > > > >   lib/librte_eal/common/include/rte_common.h | 8 +++++++-
> > > > > > > >   3 files changed, 9 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> > > > > > > > index a27192620..36b9d6e08 100644
> > > > > > > > --- a/lib/librte_eal/common/eal_common_log.c
> > > > > > > > +++ b/lib/librte_eal/common/eal_common_log.c
> > > > > > > > @@ -260,7 +260,7 @@ static const struct logtype logtype_strings[] = {
> > > > > > > >   };
> > > > > > > >   /* Logging should be first initializer (before drivers and bus) */
> > > > > > > > -RTE_INIT_PRIO(rte_log_init, 101);
> > > > > > > > +RTE_INIT_PRIO(rte_log_init, LOG);
> > > > > > > >   static void
> > > > > > > >   rte_log_init(void)
> > > > > > > >   {
> > > > > > > > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> > > > > > > > index 6fb08341a..eb9eded4e 100644
> > > > > > > > --- a/lib/librte_eal/common/include/rte_bus.h
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_bus.h
> > > > > > > > @@ -325,7 +325,7 @@ enum rte_iova_mode rte_bus_get_iommu_class(void);
> > > > > > > >    * The constructor has higher priority than PMD constructors.
> > > > > > > >    */
> > > > > > > >   #define RTE_REGISTER_BUS(nm, bus) \
> > > > > > > > -RTE_INIT_PRIO(businitfn_ ##nm, 110); \
> > > > > > > > +RTE_INIT_PRIO(businitfn_ ##nm, BUS); \
> > > > > > > >   static void businitfn_ ##nm(void) \
> > > > > > > >   {\
> > > > > > > >   	(bus).name = RTE_STR(nm);\
> > > > > > > > diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> > > > > > > > index 6c5bc5a76..8f04518f7 100644
> > > > > > > > --- a/lib/librte_eal/common/include/rte_common.h
> > > > > > > > +++ b/lib/librte_eal/common/include/rte_common.h
> > > > > > > > @@ -81,6 +81,12 @@ typedef uint16_t unaligned_uint16_t;
> > > > > > > >    */
> > > > > > > >   #define RTE_SET_USED(x) (void)(x)
> > > > > > > > +#define RTE_PRIORITY_LOG 101
> > > > > > > > +#define RTE_PRIORITY_BUS 110
> > > > > > > > +
> > > > > > > > +#define RTE_PRIO(prio) \
> > > > > > > > +	RTE_PRIORITY_ ## prio
> > > > > > > > +
> > > > > > > >   /**
> > > > > > > >    * Run function before main() with low priority.
> > > > > > > >    *
> > > > > > > > @@ -102,7 +108,7 @@ static void __attribute__((constructor, used)) func(void)
> > > > > > > >    *   Lowest number is the first to run.
> > > > > > > >    */
> > > > > > > >   #define RTE_INIT_PRIO(func, prio) \
> > > > > > > > -static void __attribute__((constructor(prio), used)) func(void)
> > > > > > > > +static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
> > > > > > > It just occured to me, that perhaps you should add a RTE_PRORITY_LAST priority,
> > > > > > > and redefine RTE_INIT to RTE_INIT_PRIO(func, RTE_PRIORITY_LAST) for clarity.  I
> > > > > > > presume that constructors with no explicit priority run last, but the gcc
> > > > > > > manual doesn't explicitly say that.  It would be a heck of a bug to track down
> > > > > > > if somehow unprioritized constructors ran early.
> > > > > > > 
> > > > > > > Neil
> > > > > > > 
> > > > > > 
> > > > > > While certainly poorly documented, the behavior is well-defined. I don't see
> > > > > > a situation where the bug you describe could arise.
> > > > > > 
> > > > > > Adding RTE_PRIORITY_LAST is pretty harmless, but I'm not sure it's
> > > > > > justified to add it. If you still think it is useful, I will do it.
> > > > > > 
> > > > > It was more just a way to unify the macros is all, probably not important.
> > > > > 
> > > > > > I'd be curious to hear if anyone has had issues of this kind.
> > > > > > 
> > > > > I've not had any, but I was suprised to see that the gcc manual didn't
> > > > > explicitly call out the implied priority of unprioritized constructors
> > > > 
> > > > I (tried to) looked into the gcc code base. It seems that when priority is
> > > > not defined, DEFAULT_INIT_PRIORITY 65536, is used.
> > > > 
> > > > --->8--- gcc/collect2.c ---
> > > >   /* Extract init_p number from ctor/dtor name.  */
> > > >   pri = atoi (name + pos);
> > > >   return pri ? pri : DEFAULT_INIT_PRIORITY;
> > > > --->8---
> > > > 
> > > > Though, I couldn't find any documentation for this fact - and, I can never
> > > > be confident about gcc code.
> > > > 
> > > > I found one of the ARM compiler (clang) does has a policy for using
> > > > non-specified priority as lower than specified priority. [1]
> > > > 
> > > > [1] https://developer.arm.com/docs/dui0774/latest/compiler-specific-function-variable-and-type-attributes/__attribute__constructorpriority-function-attribute
> > > > 
> > > > A specified value for RTE_PRIORITY_LAST is not a bad option - it would help
> > > > in keeping the priorities bound without relying on the unknown of priority
> > > > for unspecified constructors.
> > > 
> > > This is interesting, thanks for looking up the GCC code.
> > > Ok, unless someone has a strong reason not to, I will add
> > > RTE_PRIORITY_LAST. Not really convinced about it but not
> > > opposed enough either :) .
> > > 
> > I concur.  It sounds like gcc is safe, but clangs priority scheme makes me want
> > our priorities to be explicit.
> 
> Unless I've misunderstood, clang documentation describes the same
> behavior as GCC?
> 
hmm, from the description above, Shreyansh indicates that the default priority
in GCC is 65535, which effectively means "run last".  And in clang, the
documentation above says regardless of value, unprioritized constructors always
run after prioritized constructors.

So, yeah, you're right, the behavior is the same.  Though the fact that it took
me two emails and typing it out to confirm that to myself, probably suggests
spelling it out is best :)

> Anyway, I've sent a new series with RTE_PRIORITY_LAST:
> 
> https://dpdk.org/dev/patchwork/patch/38126/
> 
I think that makes the most sense.  Thanks!

Neil

> -- 
> Gaëtan Rivet
> 6WIND
> 


More information about the dev mailing list