From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f174.google.com (mail-wi0-f174.google.com [209.85.212.174]) by dpdk.org (Postfix) with ESMTP id ED58C5936 for ; Tue, 10 Feb 2015 18:45:21 +0100 (CET) Received: by mail-wi0-f174.google.com with SMTP id em10so11040786wid.1 for ; Tue, 10 Feb 2015 09:45:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=jTfFpEWTXStoG74/sOfTYOgDuwD1fZuK8e/KpWZ6s5Y=; b=DZk+tf+VDON53bGXj0sXV6yL9wzM+OKIuHZPWkk73Zw0LxAfbm42mqVVSfhj1jF69R n/bJAEcuiBFT40uvc0a4ImFuyAtZeqwnTIK3tQGI00JMgIoLO6Kr081JHyv6DlgR+Ng/ S6ZFg84IyPVujcplSkoPRB8DbfV1HGo8sAuBX1Q/GzwG+vvFoM8o/A+EY6ZNFiI18GMP arOYErwg3YBRa5cfjMFiupncXCrWWRzTssWen1dmxUmOO25hnVzyOx5ikpRkwjwIiQoz ErpdRL8KX6x7XV4EE/iLBIOnXLjJYgR3dXCc7q6hWKn2l3I8eL+L7Z2aZBDe5KNLhzCm THUA== X-Gm-Message-State: ALoCoQnw2mke3SDjMomas6fuK9/Rex4glg/kN7Cz2LwACH2VUV8sJ4MSCr9HOTlzNsmZ9FjkBQGW X-Received: by 10.180.211.83 with SMTP id na19mr39230516wic.29.1423590320339; Tue, 10 Feb 2015 09:45:20 -0800 (PST) Received: from [10.16.0.195] (6wind.net2.nerim.net. [213.41.180.237]) by mx.google.com with ESMTPSA id e4sm21670580wjw.48.2015.02.10.09.45.16 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 10 Feb 2015 09:45:19 -0800 (PST) Message-ID: <54DA43AC.2030108@6wind.com> Date: Tue, 10 Feb 2015 18:45:16 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.2.0 MIME-Version: 1.0 To: Cunming Liang , dev@dpdk.org 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> In-Reply-To: <1422842559-13617-18-git-send-email-cunming.liang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4 17/17] timer: add support to non-EAL thread X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Feb 2015 17:45:22 -0000 Hi, On 02/02/2015 03:02 AM, Cunming Liang wrote: > Allow to setup timers only for EAL (lcore) threads (__lcore_id < MAX_LCORE_ID). > E.g. – dynamically created thread will be able to reset/stop timer for lcore thread, > but it will be not allowed to setup timer for itself or another non-lcore thread. > rte_timer_manage() for non-lcore thread would simply do nothing and return 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 = rte_lcore_id(); \ > - priv_timer[__lcore_id].stats.name += (n); \ > +#define __TIMER_STAT_ADD(name, n) do { \ > + unsigned __lcore_id = rte_lcore_id(); \ > + if (__lcore_id < RTE_MAX_LCORE) \ > + priv_timer[__lcore_id].stats.name += (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 = rte_lcore_id(); > + if (lcore_id >= RTE_MAX_LCORE) > + lcore_id = 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 == 0) { > prev_status.u32 = tim->status.u32; > > + /* > + * prevent race condition of non-EAL threads > + * to update the timer. When 'owner == LCORE_ID_ANY', > + * it means updated by a non-EAL thread. > + */ > + if (lcore_id == (unsigned)LCORE_ID_ANY && > + (uint16_t)lcore_id == 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 == RTE_TIMER_RUNNING && > - (unsigned)prev_status.owner != lcore_id) > + prev_status.owner != (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 == (unsigned)LCORE_ID_ANY) { > - tim_lcore = rte_get_next_lcore(priv_timer[lcore_id].prev_lcore, > - 0, 1); > - priv_timer[lcore_id].prev_lcore = tim_lcore; > + if (lcore_id < RTE_MAX_LCORE) { if (lcore_id != LCORE_ID_ANY) ? > + tim_lcore = rte_get_next_lcore( > + priv_timer[lcore_id].prev_lcore, > + 0, 1); > + priv_timer[lcore_id].prev_lcore = tim_lcore; > + } else > + tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0, 1); I think the following line: tim_lcore = 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 expire, > return -1; > > __TIMER_STAT_ADD(reset, 1); > - if (prev_status.state == RTE_TIMER_RUNNING) { > + if (prev_status.state == RTE_TIMER_RUNNING && > + lcore_id < RTE_MAX_LCORE) { if (lcore_id != LCORE_ID_ANY) ? > priv_timer[lcore_id].updated = 1; > } > > @@ -455,7 +472,8 @@ rte_timer_stop(struct rte_timer *tim) > return -1; > > __TIMER_STAT_ADD(stop, 1); > - if (prev_status.state == RTE_TIMER_RUNNING) { > + if (prev_status.state == RTE_TIMER_RUNNING && > + lcore_id < RTE_MAX_LCORE) { if (lcore_id != LCORE_ID_ANY) ? > priv_timer[lcore_id].updated = 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 >= 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