[dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Honnappa Nagarahalli]

The current expected behaviour of the function rte_ctrl_thread_create
is rigid which makes the implementation of the function complex.
Make the expected behaviour abstract to allow for simplified
implementation.

With this change, the calls to pthread_setaffinity_np can be moved
to the control thread. This will avoid the use of
pthread_barrier_wait and simplify the synchronization mechanism
between rte_ctrl_thread_create and the calling thread.

Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
Possible patch is at:
http://patches.dpdk.org/project/dpdk/patch/20210730213709.19400-1-honnappa.nagarahalli@arm.com/

 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 9584d6bfd7..1960e3c8bf 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -11,6 +11,13 @@ here.
 Deprecation Notices
 -------------------
 
+* eal: The expected behaviour of the function ``rte_ctrl_thread_create``
+  abstracted to allow for simplified implementation. The new behaviour is
+  as follows:
+  Creates a control thread with the given name. The affinity of the new
+  thread is based on the CPU affinity retrieved at the time rte_eal_init()
+  was called, the dataplane and service lcores are then excluded.
+
 * kvargs: The function ``rte_kvargs_process`` will get a new parameter
   for returning key match count. It will ease handling of no-match case.
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Ruifeng Wang]

> -----Original Message-----
> From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> Sent: Saturday, July 31, 2021 5:45 AM
> To: dev@dpdk.org; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> lucp.at.work@gmail.com; david.marchand@redhat.com;
> thomas@monjalon.net
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> Subject: [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create
> 
> The current expected behaviour of the function rte_ctrl_thread_create is
> rigid which makes the implementation of the function complex.
> Make the expected behaviour abstract to allow for simplified
> implementation.
> 
> With this change, the calls to pthread_setaffinity_np can be moved to the
> control thread. This will avoid the use of pthread_barrier_wait and simplify
> the synchronization mechanism between rte_ctrl_thread_create and the
> calling thread.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> Possible patch is at:
> http://patches.dpdk.org/project/dpdk/patch/20210730213709.19400-1-
> honnappa.nagarahalli@arm.com/
> 
>  doc/guides/rel_notes/deprecation.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 9584d6bfd7..1960e3c8bf 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -11,6 +11,13 @@ here.
>  Deprecation Notices
>  -------------------
> 
> +* eal: The expected behaviour of the function
> +``rte_ctrl_thread_create``
> +  abstracted to allow for simplified implementation. The new behaviour
> +is
> +  as follows:
> +  Creates a control thread with the given name. The affinity of the new
> +  thread is based on the CPU affinity retrieved at the time
> +rte_eal_init()
> +  was called, the dataplane and service lcores are then excluded.
> +
>  * kvargs: The function ``rte_kvargs_process`` will get a new parameter
>    for returning key match count. It will ease handling of no-match case.
> 
> --
> 2.17.1
Acked-by: Ruifeng Wang <ruifeng.wang@arm.com>

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Jerin Jacob]

On Tue, Aug 3, 2021 at 11:24 AM Ruifeng Wang <Ruifeng.Wang@arm.com> wrote:
>
> > -----Original Message-----
> > From: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > Sent: Saturday, July 31, 2021 5:45 AM
> > To: dev@dpdk.org; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; olivier.matz@6wind.com;
> > lucp.at.work@gmail.com; david.marchand@redhat.com;
> > thomas@monjalon.net
> > Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; nd <nd@arm.com>
> > Subject: [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create
> >
> > The current expected behaviour of the function rte_ctrl_thread_create is
> > rigid which makes the implementation of the function complex.
> > Make the expected behaviour abstract to allow for simplified
> > implementation.
> >
> > With this change, the calls to pthread_setaffinity_np can be moved to the
> > control thread. This will avoid the use of pthread_barrier_wait and simplify
> > the synchronization mechanism between rte_ctrl_thread_create and the
> > calling thread.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> > Possible patch is at:
> > http://patches.dpdk.org/project/dpdk/patch/20210730213709.19400-1-
> > honnappa.nagarahalli@arm.com/
> >
> >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 9584d6bfd7..1960e3c8bf 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -11,6 +11,13 @@ here.
> >  Deprecation Notices
> >  -------------------
> >
> > +* eal: The expected behaviour of the function
> > +``rte_ctrl_thread_create``
> > +  abstracted to allow for simplified implementation. The new behaviour
> > +is
> > +  as follows:
> > +  Creates a control thread with the given name. The affinity of the new
> > +  thread is based on the CPU affinity retrieved at the time
> > +rte_eal_init()
> > +  was called, the dataplane and service lcores are then excluded.
> > +
> >  * kvargs: The function ``rte_kvargs_process`` will get a new parameter
> >    for returning key match count. It will ease handling of no-match case.
> >
> > --
> > 2.17.1
> Acked-by: Ruifeng Wang <ruifeng.wang@arm.com>

Acked-by: Jerin Jacob <jerinj@marvell.com>

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

Hi Olivier,
	Any comments on this?

Thanks,
Honnappa

> > >
> > > The current expected behaviour of the function
> > > rte_ctrl_thread_create is rigid which makes the implementation of the
> function complex.
> > > Make the expected behaviour abstract to allow for simplified
> > > implementation.
> > >
> > > With this change, the calls to pthread_setaffinity_np can be moved
> > > to the control thread. This will avoid the use of
> > > pthread_barrier_wait and simplify the synchronization mechanism
> > > between rte_ctrl_thread_create and the calling thread.
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > ---
> > > Possible patch is at:
> > > http://patches.dpdk.org/project/dpdk/patch/20210730213709.19400-1-
> > > honnappa.nagarahalli@arm.com/
> > >
> > >  doc/guides/rel_notes/deprecation.rst | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index 9584d6bfd7..1960e3c8bf 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -11,6 +11,13 @@ here.
> > >  Deprecation Notices
> > >  -------------------
> > >
> > > +* eal: The expected behaviour of the function
> > > +``rte_ctrl_thread_create``
> > > +  abstracted to allow for simplified implementation. The new
> > > +behaviour is
> > > +  as follows:
> > > +  Creates a control thread with the given name. The affinity of the
> > > +new
> > > +  thread is based on the CPU affinity retrieved at the time
> > > +rte_eal_init()
> > > +  was called, the dataplane and service lcores are then excluded.
> > > +
> > >  * kvargs: The function ``rte_kvargs_process`` will get a new parameter
> > >    for returning key match count. It will ease handling of no-match case.
> > >
> > > --
> > > 2.17.1
> > Acked-by: Ruifeng Wang <ruifeng.wang@arm.com>
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Thomas Monjalon]

