From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 6D30629CA for ; Thu, 29 Jun 2017 13:13:19 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP; 29 Jun 2017 04:13:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,280,1496127600"; d="scan'208";a="986636479" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by orsmga003.jf.intel.com with ESMTP; 29 Jun 2017 04:13:16 -0700 Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 29 Jun 2017 12:13:15 +0100 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.211]) by irsmsx155.ger.corp.intel.com ([169.254.14.182]) with mapi id 14.03.0319.002; Thu, 29 Jun 2017 12:13:15 +0100 From: "Van Haaren, Harry" To: 'Jerin Jacob' CC: "dev@dpdk.org" , "thomas@monjalon.net" , "Wiles, Keith" , "Richardson, Bruce" Thread-Topic: [PATCH 1/6] service cores: header and implementation Thread-Index: AQHS7AABB5W8rkbWMUGt13NIPogQsaI2/nwAgAAucyA= Date: Thu, 29 Jun 2017 11:13:15 +0000 Message-ID: References: <1498208779-166205-1-git-send-email-harry.van.haaren@intel.com> <20170626115910.GA5612@jerin> In-Reply-To: <20170626115910.GA5612@jerin> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGYyODk4ZDQtYjMyNC00YWVmLTk5NTUtMzFjMWI0ZTllNzdjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlhjODdKbUs4S1RcL08wTXJZMkR4d3VZMWZldzJEZXVFUnJ5d3NsVW5rbkZZPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/6] service cores: header and implementation 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: Thu, 29 Jun 2017 11:13:20 -0000 > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > > This is the third iteration of the service core concept, > > now with an implementation. > > > > Signed-off-by: Harry van Haaren >=20 > Nice work. Detailed review comments below Thanks for the (prompt!) feedback. Mostly agree, comments inline, lo= ts of noisy code out :) I have a follow up question on service-core usage, and how we can work e.g = Eventdev PMDs into Service cores. I'll kick off a new thread on the mailing= list to discuss it. Patchset v2 on the way soon. > > [keepalive] (@ref rte_keepalive.h), > > + [Service Cores] (@ref rte_service.h), >=20 > 1) IMO, To keep the consistency we can rename to "[service cores]" Done. > 2) I thought, we decided to expose rte_service_register() and > rte_service_unregister() as well, Considering the case where even applica= tion > as register for service functions if required. If it is true then I > think, registration functions can moved of private header file so that > it will visible in doxygen. To avoid bleeding implementation out of the API, this was not done. Service= s register API is not currently publicly visible - this keeps the API abstr= action very powerful. If we decide to make the register-struct public, we l= ost (almost) all of the API encapsulation, as the struct itself has to be p= ublic=20 Applications can #include if they insist - which wo= uld provide the functionality as desired, but then the application is aware= that it is using DPDK private data structures. I suggest we leave the service register API as private for this release. We= can always move it to public if required - once we are more comfortable wi= th the API and it is more widely implemented. This will help keep API/ABI s= tability - we don't have the "luxury" of EXPERIMENTAL tag in EAL :D > 3) Should we change core function name as lcore like > rte_service_lcore_add(), rte_service_lcore_del() etc as we are operating > on lcore here. Yep, done. Added "l" to all core related functions for consistency, so lcor= e is now used everywhere. > > struct rte_config { > > uint32_t master_lcore; /**< Id of the master lcore */ > > uint32_t lcore_count; /**< Number of available logical cores. = */ > > + uint32_t score_count; /**< Number of available service cores. = */ >=20 > Should we call it as service core or service lcore? Done > > +/** Return the number of services registered. > > + * > > + * The number of services registered can be passed to *rte_service_get= _by_id*, > > + * enabling the application to retireve the specificaion of each servi= ce. >=20 > s/retireve the specificaion/retrieve the specification >=20 > > + * > > + * @return The number of services registered. > > + */ > > +uint32_t rte_service_get_count(void); > > + > > +/** Return the specificaion of each service. >=20 > s/specificaion/specification Fixed > > +/* Check if a service has a specific capability. > Missing the doxygen marker(ie. change to /** Check) Fixed > > +/* Start a service core. > Missing the doxygen marker(ie. change to /** Start) Fixed > > +/** Retreve the number of service cores currently avaialble. > typo: ^^^^^^^^ ^^^^^^^^^^ > Retrieve the number of service cores currently available. Oh my do I have talent for mis-spelling :D Fixed > > + * @param array An array of at least N items. >=20 > @param [out] array An array of at least n items >=20 > > + * @param The size of *array*. >=20 > @param n The size of *array*. Done! > > + /** NUMA socket ID that this service is affinitized to */ > > + int8_t socket_id; >=20 > All other places socket_id is of type "int". Done > > +/** Private function to allow EAL to initialied default mappings. >=20 > typo: ^^^^^^^^^^^ Fixed > > +#define RTE_SERVICE_FLAG_REGISTERED_SHIFT 0 >=20 > Internal macro, Can be shorten to reduce the length(SERVICE_F_REGISTERED?= ) >=20 > > + > > +#define RTE_SERVICE_RUNSTATE_STOPPED 0 > > +#define RTE_SERVICE_RUNSTATE_RUNNING 1 >=20 > Internal macro, Can be shorten to reduce the length(SERVICE_STATE_RUNNING= ?) These are used for services and for lcore state, so just used RUNSTATE_RUNN= ING and RUNSTATE_STOPPED. > > +struct rte_service_spec_impl { > > + /* public part of the struct */ > > + struct rte_service_spec spec; >=20 > Nice approach. > Since it been used in fastpath. better to align to cache line Done :) > > +struct core_state { > aligned to cache line? Done > > +static uint32_t rte_service_count; > > +static struct rte_service_spec_impl rte_services[RTE_SERVICE_NUM_MAX]; > > +static struct core_state cores_state[RTE_MAX_LCORE]; >=20 > Since these variable are used in fastpath, better to allocate form > huge page area. It will avoid lot of global variables in code as well. > Like other module, you can add a private function for service init and it= can be > called from eal_init() Yep good point, done. > > +static int >=20 > static inline int > > +service_valid(uint32_t id) { Done > > +int32_t >=20 > bool could be enough here >=20 > > +rte_service_probe_capability(const struct rte_service_spec *service, > > + uint32_t capability) Currently the entire API is only, leaving as is. > > +int32_t > > +rte_service_register(const struct rte_service_spec *spec) > > +{ > > + uint32_t i; > > + int32_t free_slot =3D -1; > > + > > + if (spec->callback =3D=3D NULL || strlen(spec->name) =3D=3D 0) > > + return -EINVAL; > > + > > + for (i =3D 0; i < RTE_SERVICE_NUM_MAX; i++) { > > + if (!service_valid(i)) { > > + free_slot =3D i; > > + break; > > + } > > + } > > + > > + if (free_slot < 0) >=20 > if ((free_slot < 0) || (i =3D=3D RTE_SERVICE_NUM_MAX)) Ah - a bug! Nice catch, fixed. > > + s->internal_flags |=3D (1 << RTE_SERVICE_FLAG_REGISTERED_SHIFT); > > + > > + rte_smp_wmb(); > > + rte_service_count++; >=20 > IMO, You can move above rte_smp_wmb() here. Perhaps I'm not understanding correctly, but don't we need the writes to th= e service spec to be completed before allowing other cores to see the extra= service count? In short, I think the wmb() is in the right place? > > + memset(&rte_services[service_id], 0, > > + sizeof(struct rte_service_spec_impl)); > > + > > + rte_smp_wmb(); > > + rte_service_count--; >=20 > IMO, You can move above rte_smp_wmb() here. I think this part needs refactoring actually; count--; wmb(); memset(); Stop cores from seeing service, wmb() to ensure writes complete, then clear= internal config? > > +int32_t > > +rte_service_start(struct rte_service_spec *service) > > +{ > > + struct rte_service_spec_impl *s =3D > > + (struct rte_service_spec_impl *)service; > > + s->runstate =3D RTE_SERVICE_RUNSTATE_RUNNING; >=20 > Is this function can called from worker thread? if so add rte_smp_wmb() Done > > + return 0; > > +} > > + > > +int32_t > > +rte_service_stop(struct rte_service_spec *service) > > +{ > > + struct rte_service_spec_impl *s =3D > > + (struct rte_service_spec_impl *)service; > > + s->runstate =3D RTE_SERVICE_RUNSTATE_STOPPED; >=20 > Is this function can called from worker thread? if so add rte_smp_wmb() Done > > +static int32_t > > +rte_service_runner_func(void *arg) > > +{ > > + RTE_SET_USED(arg); > > + uint32_t i; > > + const int lcore =3D rte_lcore_id(); > > + struct core_state *cs =3D &cores_state[lcore]; > > + > > + while (cores_state[lcore].runstate =3D=3D RTE_SERVICE_RUNSTATE_RUNNIN= G) { > > + for (i =3D 0; i < rte_service_count; i++) { > > + struct rte_service_spec_impl *s =3D &rte_services[i]; > > + uint64_t service_mask =3D cs->service_mask; >=20 > No need to read in loop, Move it above while loop and add const. > const uint64_t service_mask =3D cs->service_mask; Yep done, I wonder would a compiler be smart enough.. :) > > + uint32_t *lock =3D (uint32_t *)&s->execute_lock; > > + if (rte_atomic32_cmpset(lock, 0, 1)) { >=20 > rte_atomic32 is costly. How about checking RTE_SERVICE_CAP_MT_SAFE > first. Yep this was on my scope for optimizing down the line. =20 > > + void *userdata =3D s->spec.callback_userdata; > > + uint64_t start =3D rte_rdtsc(); > > + s->spec.callback(userdata); > > + uint64_t end =3D rte_rdtsc(); > > + > > + uint64_t spent =3D end - start; > > + s->cycles_spent +=3D spent; > > + s->calls++; > > + cs->calls_per_service[i]++; >=20 > How about enabling the statistics based on some runtime configuration? Good idea - added an API to enable/disable statistics collection. > > + rte_atomic32_clear(&s->execute_lock); > > + } > > + } > > + rte_mb(); >=20 > Do we need full barrier here. Is rte_smp_rmb() inside the loop is > enough? Actually I'm not quite sure why there's a barrier at all.. removed. > > + uint32_t i; > > + uint32_t idx =3D 0; > > + for (i =3D 0; i < RTE_MAX_LCORE; i++) { >=20 > Are we good if "count" being the upper limit instead of RTE_MAX_LCORE? Nope, the cores could be anywhere from 0 to RTE_MAX_LCORE - we gotta scan t= hem all. > > + struct core_state *cs =3D &cores_state[i]; > > + if (cs->is_service_core) { > > + array[idx] =3D i; > > + idx++; > > + } > > + } > > + > > + ret =3D rte_service_enable_on_core(s, j); > > + if (ret) > > + rte_panic("Enabling service core %d on service %s failed\n", > > + j, s->name); >=20 > avoid panic in library Done > > + ret =3D rte_service_start(s); > > + if (ret) > > + rte_panic("failed to start service %s\n", s->name); >=20 > avoid panic in library Done > > +static int32_t > > +service_update(struct rte_service_spec *service, uint32_t lcore, > > + uint32_t *set, uint32_t *enabled) > > +{ >=20 > If the parent functions can be called from worker thread then add > rte_smp_wmb() here. Yes they could, done. > > + lcore_config[lcore].core_role =3D ROLE_SERVICE; > > + > > + /* TODO: take from EAL by setting ROLE_SERVICE? */ >=20 > I think, we need to fix TODO in v2 Good point :) done > > + lcore_config[lcore].core_role =3D ROLE_RTE; > > + cores_state[lcore].is_service_core =3D 0; > > + /* TODO: return to EAL by setting ROLE_RTE? */ >=20 > I think, we need to fix TODO in v2 Done > > + /* set core to run state first, and then launch otherwise it will > > + * return immidiatly as runstate keeps it in the service poll loop >=20 > s/immidiatly/immediately Fixed > > + int ret =3D rte_eal_remote_launch(rte_service_runner_func, 0, lcore); > > + /* returns -EBUSY if the core is already launched, 0 on success */ > > + return ret; >=20 > return rte_eal_remote_launch(rte_service_runner_func, 0, lcore); I got bitten by this twice - documenting the return values, and making it o= bvious where they come from is worth the variable IMO. Any compiler will op= timize away anyways :) > > + /* avoid divide by zeros */ >=20 > s/zeros/zero Fixed! Thanks for the lengthy review - the code has improved a lot - appreciated.