From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
To: 'Jerin Jacob' <jerin.jacob@caviumnetworks.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"thomas@monjalon.net" <thomas@monjalon.net>,
"Wiles, Keith" <keith.wiles@intel.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH 1/6] service cores: header and implementation
Date: Thu, 29 Jun 2017 11:13:15 +0000 [thread overview]
Message-ID: <E923DB57A917B54B9182A2E928D00FA640C33C53@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <20170626115910.GA5612@jerin>
> From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com]
<snip>
> > This is the third iteration of the service core concept,
> > now with an implementation.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
>
> Nice work. Detailed review comments below
Thanks for the (prompt!) feedback. Mostly agree, comments inline, <snip> lots 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),
>
> 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 application
> 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. Services register API is not currently publicly visible - this keeps the API abstraction very powerful. If we decide to make the register-struct public, we lost (almost) all of the API encapsulation, as the struct itself has to be public
Applications can #include <rte_service_private.h> if they insist - which would 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 with the API and it is more widely implemented. This will help keep API/ABI stability - 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 lcore 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. */
>
> 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 service.
>
> s/retireve the specificaion/retrieve the specification
>
> > + *
> > + * @return The number of services registered.
> > + */
> > +uint32_t rte_service_get_count(void);
> > +
> > +/** Return the specificaion of each service.
>
> 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.
>
> @param [out] array An array of at least n items
>
> > + * @param The size of *array*.
>
> @param n The size of *array*.
Done!
> > + /** NUMA socket ID that this service is affinitized to */
> > + int8_t socket_id;
>
> All other places socket_id is of type "int".
Done
> > +/** Private function to allow EAL to initialied default mappings.
>
> typo: ^^^^^^^^^^^
Fixed
> > +#define RTE_SERVICE_FLAG_REGISTERED_SHIFT 0
>
> Internal macro, Can be shorten to reduce the length(SERVICE_F_REGISTERED?)
>
> > +
> > +#define RTE_SERVICE_RUNSTATE_STOPPED 0
> > +#define RTE_SERVICE_RUNSTATE_RUNNING 1
>
> 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_RUNNING and RUNSTATE_STOPPED.
> > +struct rte_service_spec_impl {
> > + /* public part of the struct */
> > + struct rte_service_spec spec;
>
> Nice approach.
<snip>
> Since it been used in fastpath. better to align to cache line
Done :)
> > +struct core_state {
<snip>
> 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];
>
> 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
>
> static inline int
> > +service_valid(uint32_t id) {
Done
> > +int32_t
>
> bool could be enough here
>
> > +rte_service_probe_capability(const struct rte_service_spec *service,
> > + uint32_t capability)
Currently the entire API is <stdint.h> only, leaving as is.
> > +int32_t
> > +rte_service_register(const struct rte_service_spec *spec)
> > +{
> > + uint32_t i;
> > + int32_t free_slot = -1;
> > +
> > + if (spec->callback == NULL || strlen(spec->name) == 0)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) {
> > + if (!service_valid(i)) {
> > + free_slot = i;
> > + break;
> > + }
> > + }
> > +
> > + if (free_slot < 0)
>
> if ((free_slot < 0) || (i == RTE_SERVICE_NUM_MAX))
Ah - a bug! Nice catch, fixed.
> > + s->internal_flags |= (1 << RTE_SERVICE_FLAG_REGISTERED_SHIFT);
> > +
> > + rte_smp_wmb();
> > + rte_service_count++;
>
> IMO, You can move above rte_smp_wmb() here.
Perhaps I'm not understanding correctly, but don't we need the writes to the 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--;
>
> 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 =
> > + (struct rte_service_spec_impl *)service;
> > + s->runstate = RTE_SERVICE_RUNSTATE_RUNNING;
>
> 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 =
> > + (struct rte_service_spec_impl *)service;
> > + s->runstate = RTE_SERVICE_RUNSTATE_STOPPED;
>
> 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 = rte_lcore_id();
> > + struct core_state *cs = &cores_state[lcore];
> > +
> > + while (cores_state[lcore].runstate == RTE_SERVICE_RUNSTATE_RUNNING) {
> > + for (i = 0; i < rte_service_count; i++) {
> > + struct rte_service_spec_impl *s = &rte_services[i];
> > + uint64_t service_mask = cs->service_mask;
>
> No need to read in loop, Move it above while loop and add const.
> const uint64_t service_mask = cs->service_mask;
Yep done, I wonder would a compiler be smart enough.. :)
> > + uint32_t *lock = (uint32_t *)&s->execute_lock;
> > + if (rte_atomic32_cmpset(lock, 0, 1)) {
>
> rte_atomic32 is costly. How about checking RTE_SERVICE_CAP_MT_SAFE
> first.
Yep this was on my scope for optimizing down the line.
> > + void *userdata = s->spec.callback_userdata;
> > + uint64_t start = rte_rdtsc();
> > + s->spec.callback(userdata);
> > + uint64_t end = rte_rdtsc();
> > +
> > + uint64_t spent = end - start;
> > + s->cycles_spent += spent;
> > + s->calls++;
> > + cs->calls_per_service[i]++;
>
> 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();
>
> 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 = 0;
> > + for (i = 0; i < RTE_MAX_LCORE; i++) {
>
> 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 them all.
> > + struct core_state *cs = &cores_state[i];
> > + if (cs->is_service_core) {
> > + array[idx] = i;
> > + idx++;
> > + }
> > + }
> > +
<snip>
> > + ret = rte_service_enable_on_core(s, j);
> > + if (ret)
> > + rte_panic("Enabling service core %d on service %s failed\n",
> > + j, s->name);
>
> avoid panic in library
Done
> > + ret = rte_service_start(s);
> > + if (ret)
> > + rte_panic("failed to start service %s\n", s->name);
>
> avoid panic in library
Done
> > +static int32_t
> > +service_update(struct rte_service_spec *service, uint32_t lcore,
> > + uint32_t *set, uint32_t *enabled)
> > +{
<snip>
>
> 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 = ROLE_SERVICE;
> > +
> > + /* TODO: take from EAL by setting ROLE_SERVICE? */
>
> I think, we need to fix TODO in v2
Good point :) done
> > + lcore_config[lcore].core_role = ROLE_RTE;
> > + cores_state[lcore].is_service_core = 0;
> > + /* TODO: return to EAL by setting ROLE_RTE? */
>
> 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
>
> s/immidiatly/immediately
Fixed
> > + int ret = rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
> > + /* returns -EBUSY if the core is already launched, 0 on success */
> > + return ret;
>
> return rte_eal_remote_launch(rte_service_runner_func, 0, lcore);
I got bitten by this twice - documenting the return values, and making it obvious where they come from is worth the variable IMO. Any compiler will optimize away anyways :)
> > + /* avoid divide by zeros */
>
> s/zeros/zero
Fixed!
Thanks for the lengthy review - the code has improved a lot - appreciated.
next prev parent reply other threads:[~2017-06-29 11:13 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-23 9:06 Harry van Haaren
2017-06-23 9:06 ` [dpdk-dev] [PATCH 2/6] service cores: coremask parsing Harry van Haaren
2017-06-26 12:49 ` Jerin Jacob
2017-06-29 11:13 ` Van Haaren, Harry
2017-06-23 9:06 ` [dpdk-dev] [PATCH 3/6] service cores: EAL init changes Harry van Haaren
2017-06-26 12:55 ` Jerin Jacob
2017-06-29 11:13 ` Van Haaren, Harry
2017-06-23 9:06 ` [dpdk-dev] [PATCH 4/6] service cores: mark cores in lcore config as RTE Harry van Haaren
2017-06-23 9:06 ` [dpdk-dev] [PATCH 5/6] service core: add unit tests Harry van Haaren
2017-06-26 13:06 ` Jerin Jacob
2017-06-29 11:14 ` Van Haaren, Harry
2017-06-23 9:06 ` [dpdk-dev] [PATCH 6/6] service cores: enable event/sw with service Harry van Haaren
2017-06-26 13:46 ` Jerin Jacob
2017-06-29 11:15 ` Van Haaren, Harry
2017-06-26 11:59 ` [dpdk-dev] [PATCH 1/6] service cores: header and implementation Jerin Jacob
2017-06-29 11:13 ` Van Haaren, Harry [this message]
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 0/5] service cores: cover letter Harry van Haaren
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 1/5] service cores: header and implementation Harry van Haaren
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 2/5] service cores: EAL init changes Harry van Haaren
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 3/5] service cores: coremask parsing Harry van Haaren
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 4/5] service cores: add unit tests Harry van Haaren
2017-06-29 11:23 ` [dpdk-dev] [PATCH v2 5/5] service cores: enable event/sw with service Harry van Haaren
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 0/7] service cores: cover letter Harry van Haaren
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 1/7] service cores: header and implementation Harry van Haaren
2017-07-04 17:16 ` Jerin Jacob
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 2/7] service cores: EAL init changes Harry van Haaren
2017-07-04 11:35 ` Jerin Jacob
2017-07-07 16:28 ` Van Haaren, Harry
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 3/7] service cores: coremask parsing Harry van Haaren
2017-07-04 12:45 ` Jerin Jacob
2017-07-06 14:47 ` Van Haaren, Harry
2017-07-07 10:45 ` Jerin Jacob
2017-07-07 10:57 ` Van Haaren, Harry
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 4/7] service cores: add unit tests Harry van Haaren
2017-07-04 11:14 ` Jerin Jacob
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 5/7] service cores: enable event/sw with service Harry van Haaren
2017-07-04 10:52 ` Jerin Jacob
2017-07-07 16:28 ` Van Haaren, Harry
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 6/7] maintainers: claim service cores Harry van Haaren
2017-07-04 10:53 ` Jerin Jacob
2017-07-02 21:35 ` [dpdk-dev] [PATCH v3 7/7] doc: add service cores to doc and release notes Harry van Haaren
2017-07-02 22:16 ` Mcnamara, John
2017-07-04 10:56 ` Jerin Jacob
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 0/7] service cores: cover letter Harry van Haaren
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 1/7] service cores: header and implementation Harry van Haaren
2017-07-11 8:29 ` Jerin Jacob
2017-07-11 9:54 ` Thomas Monjalon
2017-07-11 12:32 ` Van Haaren, Harry
2017-07-11 12:44 ` Jerin Jacob
2017-07-11 12:49 ` Van Haaren, Harry
2017-07-11 14:10 ` Van Haaren, Harry
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 2/7] service cores: EAL init changes Harry van Haaren
2017-07-11 7:42 ` Jerin Jacob
2017-07-11 14:11 ` Van Haaren, Harry
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 3/7] service cores: coremask parsing Harry van Haaren
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 4/7] service cores: add unit tests Harry van Haaren
2017-07-11 8:12 ` Jerin Jacob
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 5/7] event/sw: enable SW PMD with service capability Harry van Haaren
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 6/7] doc: add service cores to doc and release notes Harry van Haaren
2017-07-07 16:41 ` [dpdk-dev] [PATCH v4 7/7] maintainers: claim service cores Harry van Haaren
2017-07-11 7:53 ` Jerin Jacob
2017-07-09 22:08 ` [dpdk-dev] [PATCH v4 0/7] service cores: cover letter Thomas Monjalon
2017-07-10 8:18 ` Van Haaren, Harry
2017-07-10 11:41 ` Jerin Jacob
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 " Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 1/7] service cores: header and implementation Harry van Haaren
2017-07-12 16:35 ` Jerin Jacob
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 2/7] service cores: EAL init changes Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 3/7] service cores: coremask parsing Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 4/7] service cores: add unit tests Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 5/7] event/sw: enable SW PMD with service capability Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 6/7] doc: add service cores to doc and release notes Harry van Haaren
2017-07-11 14:19 ` [dpdk-dev] [PATCH v5 7/7] maintainers: claim service cores Harry van Haaren
2017-07-12 16:49 ` [dpdk-dev] [PATCH v5 0/7] service cores: cover letter Jerin Jacob
2017-07-16 19:25 ` Thomas Monjalon
2017-07-17 8:07 ` Van Haaren, Harry
2017-07-17 15:21 ` [dpdk-dev] [PATCH] service: add corelist to EAL arguments Harry van Haaren
2017-07-17 15:53 ` Ananyev, Konstantin
2017-07-17 15:58 ` Van Haaren, Harry
2017-07-17 16:10 ` Ananyev, Konstantin
2017-07-17 16:16 ` Van Haaren, Harry
2017-07-19 5:42 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E923DB57A917B54B9182A2E928D00FA640C33C53@IRSMSX102.ger.corp.intel.com \
--to=harry.van.haaren@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=keith.wiles@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).