> From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com]
<snip email headers>
> > Hi Harry,
> >
> > Overall it looks good. Some initial comments
Off to a good start! Replies inline, in general I think we're on the same page.
This RFC was pretty "fresh", mostly to check if the community agrees with the
general concept of service-cores. The implementation details (although going
well) will have some churn I expect :)
I'd like opinions from others in the community - I'll leave this thread "open"
for a while to get more feedback, and rework a v2 RFC then.
Thanks for your review - Harry
> > > This patch adds a service header to DPDK EAL. This header is
> > > an RFC for a mechanism to allow DPDK components request a
> > > callback function to be invoked.
> > >
> > > The application can set the number of service cores, and
> > > a coremask for each particular services. The implementation
> > > of this functionality in rte_services.c (not completed) would
> > > use atomics to ensure that a service can only be polled by a
> > > single lcore at a time.
> >
> > single lcore at a time "if multipthread_capable flag is not set". Right?
> >
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
> > > ---
> > > lib/librte_eal/common/include/rte_service.h | 211
> > > ++++++++++++++++++++++++++++
> > > 1 file changed, 211 insertions(+)
> > > create mode 100644 lib/librte_eal/common/include/rte_service.h
> > >
> > > diff --git a/lib/librte_eal/common/include/rte_service.h
> b/lib/librte_eal/common/include/rte_service.h
> > > new file mode 100644
> > > index 0000000..cfa26f3
> > > --- /dev/null
> > > +++ b/lib/librte_eal/common/include/rte_service.h
> > > @@ -0,0 +1,211 @@
> > > +/*
> > > + * BSD LICENSE
> > > + *
> > > + * Copyright(c) 2017 Intel Corporation. All rights reserved.
> > > + *
> > > + * Redistribution and use in source and binary forms, with or without
> > > + * modification, are permitted provided that the following conditions
> > > + * are met:
> > > + *
> > > + * * Redistributions of source code must retain the above copyright
> > > + * notice, this list of conditions and the following disclaimer.
> > > + * * Redistributions in binary form must reproduce the above
> > > copyright
> > > + * notice, this list of conditions and the following disclaimer in
> > > + * the documentation and/or other materials provided with the
> > > + * distribution.
> > > + * * Neither the name of Intel Corporation nor the names of its
> > > + * contributors may be used to endorse or promote products derived
> > > + * from this software without specific prior written permission.
> > > + *
> > > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> > > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> > > FOR
> > > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> > > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> > > INCIDENTAL,
> > > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> > > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> > > USE,
> > > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> > > ANY
> > > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
> > > USE
> > > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > + */
> > > +
> > > +#ifndef _RTE_SERVICE_H_
> > > +#define _RTE_SERVICE_H_
> > > +
> > > +/**
> > > + * @file
> > > + *
> > > + * Service functions
> > > + *
> > > + * The service functionality provided by this header allows a DPDK
> > > component
> > > + * to indicate that it requires a function call in order for it to
> > > perform
> > > + * its processing.
> > > + *
> > > + * An example usage of this functionality would be a component that
> > > registers
> > > + * a service to perform a particular packet processing duty: for example
> > > the
> > > + * eventdev software PMD. At startup the application requests all
> > > services
> > > + * that have been registered, and the app decides how many cores will
> > > run the
> >
> > s/app/application
Noted
> > > + * required services. The EAL removes these number of cores from the
> > > available
> > > + * runtime cores, and dedicates them to performing service-core
> > > workloads. The
> > > + * application has access to the remaining lcores as normal.
> > > + *
> > > + * An example of the service core infrastructure with an application
> > > + * configuring a single service (the eventdev sw PMD), and dedicating
> > > one core
> > > + * exclusively to run the service would interact with the API as follows:
> > > + *
> > > + * 1. Eventdev SW PMD calls *rte_eal_service_register*, indicating that
> > > it
> > > + * requires an lcore to call a function in order to operate. EAL
> > > registers
> > > + * this service, but performs no other actions yet.
> > > + *
> > > + * 2. Application calls *rte_eal_service_get*, allowing EAL to provide it
> > > + * with an array of *rte_service_config* structures. These structures
> > > + * provide the application with the name of the service, along with
> > > + * metadata provided by the service when it was registered.
> > > + *
> > > + * 3. The application logic iterates over the services that require
> > > running,
> > > + * and decides to run the eventdev sw PMD service using one lcore.
> > > + *
> > > + * 4. The application calls *rte_eal_service_use_lcore* multiple times,
> > > once
> > > + * for each lcore that should be used as a service core. These cores
> > > are
> > > + * removed from the application usage, and EAL will refuse to launch
> > > + * user-specified functions on these cores.
> > > + *
> > > + * 5. The application calls *rte_eal_service_set_coremask* to set the
> > > coremask
> > > + * for the service. Note that EAL is already aware of ALL lcores that
> > > will
> > > + * be used for service-core purposes (see step 4 above) which allows
> > > EAL to
> > > + * error-check the coremask here, and ensure at least one core is
> > > going to
> > > + * be running the service.
> >
> > Regarding step 4 and 5, It looks like,
> > a) The lcore_id information is duplicate in step 4 and 5.
Not really duplicated - keep in mind we do not want to have a 1:1 mapping, the
goal of this is to allow e.g. 3 cores -> 5 services type mappings, where the
work is shared between the cores. Hence, setting coremasks and setting lcores
to use are very related, but not quite the same.
> > b) On rte_eal_service_set_coremask() failure, you may the need
> > inverse of rte_eal_service_use_lcore()(setting back to worker/normal
> > lcore)
We can pass the "type" of lcore as an argument to a function:
#define RTE_EAL_SERVICE_TYPE_APPLICATION 0
#define RTE_EAL_SERVICE_TYPE_SERVICE 1
rte_eal_service_set_lcore_type(uint32_t type);
Allows adding more core types (if anybody can think of any) and avoids
function-count bloat :)
> > But I understand the need for step 5.
> >
> > How about changing the (4) and (5) as
> >
> > step 4) rte_eal_service_set_coremask
> > step 5) rte_eal_service_apply(void)(or similar name) for error check and
> > ensure at least on core is going to be running the service.
So the sequence would be:
for( i < app_n_service_cores )
rte_eal_service_set_lcore_type( SERVICE );
for( i < services_count )
rte_eal_serivce_set_coremask( app_decided_coremask_here );
int ret = rte_eal_service_apply();
if(ret) {
/* application can rte_panic(), or reset CPU cores to default APP state and
continue */
}
/* application profits from service-cores feature in EAL */
> >
> > > + *
> > > + * 6. The application now calls remote_launch() as usual, and the
> > > application
> > > + * can perform its processing as required without manually handling
> > > the
> > > + * partitioning of lcore resources for DPDK functionality.
> > > + */
> > > +
> > > +#ifdef __cplusplus
> > > +extern "C" {
> > > +#endif
> > > +
> > > +#include <stdint.h>
> > > +
> > > +#define RTE_SERVICE_NAMESIZE 32
> > > +
> > > +/**
> > > + * Signature of callback back function to run a service.
> > > + */
> > > +typedef void (*rte_eal_service_func)(void *args);
> > > +
> > > +struct rte_service_config {
> >
> > I think, better name is rte_service_spec or something similar
Sure, seems more representative.
> > > + /* name of the service */
> > > + char name[RTE_SERVICE_NAMESIZE];
> > > + /* cores that run this service */
> >
> > If I understand it correctly,
> > a) Its for NUMA
> > b) Basically advertising the core(s) which capable or preferred to run the
> > service. Not the number of cores "required" to run the service.
> > if so, update the comments
Yes NUMA needs to be taken into account.
> > > + uint64_t coremask;
> >
> > 64bits are not enough to represent the coremask. May be array of
> > uint64_t or array of int can be used. I think, latter is more inline
> > with exiting eal scheme
Yes aware of this when posting - but given its an RFC didn't address. Good
point though :)
> Two more questions,
>
> 1) If its for only for NUMA. Is it socket_id mask enough ?
I'll work on a solution for v2 RFC, and investigate the EAL coremask
implementation for some ideas then.
> 2) What would be the tear down sequence of the service core especially when
> device stop or close happens?
Good question. Being able to "turn off" a single service while keeping other
services running would be useful I think. That might require some extra
tracking at the implementation layer, but perhaps something that's worth doing.
Opinions and use-cases welcome here!
