From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f181.google.com (mail-wr0-f181.google.com [209.85.128.181]) by dpdk.org (Postfix) with ESMTP id 6DE559135 for ; Mon, 31 Jul 2017 18:18:04 +0200 (CEST) Received: by mail-wr0-f181.google.com with SMTP id 33so124800104wrz.4 for ; Mon, 31 Jul 2017 09:18:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=7LAlupLmZyHDQqtWF1B1Gc9PPLHdOYkMdzc33q+4T/4=; b=R9LZzRqPVuRdKxGw+nIvd7NeR/PZr3RVmlygRWJbXL9Ic852VMgb+oe37l9Qw4CCQR 51ikVzW2sE1BycfJzEwZIMHhWEPPyorCEbFextYZcdf2UQ/7YzK86xnDTTRw/r6C4aqN Nz0HqFe2uICIVVhJmA8ijep/SQ0c4tDW2W+ll2ioYq1G3ggGlO+aC6FRnLBicXitBDka ddt3pU3wC2v/JdHpxbYHXyo31MhvcB6SDSVY+6BjVLd0RikhQzAdc4FmwYFLF69jZbzU FCmC0v2ZWeSHfulxJIm///SbndCQvB+J5UHKW3IRjGnwUSF44qCmTJKf1TUAl8pILDce seHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=7LAlupLmZyHDQqtWF1B1Gc9PPLHdOYkMdzc33q+4T/4=; b=HTZeH4eKG9seBZ4tS6ooqPI0CnUQp1hOCG418ABHbalJUXUvOQpGB/nLANDpd9Z6lj NwL+O2gaEK4xGdhDvoPJKNBbcPy7Z/q/p1FC421CtTZTN2clPeZDjDktjtWnpL0PNvMk aeCPKiN54pcKLrIHHjSHaZt6E6Sjl8noDM+EZnNHOQC7TnGcTvuN4O5NTzVvOQXaXDxT uy7FxjdsvTFk0maRxny9CHuxC7S+BCryqB7pkDq9MrHYQ5ztHNk6bnE9R2OClrdoAXKj YXkNb13gpqbCmRg8ZhMP/MtEtBBWCb4ofxnJAzid+UQIZBZ1zKR0BXwDklU04hrNljPK JQ9g== X-Gm-Message-State: AIVw112xWW3xriJHpK3SNip9NT45ib8+cFgTzcjngHRtPRt8Jskf/1zz xJstDzjOIxe1F+ndQcw= X-Received: by 10.223.139.23 with SMTP id n23mr5060049wra.249.1501517883998; Mon, 31 Jul 2017 09:18:03 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id s133sm1040771wmb.4.2017.07.31.09.18.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 31 Jul 2017 09:18:03 -0700 (PDT) Date: Mon, 31 Jul 2017 18:17:54 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Harry van Haaren Cc: dev@dpdk.org Message-ID: <20170731161754.GN11154@bidouze.vm.6wind.com> References: <1501516707-87024-1-git-send-email-harry.van.haaren@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1501516707-87024-1-git-send-email-harry.van.haaren@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] service: fix shifts to operate on 64 bit integers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 31 Jul 2017 16:18:04 -0000 Hi Harry, On Mon, Jul 31, 2017 at 04:58:27PM +0100, Harry van Haaren wrote: > This commit fixes shifts to an integer (1 << shift) which > is assumed to be a 32-bit integer. In this case, the shift is > variable and expected to be valid for 64-bit integers. Given that > the expectation to work with 64 bits exists, we must ensure that > the (1 << shift) one in that formula is actually a uin64_t. > > Simply defining a const uint64_t and using it ensures the compiler > is aware of the intention. The issue would only manifest if there > were greater than 31 services registered. > > Fixes: 21698354c832 ("service: introduce service cores concept") > > Signed-off-by: Harry van Haaren > --- > lib/librte_eal/common/rte_service.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c > index e82b9ad..8c1cffa 100644 > --- a/lib/librte_eal/common/rte_service.c > +++ b/lib/librte_eal/common/rte_service.c > @@ -285,8 +285,9 @@ rte_service_unregister(struct rte_service_spec *spec) > > s->internal_flags &= ~(SERVICE_F_REGISTERED); > > + const uint64_t one = 1; > for (i = 0; i < RTE_MAX_LCORE; i++) > - lcore_states[i].service_mask &= ~(1 << service_id); > + lcore_states[i].service_mask &= ~(one << service_id); Why not use UINT64_C(1)? > > memset(&rte_services[service_id], 0, > sizeof(struct rte_service_spec_impl)); > @@ -319,6 +320,7 @@ rte_service_runner_func(void *arg) > { > RTE_SET_USED(arg); > uint32_t i; > + const uint64_t one = 1; > const int lcore = rte_lcore_id(); > struct core_state *cs = &lcore_states[lcore]; > > @@ -327,7 +329,7 @@ rte_service_runner_func(void *arg) > for (i = 0; i < rte_service_count; i++) { > struct rte_service_spec_impl *s = &rte_services[i]; > if (s->runstate != RUNSTATE_RUNNING || > - !(service_mask & (1 << i))) > + !(service_mask & (one << i))) > continue; > > /* check do we need cmpset, if MT safe or <= 1 core > @@ -448,6 +450,7 @@ service_update(struct rte_service_spec *service, uint32_t lcore, > { > uint32_t i; > int32_t sid = -1; > + const uint64_t one = 1; > > for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) { > if ((struct rte_service_spec *)&rte_services[i] == service && > @@ -465,16 +468,16 @@ service_update(struct rte_service_spec *service, uint32_t lcore, > > if (set) { > if (*set) { > - lcore_states[lcore].service_mask |= (1 << sid); > + lcore_states[lcore].service_mask |= (one << sid); > rte_services[sid].num_mapped_cores++; > } else { > - lcore_states[lcore].service_mask &= ~(1 << sid); > + lcore_states[lcore].service_mask &= ~(one << sid); > rte_services[sid].num_mapped_cores--; > } > } > > if (enabled) > - *enabled = (lcore_states[lcore].service_mask & (1 << sid)); > + *enabled = (lcore_states[lcore].service_mask & (one << sid)); > > rte_smp_wmb(); > > @@ -599,6 +602,7 @@ rte_service_lcore_start(uint32_t lcore) > int32_t > rte_service_lcore_stop(uint32_t lcore) > { > + const uint64_t one = 1; > if (lcore >= RTE_MAX_LCORE) > return -EINVAL; > > @@ -607,7 +611,7 @@ rte_service_lcore_stop(uint32_t lcore) > > uint32_t i; > for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) { > - int32_t enabled = lcore_states[i].service_mask & (1 << i); > + int32_t enabled = lcore_states[i].service_mask & (one << i); > int32_t service_running = rte_services[i].runstate != > RUNSTATE_STOPPED; > int32_t only_core = rte_services[i].num_mapped_cores == 1; > -- > 2.7.4 > -- Gaëtan Rivet 6WIND