30/07/2021 23:44, Honnappa Nagarahalli:
> The current expected behaviour of the function rte_ctrl_thread_create
> is rigid which makes the implementation of the function complex.
> Make the expected behaviour abstract to allow for simplified
> implementation.
> 
> With this change, the calls to pthread_setaffinity_np can be moved
> to the control thread. This will avoid the use of
> pthread_barrier_wait and simplify the synchronization mechanism
> between rte_ctrl_thread_create and the calling thread.
> 
> Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
> +* eal: The expected behaviour of the function ``rte_ctrl_thread_create``
> +  abstracted to allow for simplified implementation. The new behaviour is
> +  as follows:
> +  Creates a control thread with the given name. The affinity of the new
> +  thread is based on the CPU affinity retrieved at the time rte_eal_init()
> +  was called, the dataplane and service lcores are then excluded.

I don't understand what is different of the current API:
 * Wrapper to pthread_create(), pthread_setname_np() and
 * pthread_setaffinity_np(). The affinity of the new thread is based
 * on the CPU affinity retrieved at the time rte_eal_init() was called,
 * the dataplane and service lcores are then excluded.

Anyway, there is not enough meaningful acks.

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>
> 
> 30/07/2021 23:44, Honnappa Nagarahalli:
> > The current expected behaviour of the function rte_ctrl_thread_create
> > is rigid which makes the implementation of the function complex.
> > Make the expected behaviour abstract to allow for simplified
> > implementation.
> >
> > With this change, the calls to pthread_setaffinity_np can be moved to
> > the control thread. This will avoid the use of pthread_barrier_wait
> > and simplify the synchronization mechanism between
> > rte_ctrl_thread_create and the calling thread.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > ---
> > +* eal: The expected behaviour of the function
> > +``rte_ctrl_thread_create``
> > +  abstracted to allow for simplified implementation. The new
> > +behaviour is
> > +  as follows:
> > +  Creates a control thread with the given name. The affinity of the
> > +new
> > +  thread is based on the CPU affinity retrieved at the time
> > +rte_eal_init()
> > +  was called, the dataplane and service lcores are then excluded.
> 
> I don't understand what is different of the current API:
>  * Wrapper to pthread_create(), pthread_setname_np() and
>  * pthread_setaffinity_np(). The affinity of the new thread is based
>  * on the CPU affinity retrieved at the time rte_eal_init() was called,
>  * the dataplane and service lcores are then excluded.
My concern is for the word "Wrapper". I am not sure how much we are bound by that to keep the code as a "wrapper".
The new patch does not change the high level behavior.

Are you saying you are ok with the patch without the deprecation notice?

> 
> Anyway, there is not enough meaningful acks.
> 

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Olivier Matz]

Hi Honnappa,

Back from holidays, sorry for the late answer.

