* [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive @ 2018-04-26 13:42 Anatoly Burakov 2018-04-26 14:30 ` Thomas Monjalon 2018-04-26 15:38 ` Stephen Hemminger 0 siblings, 2 replies; 7+ messages in thread From: Anatoly Burakov @ 2018-04-26 13:42 UTC (permalink / raw) To: dev Cc: Neil Horman, John McNamara, Marko Kovacevic, Robert Sanford, thomas, erik.g.carrillo, olivier.matz rte_lcore_has_role() returns 0 if role of lcore matches requested role. The return value of the API is confusing, and this is a known problem with a deprecation notice announcing the change to more intuitive semantics: Commit 064518f68d48 ("doc: announce EAL API change to lcore role function") Cc: erik.g.carrillo@intel.com Implement changes announced in the deprecation notice, and remove it. Also, fix usages of this API to reflect the change. Control thread patches expected new behavior and were broken before, now they are fixed as well. Fixes: d651ee4919cd ("eal: set affinity for control threads") Cc: olivier.matz@6wind.com Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> --- doc/guides/rel_notes/deprecation.rst | 5 ----- lib/librte_eal/common/eal_common_thread.c | 5 +---- lib/librte_timer/rte_timer.c | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst index 72ab33c..3353519 100644 --- a/doc/guides/rel_notes/deprecation.rst +++ b/doc/guides/rel_notes/deprecation.rst @@ -28,11 +28,6 @@ Deprecation Notices - ``eal_parse_pci_DomBDF`` replaced by ``rte_pci_addr_parse`` - ``rte_eal_compare_pci_addr`` replaced by ``rte_pci_addr_cmp`` -* eal: The semantics of the return value for the ``rte_lcore_has_role`` function - are planned to change in v18.05. The function currently returns 0 and <0 for - success and failure, respectively. This will change to 1 and 0 for true and - false, respectively, to make use of the function more intuitive. - * eal: a new set of mbuf mempool ops name APIs for user, platform and best mempool names have been defined in ``rte_mbuf`` in v18.02. The uses of ``rte_eal_mbuf_default_mempool_ops`` shall be replaced by diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c index 4e75cb8..fcf00cd 100644 --- a/lib/librte_eal/common/eal_common_thread.c +++ b/lib/librte_eal/common/eal_common_thread.c @@ -34,10 +34,7 @@ rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role) if (lcore_id >= RTE_MAX_LCORE) return -EINVAL; - if (cfg->lcore_role[lcore_id] == role) - return 0; - - return -EINVAL; + return cfg->lcore_role[lcore_id] == role; } int eal_cpuset_socket_id(rte_cpuset_t *cpusetp) diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c index 4bbcd06..590488c 100644 --- a/lib/librte_timer/rte_timer.c +++ b/lib/librte_timer/rte_timer.c @@ -403,7 +403,7 @@ rte_timer_reset(struct rte_timer *tim, uint64_t ticks, if (unlikely((tim_lcore != (unsigned)LCORE_ID_ANY) && !(rte_lcore_is_enabled(tim_lcore) || - rte_lcore_has_role(tim_lcore, ROLE_SERVICE) == 0))) + rte_lcore_has_role(tim_lcore, ROLE_SERVICE)))) return -1; if (type == PERIODICAL) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 13:42 [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive Anatoly Burakov @ 2018-04-26 14:30 ` Thomas Monjalon 2018-04-26 14:44 ` Carrillo, Erik G 2018-04-26 15:38 ` Stephen Hemminger 1 sibling, 1 reply; 7+ messages in thread From: Thomas Monjalon @ 2018-04-26 14:30 UTC (permalink / raw) To: Anatoly Burakov Cc: dev, Neil Horman, John McNamara, Marko Kovacevic, Robert Sanford, erik.g.carrillo, olivier.matz, shreyansh.jain 26/04/2018 15:42, Anatoly Burakov: > rte_lcore_has_role() returns 0 if role of lcore matches requested > role. The return value of the API is confusing, and this is a known > problem with a deprecation notice announcing the change to more > intuitive semantics: > > Commit 064518f68d48 ("doc: announce EAL API change to lcore role function") > Cc: erik.g.carrillo@intel.com > > Implement changes announced in the deprecation notice, and remove it. > Also, fix usages of this API to reflect the change. Control thread patches > expected new behavior and were broken before, now they are fixed as well. > > Fixes: d651ee4919cd ("eal: set affinity for control threads") > Cc: olivier.matz@6wind.com > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> Applied, thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 14:30 ` Thomas Monjalon @ 2018-04-26 14:44 ` Carrillo, Erik G 2018-04-26 14:54 ` Thomas Monjalon 0 siblings, 1 reply; 7+ messages in thread From: Carrillo, Erik G @ 2018-04-26 14:44 UTC (permalink / raw) To: Thomas Monjalon, Burakov, Anatoly Cc: dev, Neil Horman, Mcnamara, John, Kovacevic, Marko, Robert Sanford, olivier.matz, shreyansh.jain Thanks, Anatoly and Thomas. I had also considered the following chunk for the release notes: diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst index 04ff4fe..127a7e2 100644 --- a/doc/guides/rel_notes/release_18_05.rst +++ b/doc/guides/rel_notes/release_18_05.rst @@ -72,6 +72,11 @@ API Changes Also, make sure to start the actual text at the margin. ========================================================= +* **rte_lcore_has_role() return values changed** + + This function now returns 1 or 0 for true or false, respectively, rather + than 0 or <0 for success or failure to make use of the function more + intuitive. ABI Changes ----------- Do we want this note? Also, it looks like the Doxygen documentation of the function in the header file didn't get updated. Regards, Erik > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, April 26, 2018 9:31 AM > To: Burakov, Anatoly <anatoly.burakov@intel.com> > Cc: dev@dpdk.org; Neil Horman <nhorman@tuxdriver.com>; Mcnamara, > John <john.mcnamara@intel.com>; Kovacevic, Marko > <marko.kovacevic@intel.com>; Robert Sanford <rsanford@akamai.com>; > Carrillo, Erik G <erik.g.carrillo@intel.com>; olivier.matz@6wind.com; > shreyansh.jain@nxp.com > Subject: Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function > more intuitive > > 26/04/2018 15:42, Anatoly Burakov: > > rte_lcore_has_role() returns 0 if role of lcore matches requested > > role. The return value of the API is confusing, and this is a known > > problem with a deprecation notice announcing the change to more > > intuitive semantics: > > > > Commit 064518f68d48 ("doc: announce EAL API change to lcore role > > function") > > Cc: erik.g.carrillo@intel.com > > > > Implement changes announced in the deprecation notice, and remove it. > > Also, fix usages of this API to reflect the change. Control thread > > patches expected new behavior and were broken before, now they are > fixed as well. > > > > Fixes: d651ee4919cd ("eal: set affinity for control threads") > > Cc: olivier.matz@6wind.com > > > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> > > Applied, thanks > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 14:44 ` Carrillo, Erik G @ 2018-04-26 14:54 ` Thomas Monjalon 2018-04-26 14:56 ` Carrillo, Erik G 0 siblings, 1 reply; 7+ messages in thread From: Thomas Monjalon @ 2018-04-26 14:54 UTC (permalink / raw) To: Carrillo, Erik G Cc: Burakov, Anatoly, dev, Neil Horman, Mcnamara, John, Kovacevic, Marko, Robert Sanford, olivier.matz, shreyansh.jain 26/04/2018 16:44, Carrillo, Erik G: > Thanks, Anatoly and Thomas. I had also considered the following chunk for the release notes: > > diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst > index 04ff4fe..127a7e2 100644 > --- a/doc/guides/rel_notes/release_18_05.rst > +++ b/doc/guides/rel_notes/release_18_05.rst > @@ -72,6 +72,11 @@ API Changes > Also, make sure to start the actual text at the margin. > ========================================================= > > +* **rte_lcore_has_role() return values changed** > + > + This function now returns 1 or 0 for true or false, respectively, rather > + than 0 or <0 for success or failure to make use of the function more > + intuitive. > > ABI Changes > ----------- > > Do we want this note? Also, it looks like the Doxygen documentation of the function in the header file didn't get updated. Oh, you are right, this patch is not complete. I've fixed it: --- a/doc/guides/rel_notes/release_18_05.rst +++ b/doc/guides/rel_notes/release_18_05.rst @@ -161,6 +161,12 @@ API Changes announced at least one release before the ABI change is made. There are no ABI breaking changes planned. +* eal: ``rte_lcore_has_role()`` return value changed. + + This function now returns true or false, respectively, rather + than 0 or <0 for success or failure. + It makes use of the function more intuitive. + * mempool: capability flags and related functions have been removed. Flags ``MEMPOOL_F_CAPA_PHYS_CONTIG`` and diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h index 334a0629e..1a2f37eaa 100644 --- a/lib/librte_eal/common/include/rte_lcore.h +++ b/lib/librte_eal/common/include/rte_lcore.h @@ -311,7 +311,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, * @param role * The role to be checked against. * @return - * On success, return 0; otherwise return a negative value. + * Boolean value: positive if test is true; otherwise returns 0. */ Thanks Erik! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 14:54 ` Thomas Monjalon @ 2018-04-26 14:56 ` Carrillo, Erik G 2018-04-26 15:37 ` Stephen Hemminger 0 siblings, 1 reply; 7+ messages in thread From: Carrillo, Erik G @ 2018-04-26 14:56 UTC (permalink / raw) To: Thomas Monjalon Cc: Burakov, Anatoly, dev, Neil Horman, Mcnamara, John, Kovacevic, Marko, Robert Sanford, olivier.matz, shreyansh.jain Great, thanks Thomas. > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, April 26, 2018 9:55 AM > To: Carrillo, Erik G <erik.g.carrillo@intel.com> > Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Neil > Horman <nhorman@tuxdriver.com>; Mcnamara, John > <john.mcnamara@intel.com>; Kovacevic, Marko > <marko.kovacevic@intel.com>; Robert Sanford <rsanford@akamai.com>; > olivier.matz@6wind.com; shreyansh.jain@nxp.com > Subject: Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function > more intuitive > > 26/04/2018 16:44, Carrillo, Erik G: > > Thanks, Anatoly and Thomas. I had also considered the following chunk for > the release notes: > > > > diff --git a/doc/guides/rel_notes/release_18_05.rst > b/doc/guides/rel_notes/release_18_05.rst > > index 04ff4fe..127a7e2 100644 > > --- a/doc/guides/rel_notes/release_18_05.rst > > +++ b/doc/guides/rel_notes/release_18_05.rst > > @@ -72,6 +72,11 @@ API Changes > > Also, make sure to start the actual text at the margin. > > > ========================================================= > > > > +* **rte_lcore_has_role() return values changed** > > + > > + This function now returns 1 or 0 for true or false, respectively, rather > > + than 0 or <0 for success or failure to make use of the function more > > + intuitive. > > > > ABI Changes > > ----------- > > > > Do we want this note? Also, it looks like the Doxygen documentation of > the function in the header file didn't get updated. > > > Oh, you are right, this patch is not complete. > I've fixed it: > > --- a/doc/guides/rel_notes/release_18_05.rst > +++ b/doc/guides/rel_notes/release_18_05.rst > @@ -161,6 +161,12 @@ API Changes > announced at least one release before the ABI change is made. There are > no > ABI breaking changes planned. > > +* eal: ``rte_lcore_has_role()`` return value changed. > + > + This function now returns true or false, respectively, rather than 0 > + or <0 for success or failure. > + It makes use of the function more intuitive. > + > * mempool: capability flags and related functions have been removed. > > Flags ``MEMPOOL_F_CAPA_PHYS_CONTIG`` and diff --git > a/lib/librte_eal/common/include/rte_lcore.h > b/lib/librte_eal/common/include/rte_lcore.h > index 334a0629e..1a2f37eaa 100644 > --- a/lib/librte_eal/common/include/rte_lcore.h > +++ b/lib/librte_eal/common/include/rte_lcore.h > @@ -311,7 +311,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char > *name, > * @param role > * The role to be checked against. > * @return > - * On success, return 0; otherwise return a negative value. > + * Boolean value: positive if test is true; otherwise returns 0. > */ > > Thanks Erik! > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 14:56 ` Carrillo, Erik G @ 2018-04-26 15:37 ` Stephen Hemminger 0 siblings, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2018-04-26 15:37 UTC (permalink / raw) To: Carrillo, Erik G Cc: Thomas Monjalon, Burakov, Anatoly, dev, Neil Horman, Mcnamara, John, Kovacevic, Marko, Robert Sanford, olivier.matz, shreyansh.jain On Thu, 26 Apr 2018 14:56:19 +0000 "Carrillo, Erik G" <erik.g.carrillo@intel.com> wrote: > Great, thanks Thomas. > > > -----Original Message----- > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > Sent: Thursday, April 26, 2018 9:55 AM > > To: Carrillo, Erik G <erik.g.carrillo@intel.com> > > Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Neil > > Horman <nhorman@tuxdriver.com>; Mcnamara, John > > <john.mcnamara@intel.com>; Kovacevic, Marko > > <marko.kovacevic@intel.com>; Robert Sanford <rsanford@akamai.com>; > > olivier.matz@6wind.com; shreyansh.jain@nxp.com > > Subject: Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function > > more intuitive > > > > 26/04/2018 16:44, Carrillo, Erik G: > > > Thanks, Anatoly and Thomas. I had also considered the following chunk for > > the release notes: > > > > > > diff --git a/doc/guides/rel_notes/release_18_05.rst > > b/doc/guides/rel_notes/release_18_05.rst > > > index 04ff4fe..127a7e2 100644 > > > --- a/doc/guides/rel_notes/release_18_05.rst > > > +++ b/doc/guides/rel_notes/release_18_05.rst > > > @@ -72,6 +72,11 @@ API Changes > > > Also, make sure to start the actual text at the margin. > > > > > ========================================================= > > > > > > +* **rte_lcore_has_role() return values changed** > > > + > > > + This function now returns 1 or 0 for true or false, respectively, rather > > > + than 0 or <0 for success or failure to make use of the function more > > > + intuitive. > > > > > > ABI Changes > > > ----------- > > > > > > Do we want this note? Also, it looks like the Doxygen documentation of > > the function in the header file didn't get updated. > > > > > > Oh, you are right, this patch is not complete. > > I've fixed it: > > > > --- a/doc/guides/rel_notes/release_18_05.rst > > +++ b/doc/guides/rel_notes/release_18_05.rst > > @@ -161,6 +161,12 @@ API Changes > > announced at least one release before the ABI change is made. There are > > no > > ABI breaking changes planned. > > > > +* eal: ``rte_lcore_has_role()`` return value changed. > > + > > + This function now returns true or false, respectively, rather than 0 > > + or <0 for success or failure. > > + It makes use of the function more intuitive. > > + > > * mempool: capability flags and related functions have been removed. > > > > Flags ``MEMPOOL_F_CAPA_PHYS_CONTIG`` and diff --git > > a/lib/librte_eal/common/include/rte_lcore.h > > b/lib/librte_eal/common/include/rte_lcore.h > > index 334a0629e..1a2f37eaa 100644 > > --- a/lib/librte_eal/common/include/rte_lcore.h > > +++ b/lib/librte_eal/common/include/rte_lcore.h > > @@ -311,7 +311,7 @@ rte_ctrl_thread_create(pthread_t *thread, const char > > *name, > > * @param role > > * The role to be checked against. > > * @return > > - * On success, return 0; otherwise return a negative value. > > + * Boolean value: positive if test is true; otherwise returns 0. > > */ > > > > Thanks Erik! > > > Usually the safest way to introduce this is introduce a new function with a different name. Then retire the old one. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive 2018-04-26 13:42 [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive Anatoly Burakov 2018-04-26 14:30 ` Thomas Monjalon @ 2018-04-26 15:38 ` Stephen Hemminger 1 sibling, 0 replies; 7+ messages in thread From: Stephen Hemminger @ 2018-04-26 15:38 UTC (permalink / raw) To: Anatoly Burakov Cc: dev, Neil Horman, John McNamara, Marko Kovacevic, Robert Sanford, thomas, erik.g.carrillo, olivier.matz On Thu, 26 Apr 2018 14:42:31 +0100 Anatoly Burakov <anatoly.burakov@intel.com> wrote: > rte_lcore_has_role() returns 0 if role of lcore matches requested > role. The return value of the API is confusing, and this is a known > problem with a deprecation notice announcing the change to more > intuitive semantics: > > Commit 064518f68d48 ("doc: announce EAL API change to lcore role function") > Cc: erik.g.carrillo@intel.com > > Implement changes announced in the deprecation notice, and remove it. > Also, fix usages of this API to reflect the change. Control thread patches > expected new behavior and were broken before, now they are fixed as well. > > Fixes: d651ee4919cd ("eal: set affinity for control threads") > Cc: olivier.matz@6wind.com > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com> It would make more sense if rte_lcore_has_role returned a bool ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-26 15:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-26 13:42 [dpdk-dev] [PATCH] lcore: make semantics of lcore role function more intuitive Anatoly Burakov 2018-04-26 14:30 ` Thomas Monjalon 2018-04-26 14:44 ` Carrillo, Erik G 2018-04-26 14:54 ` Thomas Monjalon 2018-04-26 14:56 ` Carrillo, Erik G 2018-04-26 15:37 ` Stephen Hemminger 2018-04-26 15:38 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).