| Message ID | 20210730214453.19975-1-honnappa.nagarahalli@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | doc: abstract the behaviour of rte_ctrl_thread_create | expand |
> -----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>
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>
<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>
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.
<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. >
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
<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
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.
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(+) -- 2.17.1