On Mon, Aug 09, 2021 at 01:18:42PM +0000, Honnappa Nagarahalli wrote:
> <snip>
> > 
> > 30/07/2021 23:44, Honnappa Nagarahalli:
> > > The current expected behaviour of the function rte_ctrl_thread_create
> > > is rigid which makes the implementation of the function complex.
> > > Make the expected behaviour abstract to allow for simplified
> > > implementation.
> > >
> > > With this change, the calls to pthread_setaffinity_np can be moved to
> > > the control thread. This will avoid the use of pthread_barrier_wait
> > > and simplify the synchronization mechanism between
> > > rte_ctrl_thread_create and the calling thread.
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > ---
> > > +* eal: The expected behaviour of the function
> > > +``rte_ctrl_thread_create``
> > > +  abstracted to allow for simplified implementation. The new
> > > +behaviour is
> > > +  as follows:
> > > +  Creates a control thread with the given name. The affinity of the
> > > +new
> > > +  thread is based on the CPU affinity retrieved at the time
> > > +rte_eal_init()
> > > +  was called, the dataplane and service lcores are then excluded.
> > 
> > I don't understand what is different of the current API:
> >  * Wrapper to pthread_create(), pthread_setname_np() and
> >  * pthread_setaffinity_np(). The affinity of the new thread is based
> >  * on the CPU affinity retrieved at the time rte_eal_init() was called,
> >  * the dataplane and service lcores are then excluded.
> My concern is for the word "Wrapper". I am not sure how much we are bound by that to keep the code as a "wrapper".
> The new patch does not change the high level behavior.

I am ok to remove the word "wrapper" from the description, and I agree
it can be better described without quoting the pthread_* functions.

> Are you saying you are ok with the patch without the deprecation notice?

I don't think it requires a deprecation notice if the API and ABI is
left unchanged. To be honnest, I find a bit hard to understand what is
really changed by reading the deprecation notice:

> +* eal: The expected behaviour of the function ``rte_ctrl_thread_create``
> +  abstracted to allow for simplified implementation. The new behaviour is
> +  as follows:
> +  Creates a control thread with the given name. The affinity of the new
> +  thread is based on the CPU affinity retrieved at the time rte_eal_init()
> +  was called, the dataplane and service lcores are then excluded.

I'll send my comments to your patch:
http://patches.dpdk.org/project/dpdk/patch/20210802051652.3611-1-honnappa.nagarahalli@arm.com/


Thanks,
Olivier

Re: [dpdk-dev] [PATCH] doc: abstract the behaviour of rte_ctrl_thread_create

[Honnappa Nagarahalli]

<snip>

> > >
> > > 30/07/2021 23:44, Honnappa Nagarahalli:
> > > > The current expected behaviour of the function
> > > > rte_ctrl_thread_create is rigid which makes the implementation of the
> function complex.
> > > > Make the expected behaviour abstract to allow for simplified
> > > > implementation.
> > > >
> > > > With this change, the calls to pthread_setaffinity_np can be moved
> > > > to the control thread. This will avoid the use of
> > > > pthread_barrier_wait and simplify the synchronization mechanism
> > > > between rte_ctrl_thread_create and the calling thread.
> > > >
> > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > ---
> > > > +* eal: The expected behaviour of the function
> > > > +``rte_ctrl_thread_create``
> > > > +  abstracted to allow for simplified implementation. The new
> > > > +behaviour is
> > > > +  as follows:
> > > > +  Creates a control thread with the given name. The affinity of
> > > > +the new
> > > > +  thread is based on the CPU affinity retrieved at the time
> > > > +rte_eal_init()
> > > > +  was called, the dataplane and service lcores are then excluded.
> > >
> > > I don't understand what is different of the current API:
> > >  * Wrapper to pthread_create(), pthread_setname_np() and
> > >  * pthread_setaffinity_np(). The affinity of the new thread is based
> > >  * on the CPU affinity retrieved at the time rte_eal_init() was
> > > called,
> > >  * the dataplane and service lcores are then excluded.
> > My concern is for the word "Wrapper". I am not sure how much we are
> bound by that to keep the code as a "wrapper".
> > The new patch does not change the high level behavior.
> 
> I am ok to remove the word "wrapper" from the description, and I agree it can
> be better described without quoting the pthread_* functions.
> 
> > Are you saying you are ok with the patch without the deprecation notice?
> 
> I don't think it requires a deprecation notice if the API and ABI is left
> unchanged. To be honnest, I find a bit hard to understand what is really
> changed by reading the deprecation notice:
Thanks Olivier. I agree, I was also not sure. The term "wrapper" made me feel that we are defining certain return codes to the application.

At the macro level, I think the expected behavior remains the same.

> 
> > +* eal: The expected behaviour of the function
> > +``rte_ctrl_thread_create``
> > +  abstracted to allow for simplified implementation. The new
> > +behaviour is
> > +  as follows:
> > +  Creates a control thread with the given name. The affinity of the
> > +new
> > +  thread is based on the CPU affinity retrieved at the time
> > +rte_eal_init()
> > +  was called, the dataplane and service lcores are then excluded.
> 
> I'll send my comments to your patch:
> http://patches.dpdk.org/project/dpdk/patch/20210802051652.3611-1-
> honnappa.nagarahalli@arm.com/
> 
> 
> Thanks,
> Olivier