From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier MATZ Subject: Re: [PATCH v4 17/17] timer: add support to non-EAL thread Date: Tue, 10 Feb 2015 18:45:16 +0100 Message-ID: <54DA43AC.2030108@6wind.com> References: <1422491072-5114-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-1-git-send-email-cunming.liang@intel.com> <1422842559-13617-18-git-send-email-cunming.liang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable To: Cunming Liang , dev-VfR2kkLFssw@public.gmane.org Return-path: In-Reply-To: <1422842559-13617-18-git-send-email-cunming.liang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" Hi, On 02/02/2015 03:02 AM, Cunming Liang wrote: > Allow to setup timers only for EAL (lcore) threads (__lcore_id < MAX_LC= ORE_ID). > E.g. =E2=80=93 dynamically created thread will be able to reset/stop ti= mer for lcore thread, > but it will be not allowed to setup timer for itself or another non-lco= re thread. > rte_timer_manage() for non-lcore thread would simply do nothing and ret= urn straightway. > > Signed-off-by: Cunming Liang > --- > lib/librte_timer/rte_timer.c | 40 +++++++++++++++++++++++++++++++----= ----- > lib/librte_timer/rte_timer.h | 2 +- > 2 files changed, 32 insertions(+), 10 deletions(-) > > diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.= c > index 269a992..601c159 100644 > --- a/lib/librte_timer/rte_timer.c > +++ b/lib/librte_timer/rte_timer.c > @@ -79,9 +79,10 @@ static struct priv_timer priv_timer[RTE_MAX_LCORE]; > > /* when debug is enabled, store some statistics */ > #ifdef RTE_LIBRTE_TIMER_DEBUG > -#define __TIMER_STAT_ADD(name, n) do { \ > - unsigned __lcore_id =3D rte_lcore_id(); \ > - priv_timer[__lcore_id].stats.name +=3D (n); \ > +#define __TIMER_STAT_ADD(name, n) do { \ > + unsigned __lcore_id =3D rte_lcore_id(); \ > + if (__lcore_id < RTE_MAX_LCORE) \ > + priv_timer[__lcore_id].stats.name +=3D (n); \ > } while(0) > #else > #define __TIMER_STAT_ADD(name, n) do {} while(0) > @@ -127,15 +128,26 @@ timer_set_config_state(struct rte_timer *tim, > unsigned lcore_id; > > lcore_id =3D rte_lcore_id(); > + if (lcore_id >=3D RTE_MAX_LCORE) > + lcore_id =3D LCORE_ID_ANY; Is this still valid? In my understanding, rte_lcore_id() was returning the core id or LCORE_ID_ANY if it's a non-EAL thread. > > /* wait that the timer is in correct status before update, > * and mark it as being configured */ > while (success =3D=3D 0) { > prev_status.u32 =3D tim->status.u32; > > + /* > + * prevent race condition of non-EAL threads > + * to update the timer. When 'owner =3D=3D LCORE_ID_ANY', > + * it means updated by a non-EAL thread. > + */ > + if (lcore_id =3D=3D (unsigned)LCORE_ID_ANY && > + (uint16_t)lcore_id =3D=3D prev_status.owner) > + return -1; > + Are you sure this is required? I think prev_status.owner can be LCORE_ID_ANY only in config state, as a timer cannot be scheduled on a non-EAL thread. And there is already a test that returns -1 if state is CONFIG. > /* timer is running on another core, exit */ > if (prev_status.state =3D=3D RTE_TIMER_RUNNING && > - (unsigned)prev_status.owner !=3D lcore_id) > + prev_status.owner !=3D (uint16_t)lcore_id) > return -1; > > /* timer is being configured on another core */ > @@ -366,9 +378,13 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t = expire, > > /* round robin for tim_lcore */ > if (tim_lcore =3D=3D (unsigned)LCORE_ID_ANY) { > - tim_lcore =3D rte_get_next_lcore(priv_timer[lcore_id].prev_lcore, > - 0, 1); > - priv_timer[lcore_id].prev_lcore =3D tim_lcore; > + if (lcore_id < RTE_MAX_LCORE) { if (lcore_id !=3D LCORE_ID_ANY) ? > + tim_lcore =3D rte_get_next_lcore( > + priv_timer[lcore_id].prev_lcore, > + 0, 1); > + priv_timer[lcore_id].prev_lcore =3D tim_lcore; > + } else > + tim_lcore =3D rte_get_next_lcore(LCORE_ID_ANY, 0, 1); I think the following line: tim_lcore =3D rte_get_next_lcore(LCORE_ID_ANY, 0, 1); Will return the first enabled core. Maybe using rte_get_master_lcore() is clearer? > } > > /* wait that the timer is in correct status before update, > @@ -378,7 +394,8 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t e= xpire, > return -1; > > __TIMER_STAT_ADD(reset, 1); > - if (prev_status.state =3D=3D RTE_TIMER_RUNNING) { > + if (prev_status.state =3D=3D RTE_TIMER_RUNNING && > + lcore_id < RTE_MAX_LCORE) { if (lcore_id !=3D LCORE_ID_ANY) ? > priv_timer[lcore_id].updated =3D 1; > } > > @@ -455,7 +472,8 @@ rte_timer_stop(struct rte_timer *tim) > return -1; > > __TIMER_STAT_ADD(stop, 1); > - if (prev_status.state =3D=3D RTE_TIMER_RUNNING) { > + if (prev_status.state =3D=3D RTE_TIMER_RUNNING && > + lcore_id < RTE_MAX_LCORE) { if (lcore_id !=3D LCORE_ID_ANY) ? > priv_timer[lcore_id].updated =3D 1; > } > > @@ -499,6 +517,10 @@ void rte_timer_manage(void) > uint64_t cur_time; > int i, ret; > > + /* timer manager only runs on EAL thread */ > + if (lcore_id >=3D RTE_MAX_LCORE) > + return; > + Maybe an assert is more visible here. Else, if someone calls rte_timer_manage() from a non-EAL core, it will just exit silently. Maybe adding a comment in rte_timer.h saying that this function must be called from an EAL core would also help. Regards, Olivier