[dpdk-dev] [RFC] service core concept header implementation

[Van Haaren, Harry]

> From: Jerin Jacob [mailto:jerin.jacob at 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.haaren at 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!