diff mbox series

[v10,05/27] eal: introduce dtor macros

Message ID 42037d90036505fc764572b8a4c8b25a529c4ec7.1530791217.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Device querying |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Gaëtan Rivet July 5, 2018, 11:48 a.m. UTC 
  Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/include/rte_common.h | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Shreyansh Jain July 6, 2018, 4:17 a.m. UTC | #1 
On Thursday 05 July 2018 05:18 PM, Gaetan Rivet wrote:
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_eal/common/include/rte_common.h | 23 ++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 434adfd45..0dd832728 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -111,6 +111,29 @@ static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
>   #define RTE_INIT(func) \
>   	RTE_INIT_PRIO(func, LAST)
>   

Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
Thomas Monjalon July 10, 2018, 11:40 a.m. UTC | #2 
05/07/2018 13:48, Gaetan Rivet:
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Please justify why you need destructors, by providing a commit log.

> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -111,6 +111,29 @@ static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
>  #define RTE_INIT(func) \
>  	RTE_INIT_PRIO(func, LAST)
>  
> +/**
> + * Run after main() with low priority.
> + *
> + * @param func
> + *   Destructor function name.
> + * @param prio
> + *   Priority number must be above 100.
> + *   Lowest number is the last to run.
> + */
> +#define RTE_FINI_PRIO(func, prio) \
> +static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)

I don't like the name of this macro.
What about RTE_CLEAN_PRIO?
Gaëtan Rivet July 10, 2018, 12:56 p.m. UTC | #3 
Hi Thomas,

On Tue, Jul 10, 2018 at 01:40:01PM +0200, Thomas Monjalon wrote:
> 05/07/2018 13:48, Gaetan Rivet:
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> 
> Please justify why you need destructors, by providing a commit log.
> 
> > --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -111,6 +111,29 @@ static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
> >  #define RTE_INIT(func) \
> >  	RTE_INIT_PRIO(func, LAST)
> >  
> > +/**
> > + * Run after main() with low priority.
> > + *
> > + * @param func
> > + *   Destructor function name.
> > + * @param prio
> > + *   Priority number must be above 100.
> > + *   Lowest number is the last to run.
> > + */
> > +#define RTE_FINI_PRIO(func, prio) \
> > +static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> 
> I don't like the name of this macro.
> What about RTE_CLEAN_PRIO?
> 
> 

FINI is symmetrical to INIT in referencing the related ELF section.

RTE_CLEAN presumes that the intended purpose of the function will be to
cleanup resources. As far as we are concerned, this code could send a
signal, dump config info or format / (which would be a pretty advanced
cleanup, granted).

Sometimes, it could be used to release resources, presumably.

I'm not a fan of FINI either, but I appreciate the symmetry.
It's pretty neutral about what it does, as its meaning is literally
"The following function will be part of the .fini section".

Alternatives:

   FINALIZE
   UNINIT

But they have the same issue as RTE_CLEAN, IMO.
Thomas Monjalon July 10, 2018, 1:06 p.m. UTC | #4 
10/07/2018 14:56, Gaëtan Rivet:
> Hi Thomas,
> 
> On Tue, Jul 10, 2018 at 01:40:01PM +0200, Thomas Monjalon wrote:
> > 05/07/2018 13:48, Gaetan Rivet:
> > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > 
> > Please justify why you need destructors, by providing a commit log.
> > 
> > > --- a/lib/librte_eal/common/include/rte_common.h
> > > +++ b/lib/librte_eal/common/include/rte_common.h
> > > @@ -111,6 +111,29 @@ static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
> > >  #define RTE_INIT(func) \
> > >  	RTE_INIT_PRIO(func, LAST)
> > >  
> > > +/**
> > > + * Run after main() with low priority.
> > > + *
> > > + * @param func
> > > + *   Destructor function name.
> > > + * @param prio
> > > + *   Priority number must be above 100.
> > > + *   Lowest number is the last to run.
> > > + */
> > > +#define RTE_FINI_PRIO(func, prio) \
> > > +static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> > 
> > I don't like the name of this macro.
> > What about RTE_CLEAN_PRIO?
> > 
> > 
> 
> FINI is symmetrical to INIT in referencing the related ELF section.
> 
> RTE_CLEAN presumes that the intended purpose of the function will be to
> cleanup resources. As far as we are concerned, this code could send a
> signal, dump config info or format / (which would be a pretty advanced
> cleanup, granted).
> 
> Sometimes, it could be used to release resources, presumably.
> 
> I'm not a fan of FINI either, but I appreciate the symmetry.
> It's pretty neutral about what it does, as its meaning is literally
> "The following function will be part of the .fini section".
> 
> Alternatives:
> 
>    FINALIZE
>    UNINIT
> 
> But they have the same issue as RTE_CLEAN, IMO.

OK, you convinced me. And I remember now that you already convinced
me earlier with the same argument :)

Acked-by: Thomas Monjalon <thomas@monjalon.net>
diff mbox series

Patch

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 434adfd45..0dd832728 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -111,6 +111,29 @@  static void __attribute__((constructor(RTE_PRIO(prio)), used)) func(void)
 #define RTE_INIT(func) \
 	RTE_INIT_PRIO(func, LAST)
 
+/**
+ * Run after main() with low priority.
+ *
+ * @param func
+ *   Destructor function name.
+ * @param prio
+ *   Priority number must be above 100.
+ *   Lowest number is the last to run.
+ */
+#define RTE_FINI_PRIO(func, prio) \
+static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
+
+/**
+ * Run after main() with high priority.
+ *
+ * The destructor will be run *before* prioritized destructors.
+ *
+ * @param func
+ *   Destructor function name.
+ */
+#define RTE_FINI(func) \
+	RTE_FINI_PRIO(func, LAST)
+
 /**
  * Force a function to be inlined
  */