| Message ID | 1451320813-9428-3-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 12/29/2015 11:06, Elo, Matias (Nokia - FI/Espoo) wrote: >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT Maxim >> Uvarov >> Sent: Monday, December 28, 2015 6:40 PM >> To: lng-odp@lists.linaro.org >> Subject: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >> odph_pktio_wailt_linkup() helper to wait link up >> >> Define pktio helper to wait link up after odp_pktio_start(). >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >> --- >> helper/Makefile.am | 1 + >> helper/include/odp/helper/pktio.h | 65 >> +++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 66 insertions(+) >> create mode 100644 helper/include/odp/helper/pktio.h >> >> diff --git a/helper/Makefile.am b/helper/Makefile.am >> index 1906ae2..c01f43a 100644 >> --- a/helper/Makefile.am >> +++ b/helper/Makefile.am >> @@ -18,6 +18,7 @@ helperinclude_HEADERS = \ >> $(srcdir)/include/odp/helper/strong_types.h\ >> $(srcdir)/include/odp/helper/tcp.h\ >> $(srcdir)/include/odp/helper/table.h\ >> + $(srcdir)/include/odp/helper/pktio.h\ >> $(srcdir)/include/odp/helper/udp.h >> >> noinst_HEADERS = \ >> diff --git a/helper/include/odp/helper/pktio.h >> b/helper/include/odp/helper/pktio.h >> new file mode 100644 >> index 0000000..18e6f94 >> --- /dev/null >> +++ b/helper/include/odp/helper/pktio.h >> @@ -0,0 +1,65 @@ >> +/* Copyright (c) 2015, Linaro Limited >> + * All rights reserved. >> + * >> + * SPDX-License-Identifier: BSD-3-Clause >> + */ >> + >> +/** >> + * @file >> + * >> + * ODP pktio helper API >> + * >> + */ >> + >> +#ifndef ODPH_PKTIO_H_ >> +#define ODPH_PKTIO_H_ >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include <odp/time.h> >> +#include <odp/packet_io.h> >> + >> +/** time to wait after odp_pktio_start() to bring link up. >> + * It's is hardware dependent value, here we will have some common > First letter in capital, remove double 'is'. Ok. > >> + * value. >> + */ >> +#define _ODPH_PKTIO_LINK_UP_TIME (50 * ODP_TIME_MSEC_IN_NS) >> + >> +/** Number of odp_pktio_link_status() calls in time interval. */ >> +#define _ODPH_PKTIO_LINK_TRIES 5 >> + >> +/** >> + * Wait for pktio link up. >> + * >> + * After odp_pktio_link_start(pktio) some platforms need small delay >> + * to bring up corresponding pktio link. >> + * >> + * @param pktio Packet IO handle. >> + * >> + * @retval 1 link is up >> + * @retval 0 link is down >> + * @retval <0 on failure >> +*/ >> +static inline int odph_pktio_wait_linkup(odp_pktio_t pktio) >> +{ >> + uint64_t wait_ns = _ODPH_PKTIO_LINK_UP_TIME / >> _ODPH_PKTIO_LINK_TRIES; >> + int i; >> + int ret = -1; >> + >> + for (i = 0; i < _ODPH_PKTIO_LINK_TRIES; i++) { >> + ret = odp_pktio_link_status(pktio); >> + if (ret < 0 || ret == 1) >> + return ret; >> + /* link is down, call status again after delay */ >> + odp_time_wait_ns(wait_ns); >> + } >> + return ret; >> +} > I was originally thinking simply adding the helper function to the pktio.c validation test directly. However, this function could be useful in the pktio API. In this case the timeout value should be a function argument (e.g. uint64_t nanosec) I think that each pktio should have it's own link up time. Maybe to introduce uint64_tt nonosec odp_pktio_link_up_time() or odp_pktio_conf_link_up_time(). Or odp_pktio_link_status_wait(pktio). I will send RFC for that so we can discuss. I did not add it to pktio.c due all tests and examples need that link wait (like generator, packet and etc.). So I added it to helper assuming that 50 milliseconds is enough time to wait. That should allow to merge odp_pktio_link_status() implementation and validation tests. And wait function can be in separate patch. What do you think? Maxim. >> + >> +#ifdef __cplusplus >> +} >> +#endif >> + >> +#endif >> -- >> 1.9.1 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
On 12/29/2015 14:30, Elo, Matias (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >> Sent: Tuesday, December 29, 2015 12:56 PM >> To: Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com>; lng- >> odp@lists.linaro.org >> Subject: Re: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >> odph_pktio_wailt_linkup() helper to wait link up >> >> On 12/29/2015 11:06, Elo, Matias (Nokia - FI/Espoo) wrote: >>>> -----Original Message----- >>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Maxim >>>> Uvarov >>>> Sent: Monday, December 28, 2015 6:40 PM >>>> To: lng-odp@lists.linaro.org >>>> Subject: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >>>> odph_pktio_wailt_linkup() helper to wait link up >>>> >>>> Define pktio helper to wait link up after odp_pktio_start(). >>>> >>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>>> --- >>>> helper/Makefile.am | 1 + >>>> helper/include/odp/helper/pktio.h | 65 >>>> +++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 66 insertions(+) >>>> create mode 100644 helper/include/odp/helper/pktio.h >>>> >>>> diff --git a/helper/Makefile.am b/helper/Makefile.am >>>> index 1906ae2..c01f43a 100644 >>>> --- a/helper/Makefile.am >>>> +++ b/helper/Makefile.am >>>> @@ -18,6 +18,7 @@ helperinclude_HEADERS = \ >>>> $(srcdir)/include/odp/helper/strong_types.h\ >>>> $(srcdir)/include/odp/helper/tcp.h\ >>>> $(srcdir)/include/odp/helper/table.h\ >>>> + $(srcdir)/include/odp/helper/pktio.h\ >>>> $(srcdir)/include/odp/helper/udp.h >>>> >>>> noinst_HEADERS = \ >>>> diff --git a/helper/include/odp/helper/pktio.h >>>> b/helper/include/odp/helper/pktio.h >>>> new file mode 100644 >>>> index 0000000..18e6f94 >>>> --- /dev/null >>>> +++ b/helper/include/odp/helper/pktio.h >>>> @@ -0,0 +1,65 @@ >>>> +/* Copyright (c) 2015, Linaro Limited >>>> + * All rights reserved. >>>> + * >>>> + * SPDX-License-Identifier: BSD-3-Clause >>>> + */ >>>> + >>>> +/** >>>> + * @file >>>> + * >>>> + * ODP pktio helper API >>>> + * >>>> + */ >>>> + >>>> +#ifndef ODPH_PKTIO_H_ >>>> +#define ODPH_PKTIO_H_ >>>> + >>>> +#ifdef __cplusplus >>>> +extern "C" { >>>> +#endif >>>> + >>>> +#include <odp/time.h> >>>> +#include <odp/packet_io.h> >>>> + >>>> +/** time to wait after odp_pktio_start() to bring link up. >>>> + * It's is hardware dependent value, here we will have some common >>> First letter in capital, remove double 'is'. >> Ok. >> >>>> + * value. >>>> + */ >>>> +#define _ODPH_PKTIO_LINK_UP_TIME (50 * ODP_TIME_MSEC_IN_NS) >>>> + >>>> +/** Number of odp_pktio_link_status() calls in time interval. */ >>>> +#define _ODPH_PKTIO_LINK_TRIES 5 >>>> + >>>> +/** >>>> + * Wait for pktio link up. >>>> + * >>>> + * After odp_pktio_link_start(pktio) some platforms need small delay >>>> + * to bring up corresponding pktio link. >>>> + * >>>> + * @param pktio Packet IO handle. >>>> + * >>>> + * @retval 1 link is up >>>> + * @retval 0 link is down >>>> + * @retval <0 on failure >>>> +*/ >>>> +static inline int odph_pktio_wait_linkup(odp_pktio_t pktio) >>>> +{ >>>> + uint64_t wait_ns = _ODPH_PKTIO_LINK_UP_TIME / >>>> _ODPH_PKTIO_LINK_TRIES; >>>> + int i; >>>> + int ret = -1; >>>> + >>>> + for (i = 0; i < _ODPH_PKTIO_LINK_TRIES; i++) { >>>> + ret = odp_pktio_link_status(pktio); >>>> + if (ret < 0 || ret == 1) >>>> + return ret; >>>> + /* link is down, call status again after delay */ >>>> + odp_time_wait_ns(wait_ns); >>>> + } >>>> + return ret; >>>> +} >>> I was originally thinking simply adding the helper function to the pktio.c >> validation test directly. However, this function could be useful in the pktio API. In >> this case the timeout value should be a function argument (e.g. uint64_t >> nanosec) >> >> I think that each pktio should have it's own link up time. Maybe to >> introduce uint64_tt nonosec odp_pktio_link_up_time() or >> odp_pktio_conf_link_up_time(). Or odp_pktio_link_status_wait(pktio). I >> will send RFC for that so we can discuss. > The link up time depends on the opposing end of the link, so there is no way > of knowing how long it will take. Instead of that I would suggest adding a new > pktio API call like: > > int odp_pktio_wait_linkup(odp_pktio_t pktio, uint64_t timeout_ns) > >> I did not add it to pktio.c due all tests and examples need that link >> wait (like generator, packet and etc.). So I added it to helper assuming >> that 50 milliseconds is enough time to wait. That should allow to merge >> odp_pktio_link_status() implementation and validation tests. And >> wait function can be in separate patch. >> >> What do you think? > As mentioned above I would add the new API call odp_pktio_wait_linkup() and > use that in all the tests and examples. This could be done in a separate patch. > > For this patch set you could move odph_pktio_wait_linkup() functionality to pktio.c > as a stopgap until the new API function is available. > > -Matias How in that case apps will be portable? I.e. which timeout they will use? If just adding timeout required why not move that to odp helper instead of introducing new call with argument which is not clear value settings? If we can not calculate time. Than my original approach looks more clean. Just have some common value for timeout and use it everywhere. Maxim. > >> Maxim. >> >>>> + >>>> +#ifdef __cplusplus >>>> +} >>>> +#endif >>>> + >>>> +#endif >>>> -- >>>> 1.9.1 >>>> >>>> _______________________________________________ >>>> lng-odp mailing list >>>> lng-odp@lists.linaro.org >>>> https://lists.linaro.org/mailman/listinfo/lng-odp
On 12/29/2015 15:14, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >> -----Original Message----- >> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of EXT >> Maxim Uvarov >> Sent: Tuesday, December 29, 2015 1:46 PM >> To: Elo, Matias (Nokia - FI/Espoo); lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >> odph_pktio_wailt_linkup() helper to wait link up >> >> On 12/29/2015 14:30, Elo, Matias (Nokia - FI/Espoo) wrote: >>>> -----Original Message----- >>>> From: EXT Maxim Uvarov [mailto:maxim.uvarov@linaro.org] >>>> Sent: Tuesday, December 29, 2015 12:56 PM >>>> To: Elo, Matias (Nokia - FI/Espoo) <matias.elo@nokia.com>; lng- >>>> odp@lists.linaro.org >>>> Subject: Re: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >>>> odph_pktio_wailt_linkup() helper to wait link up >>>> >>>> On 12/29/2015 11:06, Elo, Matias (Nokia - FI/Espoo) wrote: >>>>>> -----Original Message----- >>>>>> From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> EXT >>>> Maxim >>>>>> Uvarov >>>>>> Sent: Monday, December 28, 2015 6:40 PM >>>>>> To: lng-odp@lists.linaro.org >>>>>> Subject: [lng-odp] [API-NEXT PATCHv2 2/4] helper: define >>>>>> odph_pktio_wailt_linkup() helper to wait link up >>>>>> >>>>>> Define pktio helper to wait link up after odp_pktio_start(). >>>>>> >>>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> >>>>>> --- >>>>>> helper/Makefile.am | 1 + >>>>>> helper/include/odp/helper/pktio.h | 65 >>>>>> +++++++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 66 insertions(+) >>>>>> create mode 100644 helper/include/odp/helper/pktio.h >>>>>> >>>>>> diff --git a/helper/Makefile.am b/helper/Makefile.am >>>>>> index 1906ae2..c01f43a 100644 >>>>>> --- a/helper/Makefile.am >>>>>> +++ b/helper/Makefile.am >>>>>> @@ -18,6 +18,7 @@ helperinclude_HEADERS = \ >>>>>> $(srcdir)/include/odp/helper/strong_types.h\ >>>>>> $(srcdir)/include/odp/helper/tcp.h\ >>>>>> $(srcdir)/include/odp/helper/table.h\ >>>>>> + $(srcdir)/include/odp/helper/pktio.h\ >>>>>> $(srcdir)/include/odp/helper/udp.h >>>>>> >>>>>> noinst_HEADERS = \ >>>>>> diff --git a/helper/include/odp/helper/pktio.h >>>>>> b/helper/include/odp/helper/pktio.h >>>>>> new file mode 100644 >>>>>> index 0000000..18e6f94 >>>>>> --- /dev/null >>>>>> +++ b/helper/include/odp/helper/pktio.h >>>>>> @@ -0,0 +1,65 @@ >>>>>> +/* Copyright (c) 2015, Linaro Limited >>>>>> + * All rights reserved. >>>>>> + * >>>>>> + * SPDX-License-Identifier: BSD-3-Clause >>>>>> + */ >>>>>> + >>>>>> +/** >>>>>> + * @file >>>>>> + * >>>>>> + * ODP pktio helper API >>>>>> + * >>>>>> + */ >>>>>> + >>>>>> +#ifndef ODPH_PKTIO_H_ >>>>>> +#define ODPH_PKTIO_H_ >>>>>> + >>>>>> +#ifdef __cplusplus >>>>>> +extern "C" { >>>>>> +#endif >>>>>> + >>>>>> +#include <odp/time.h> >>>>>> +#include <odp/packet_io.h> >>>>>> + >>>>>> +/** time to wait after odp_pktio_start() to bring link up. >>>>>> + * It's is hardware dependent value, here we will have some common >>>>> First letter in capital, remove double 'is'. >>>> Ok. >>>> >>>>>> + * value. >>>>>> + */ >>>>>> +#define _ODPH_PKTIO_LINK_UP_TIME (50 * ODP_TIME_MSEC_IN_NS) >>>>>> + >>>>>> +/** Number of odp_pktio_link_status() calls in time interval. */ >>>>>> +#define _ODPH_PKTIO_LINK_TRIES 5 >>>>>> + >>>>>> +/** >>>>>> + * Wait for pktio link up. >>>>>> + * >>>>>> + * After odp_pktio_link_start(pktio) some platforms need small delay >>>>>> + * to bring up corresponding pktio link. >>>>>> + * >>>>>> + * @param pktio Packet IO handle. >>>>>> + * >>>>>> + * @retval 1 link is up >>>>>> + * @retval 0 link is down >>>>>> + * @retval <0 on failure >>>>>> +*/ >>>>>> +static inline int odph_pktio_wait_linkup(odp_pktio_t pktio) >>>>>> +{ >>>>>> + uint64_t wait_ns = _ODPH_PKTIO_LINK_UP_TIME / >>>>>> _ODPH_PKTIO_LINK_TRIES; >>>>>> + int i; >>>>>> + int ret = -1; >>>>>> + >>>>>> + for (i = 0; i < _ODPH_PKTIO_LINK_TRIES; i++) { >>>>>> + ret = odp_pktio_link_status(pktio); >>>>>> + if (ret < 0 || ret == 1) >>>>>> + return ret; >>>>>> + /* link is down, call status again after delay */ >>>>>> + odp_time_wait_ns(wait_ns); >>>>>> + } >>>>>> + return ret; >>>>>> +} >>>>> I was originally thinking simply adding the helper function to the >> pktio.c >>>> validation test directly. However, this function could be useful in the >> pktio API. In >>>> this case the timeout value should be a function argument (e.g. >> uint64_t >>>> nanosec) >>>> >>>> I think that each pktio should have it's own link up time. Maybe to >>>> introduce uint64_tt nonosec odp_pktio_link_up_time() or >>>> odp_pktio_conf_link_up_time(). Or odp_pktio_link_status_wait(pktio). I >>>> will send RFC for that so we can discuss. >>> The link up time depends on the opposing end of the link, so there is >> no way >>> of knowing how long it will take. Instead of that I would suggest adding >> a new >>> pktio API call like: >>> >>> int odp_pktio_wait_linkup(odp_pktio_t pktio, uint64_t timeout_ns) >>> >>>> I did not add it to pktio.c due all tests and examples need that link >>>> wait (like generator, packet and etc.). So I added it to helper >> assuming >>>> that 50 milliseconds is enough time to wait. That should allow to >> merge >>>> odp_pktio_link_status() implementation and validation tests. And >>>> wait function can be in separate patch. >>>> >>>> What do you think? >>> As mentioned above I would add the new API call odp_pktio_wait_linkup() >> and >>> use that in all the tests and examples. This could be done in a separate >> patch. >>> For this patch set you could move odph_pktio_wait_linkup() functionality >> to pktio.c >>> as a stopgap until the new API function is available. >>> >>> -Matias >> How in that case apps will be portable? I.e. which timeout they will use? >> If just adding timeout required why not move that to odp helper instead of >> introducing new call with argument which is not clear value settings? >> >> If we can not calculate time. Than my original approach looks more clean. >> Just have some common value for timeout and use it everywhere. > > > It's application dependent how long time it can wait for a link to come up. As Matias said, a link comes up when both ends of the link are up (application may not be able to affect the other side of the link). Some application may not be able to wait more than 1ms before it has to do some counter measure, another app (like our validation tests) may be able to wait for several seconds. Hence, the link wait function needs a time parameter. > > Also it's common enough to wait until link comes up that a proper API call is better option than a helper call. An implementation may set up interrupts and timers to wait a link instead of busy looping. Helper would always busy loop. > > It's better to add this directly to API instead of adding it first as helper, then add it to API and remove from helpers. > > So, our suggestion is > - short term: test code internal function > - long term: a new API call (with nsec param) > > > -Petri > ok will send v3. Maxim. > > > >> Maxim. >>>> Maxim. >>>> >>>>>> + >>>>>> +#ifdef __cplusplus >>>>>> +} >>>>>> +#endif >>>>>> + >>>>>> +#endif >>>>>> -- >>>>>> 1.9.1 >>>>>> >>>>>> _______________________________________________ >>>>>> lng-odp mailing list >>>>>> lng-odp@lists.linaro.org >>>>>> https://lists.linaro.org/mailman/listinfo/lng-odp >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/helper/Makefile.am b/helper/Makefile.am index 1906ae2..c01f43a 100644 --- a/helper/Makefile.am +++ b/helper/Makefile.am @@ -18,6 +18,7 @@ helperinclude_HEADERS = \ $(srcdir)/include/odp/helper/strong_types.h\ $(srcdir)/include/odp/helper/tcp.h\ $(srcdir)/include/odp/helper/table.h\ + $(srcdir)/include/odp/helper/pktio.h\ $(srcdir)/include/odp/helper/udp.h noinst_HEADERS = \ diff --git a/helper/include/odp/helper/pktio.h b/helper/include/odp/helper/pktio.h new file mode 100644 index 0000000..18e6f94 --- /dev/null +++ b/helper/include/odp/helper/pktio.h @@ -0,0 +1,65 @@ +/* Copyright (c) 2015, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +/** + * @file + * + * ODP pktio helper API + * + */ + +#ifndef ODPH_PKTIO_H_ +#define ODPH_PKTIO_H_ + +#ifdef __cplusplus +extern "C" { +#endif + +#include <odp/time.h> +#include <odp/packet_io.h> + +/** time to wait after odp_pktio_start() to bring link up. + * It's is hardware dependent value, here we will have some common + * value. + */ +#define _ODPH_PKTIO_LINK_UP_TIME (50 * ODP_TIME_MSEC_IN_NS) + +/** Number of odp_pktio_link_status() calls in time interval. */ +#define _ODPH_PKTIO_LINK_TRIES 5 + +/** + * Wait for pktio link up. + * + * After odp_pktio_link_start(pktio) some platforms need small delay + * to bring up corresponding pktio link. + * + * @param pktio Packet IO handle. + * + * @retval 1 link is up + * @retval 0 link is down + * @retval <0 on failure +*/ +static inline int odph_pktio_wait_linkup(odp_pktio_t pktio) +{ + uint64_t wait_ns = _ODPH_PKTIO_LINK_UP_TIME / _ODPH_PKTIO_LINK_TRIES; + int i; + int ret = -1; + + for (i = 0; i < _ODPH_PKTIO_LINK_TRIES; i++) { + ret = odp_pktio_link_status(pktio); + if (ret < 0 || ret == 1) + return ret; + /* link is down, call status again after delay */ + odp_time_wait_ns(wait_ns); + } + return ret; +} + +#ifdef __cplusplus +} +#endif + +#endif
Define pktio helper to wait link up after odp_pktio_start(). Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- helper/Makefile.am | 1 + helper/include/odp/helper/pktio.h | 65 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 helper/include/odp/helper/pktio.h