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

  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.

Signed-off-by: Harry van Haaren <harry.van.haaren@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
  

-----Original Message-----
> Date: Wed, 3 May 2017 12:29:21 +0100
> From: Harry van Haaren <harry.van.haaren@intel.com>
> To: dev@dpdk.org
> CC: Harry van Haaren <harry.van.haaren@intel.com>,
>  bruce.richardson@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com,
>  narender.vangati@intel.com, jerin.jacob@caviumnetworks.com,
>  gage.eads@intel.com
> Subject: [RFC] service core concept header implementation
> X-Mailer: git-send-email 2.7.4

Hi Harry,

Overall it looks good. Some initial comments

> 
> 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@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

> + * 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.
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)

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.

> + *
> + * 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

> +	/* 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

> +	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

> +	/* when set, multiple lcores can run this service simultaneously without
> +	 * the need for software atomics to ensure that two cores do not
> +	 * attempt to run the service at the same time.
> +	 */
> +	uint8_t multithread_capable;
> +};
> +
> +/** @internal - this will not be visible to application, defined in a seperate
> + * rte_eal_service_impl.h header. The application does not need to be exposed
> + * to the actual function pointers - so hide them. */
> +struct rte_service {
> +	/* callback to be called to run the service */
> +	rte_eal_service_func cb;
> +	/* args to the callback function */
> +	void *cb_args;
> +	/* configuration of the service */
> +	struct rte_service_config config;
> +};
> +
> +/**
> + * @internal - Only DPDK internal components request "service" cores.

remove @internal here

> + *
> + * Registers a service in EAL.
> + *
> + * Registered services' configurations are exposed to an application using
> + * *rte_eal_service_get_all*. These services have names which can be understood
> + * by the application, and the application logic can decide how to allocate
> + * cores to run the various services.
> + *
> + * This function is expected to be called by a DPDK component to indicate that
> + * it require a CPU to run a specific function in order for it to perform its
> + * processing. An example of such a component is the eventdev software PMD.
> + *
> + * The config struct should be filled in as appropriate by the PMD. For example
> + * the name field should include some level of detail (e.g. "ethdev_p1_rx_q3"
> + * might mean RX from an ethdev from port 1, on queue 3).
> + *
> + * @param service
> + *   The service structure to be registered with EAL.
> + *
> + * @return
> + *   On success, zero
> + *   On failure, a negative error
> + */
> +int rte_eal_service_register(const struct rte_service *service);
> +
> +/**
> + * Get the count of services registered in the EAL.
> + *
> + * @return the number of services registered with EAL.
> + */
> +uint32_t rte_eal_service_get_count();
> +
> +/**
> + * Writes all registered services to the application supplied array.
> + *
> + * This function can be used by the application to understand if and what
> + * services require running. Each service provides a config struct exposing
> + * attributes of the service, which can be used by the application to decide on
> + * its strategy of running services on cores.
> + *
> + * @param service_config

param [out] service_config.

> + *   An array of service config structures to be filled in
> + *
> + * @param n
> + *   The size of the service config array
> + *
> + * @return 0 on successful providing all service configs
> + *         -ENOSPC if array passed in is too small
> + */
> +int rte_eal_service_get_all(const struct rte_service_config *service_config,
> +			    uint32_t n);
> +
> +/**
> + * Use *lcore_id* as a service core.
> + *
> + * EAL will internally remove *lcore_id* from the application accessible
> + * core list. As a result, the application will never have its code run on
> + * the service core, making the service cores totally transparent to an app.
> + *
> + * This function should be called before *rte_eal_service_set_coremask* so that
> + * when setting the core mask the value can be error checked to ensure that EAL
> + * has an lcore backing the coremask specified.
> + */
> +int rte_eal_service_use_lcore(uint16_t lcore_id);
> +
> +/**
> + * Set a coremask for a service.
> + *
> + * A configuration for a service indicates which cores run the service, and
> + * what other parameters are required to correclty handle the underlying device.
> + *
> + * Examples of advanced usage might be a HW device that supports multiple lcores
> + * transmitting packets on the same queue - the hardware provides a mechanism
> + * for multithreaded enqueue. In this case, the atomic_
Missing text after atomic_.


missing @param
> + *
> + * @return 0 Success
> + *         -ENODEV   Device with *name* does not exist
> + *         -ENOTSUP  Configuration is un-supported
> + */
> +int rte_eal_service_set_coremask(const char *name, uint64_t coremask);
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +
> +#endif /* _RTE_SERVICE_H_ */
> -- 
> 2.7.4
>
  

-----Original Message-----
> Date: Thu, 4 May 2017 12:05:54 +0530
> From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> To: Harry van Haaren <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org, bruce.richardson@intel.com, hemant.agrawal@nxp.com,
>  nipun.gupta@nxp.com, narender.vangati@intel.com, gage.eads@intel.com
> Subject: Re: [RFC] service core concept header implementation
> User-Agent: Mutt/1.8.2 (2017-04-18)
> 
> -----Original Message-----
> > Date: Wed, 3 May 2017 12:29:21 +0100
> > From: Harry van Haaren <harry.van.haaren@intel.com>
> > To: dev@dpdk.org
> > CC: Harry van Haaren <harry.van.haaren@intel.com>,
> >  bruce.richardson@intel.com, hemant.agrawal@nxp.com, nipun.gupta@nxp.com,
> >  narender.vangati@intel.com, jerin.jacob@caviumnetworks.com,
> >  gage.eads@intel.com
> > Subject: [RFC] service core concept header implementation
> > X-Mailer: git-send-email 2.7.4
> 
> Hi Harry,
> 
> Overall it looks good. Some initial comments
> 
> > 
> > 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@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
> 
> > + * 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.
> 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)
> 
> 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.
> 
> > + *
> > + * 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
> 
> > +	/* 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
> 
> > +	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

Two more questions,

1) If its for only for NUMA. Is it socket_id mask enough ?
2) What would be the tear down sequence of the service core especially when
device stop or close happens?

> 
> > +	/* when set, multiple lcores can run this service simultaneously without
> > +	 * the need for software atomics to ensure that two cores do not
> > +	 * attempt to run the service at the same time.
> > +	 */
> > +	uint8_t multithread_capable;
> > +};
> > +

























































































.

s
  

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

-----Original Message-----
> Date: Fri, 5 May 2017 15:48:02 +0000
> From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, "Richardson, Bruce"
>  <bruce.richardson@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
>  "Vangati, Narender" <narender.vangati@intel.com>, "Eads, Gage"
>  <gage.eads@intel.com>
> Subject: RE: [RFC] service core concept header implementation
> 
> 
> > From: Jerin Jacob [mailto:jerin.jacob@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
> 
> 
> > >
> > > 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 );

I thought, The application may not need to set the explicit
rte_eal_service_set_lcore_type, it can be internally set by
rte_eal_service_set_coremask

i.e

for( i < services_count )
    rte_eal_service_set_coremask( app_decided_coremask_here );

int ret = rte_eal_service_apply();

rte_eal_service_set_coremask(app_decided_coremask_here)
{
	proposed_implementation;
	for_each_bit_set(app_decided_coremask_here)
		rte_eal_service_set_lcore_type(SERVICE);
}


> 
> 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 */
> 
> > 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!

I think it makes sense to add "turn off" support for each service
functions as we are multiplexing the service core.


> 
> 
>
  

Hi Harry,
I had a look, my questions/comments below.
From  my perspective it looks like an attempt to introduce simple scheduling library inside DPDK.
Though some things left unclear from my perspective, but probably I just misunderstood your
intentions here.
Thanks
Konstantin

> 
> 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.
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@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
> 

...

> +
> +/**
> + * Signature of callback back function to run a service.
> + */
> +typedef void (*rte_eal_service_func)(void *args);
> +
> +struct rte_service_config {
> +	/* name of the service */
> +	char name[RTE_SERVICE_NAMESIZE];
> +	/* cores that run this service */
> +	uint64_t coremask;

Why uint64_t?
Even now by default we support more than 64 cores.
We have rte_cpuset_t for such things.

Ok, the first main question:
Is that possible to register multiple services with intersecting coremasks or not?
If not, then I suppose there is no practical difference from what we have right now
with eal_remote_launch() .
If yes, then several questions arise:
   a) How the service scheduling function will going to switch from one service to another
       on particular core?
       As we don't have interrupt framework for that, I suppose the only choice is to rely
       on service voluntary fiving up cpu. Is that so?
 b) Would it be possible to specify percentage of core cycles that service can consume?
     I suppose the answer is 'no', at least for current iteration. 

> +	/* when set, multiple lcores can run this service simultaneously without
> +	 * the need for software atomics to ensure that two cores do not
> +	 * attempt to run the service at the same time.
> +	 */
> +	uint8_t multithread_capable;

Ok, and what would happen if this flag is not set, and user specified more
then one cpu in the coremask?

> +};
> +
> +/** @internal - this will not be visible to application, defined in a seperate
> + * rte_eal_service_impl.h header.

Why not?
If the application  registers the service, it for sure needs to know what exactly that service
would do (cb func and data).

> The application does not need to be exposed
> + * to the actual function pointers - so hide them. */
> +struct rte_service {
> +	/* callback to be called to run the service */
> +	rte_eal_service_func cb;
> +	/* args to the callback function */
> +	void *cb_args;

If user plans to run that service on multiple cores, then he might
need a separate instance of cb_args for each core.

> +	/* configuration of the service */
> +	struct rte_service_config config;
> +};
> +
> +/**
> + * @internal - Only DPDK internal components request "service" cores.
> + *
> + * Registers a service in EAL.
> + *
> + * Registered services' configurations are exposed to an application using
> + * *rte_eal_service_get_all*. These services have names which can be understood
> + * by the application, and the application logic can decide how to allocate
> + * cores to run the various services.
> + *
> + * This function is expected to be called by a DPDK component to indicate that
> + * it require a CPU to run a specific function in order for it to perform its
> + * processing. An example of such a component is the eventdev software PMD.
> + *
> + * The config struct should be filled in as appropriate by the PMD. For example
> + * the name field should include some level of detail (e.g. "ethdev_p1_rx_q3"
> + * might mean RX from an ethdev from port 1, on queue 3).
> + *
> + * @param service
> + *   The service structure to be registered with EAL.
> + *
> + * @return
> + *   On success, zero
> + *   On failure, a negative error
> + */
> +int rte_eal_service_register(const struct rte_service *service);
> +
> +/**
> + * Get the count of services registered in the EAL.
> + *
> + * @return the number of services registered with EAL.
> + */
> +uint32_t rte_eal_service_get_count();
> +
> +/**
> + * Writes all registered services to the application supplied array.
> + *
> + * This function can be used by the application to understand if and what
> + * services require running. Each service provides a config struct exposing
> + * attributes of the service, which can be used by the application to decide on
> + * its strategy of running services on cores.
> + *
> + * @param service_config
> + *   An array of service config structures to be filled in
> + *
> + * @param n
> + *   The size of the service config array
> + *
> + * @return 0 on successful providing all service configs
> + *         -ENOSPC if array passed in is too small
> + */
> +int rte_eal_service_get_all(const struct rte_service_config *service_config,
> +			    uint32_t n);

I'd suggest to follow snprintf() notation here: always return number of all existing services.
Then you wouldn't need rte_eal_service_get_count() at all.

> +
> +/**
> + * Use *lcore_id* as a service core.
> + *
> + * EAL will internally remove *lcore_id* from the application accessible
> + * core list. As a result, the application will never have its code run on
> + * the service core, making the service cores totally transparent to an app.
> + *
> + * This function should be called before *rte_eal_service_set_coremask* so that
> + * when setting the core mask the value can be error checked to ensure that EAL
> + * has an lcore backing the coremask specified.
> + */
> +int rte_eal_service_use_lcore(uint16_t lcore_id);
> +
> +/**
> + * Set a coremask for a service.
> + *
> + * A configuration for a service indicates which cores run the service, and
> + * what other parameters are required to correclty handle the underlying device.
> + *
> + * Examples of advanced usage might be a HW device that supports multiple lcores
> + * transmitting packets on the same queue - the hardware provides a mechanism
> + * for multithreaded enqueue. In this case, the atomic_
> + *
> + * @return 0 Success
> + *         -ENODEV   Device with *name* does not exist
> + *         -ENOTSUP  Configuration is un-supported
> + */
> +int rte_eal_service_set_coremask(const char *name, uint64_t coremask);

Not sure why do we need that function?
We do know a service coremask after register() is executed.
Would it be possible to add/remove cores from the service on the fly?
I.E without stopping the actual service scheduling function first?

>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.

Ok, if the user has to do remote_launch() manually for each service core anyway...
Again it means user can't start/stop individual services,  it has to stop the whole service
core first(and all services it is running) to stop a particular service.
Sounds not very convenient for the user from my perspective.
Wonder can we have an API like that:

/*
 * reserves all cores in coremask as service cores.
 * in fact it would mean eal_remote_launch(service_main_func) on each of them.
 */
int rte_eal_service_start(const rte_cpuset_t *coremask);

/*
 * stops all services and returns service lcores back to the EAL.
 */
int rte_eal_service_stop((const rte_cpuset_t *coremask);

struct rte_service_config  {name; cb_func; flags; ...};
    
struct rte_service *rte_service_register(struct rte_service_config  *cfg);
rte_service_unregister(struct rte_service *srv);

/*
  * adds/deletes lcore from the list of service cpus to run this service on.
  * An open question - should we allow to do add/del lcore while service is running or not.
  * From user perspective, it would be much more convenient to allow.  
  */
int rte_service_add_lcore(struct rte_service *, uint32_t lcore_id, void *lcore_cb_arg);
int rte_service_del_lcore(struct rte_service *, uint32_t lcore_id);

/*
 * starts/stops the service.
 */
int rte_service_start(struct rte_service *srv, const rte_cpuset_t   *coremask);
int rte_service_stop(struct rte_service *srv, const rte_cpuset_t   *coremask);
  

On Wed, May 17, 2017 at 12:47:52PM +0000, Ananyev, Konstantin wrote:
> Hi Harry,
> I had a look, my questions/comments below.
> From  my perspective it looks like an attempt to introduce simple scheduling library inside DPDK.
> Though some things left unclear from my perspective, but probably I just misunderstood your
> intentions here.
> Thanks
> Konstantin
> 

Hi Konstantin,

Thanks for the feedback, the specific detail of which I'll perhaps leave
for Harry to reply to or include in a later version of this patchset.
However, from a higher level, I think the big difference in what we
envisage compared to what you suggest in your comments is that these are
not services set up by the application. If the app wants to run
something it uses remote_launch as now. This is instead for other
components to request threads - or a share of a thread - for their own
use, since they cannot call remote_launch directly, as the app owns the
threads, not the individual libraries.
See also comments made in reply to Thomas mail.

Hope this helps clarify things a little.

/Bruce

> > 
> > 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.
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@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
> > 
> 
> ...
> 
> > +
> > +/**
> > + * Signature of callback back function to run a service.
> > + */
> > +typedef void (*rte_eal_service_func)(void *args);
> > +
> > +struct rte_service_config {
> > +	/* name of the service */
> > +	char name[RTE_SERVICE_NAMESIZE];
> > +	/* cores that run this service */
> > +	uint64_t coremask;
> 
> Why uint64_t?
> Even now by default we support more than 64 cores.
> We have rte_cpuset_t for such things.
> 
> Ok, the first main question:
> Is that possible to register multiple services with intersecting coremasks or not?
> If not, then I suppose there is no practical difference from what we have right now
> with eal_remote_launch() .
> If yes, then several questions arise:
>    a) How the service scheduling function will going to switch from one service to another
>        on particular core?
>        As we don't have interrupt framework for that, I suppose the only choice is to rely
>        on service voluntary fiving up cpu. Is that so?
>  b) Would it be possible to specify percentage of core cycles that service can consume?
>      I suppose the answer is 'no', at least for current iteration. 
> 
> > +	/* when set, multiple lcores can run this service simultaneously without
> > +	 * the need for software atomics to ensure that two cores do not
> > +	 * attempt to run the service at the same time.
> > +	 */
> > +	uint8_t multithread_capable;
> 
> Ok, and what would happen if this flag is not set, and user specified more
> then one cpu in the coremask?
> 
> > +};
> > +
> > +/** @internal - this will not be visible to application, defined in a seperate
> > + * rte_eal_service_impl.h header.
> 
> Why not?
> If the application  registers the service, it for sure needs to know what exactly that service
> would do (cb func and data).
> 
> > The application does not need to be exposed
> > + * to the actual function pointers - so hide them. */
> > +struct rte_service {
> > +	/* callback to be called to run the service */
> > +	rte_eal_service_func cb;
> > +	/* args to the callback function */
> > +	void *cb_args;
> 
> If user plans to run that service on multiple cores, then he might
> need a separate instance of cb_args for each core.
> 
> > +	/* configuration of the service */
> > +	struct rte_service_config config;
> > +};
> > +
> > +/**
> > + * @internal - Only DPDK internal components request "service" cores.
> > + *
> > + * Registers a service in EAL.
> > + *
> > + * Registered services' configurations are exposed to an application using
> > + * *rte_eal_service_get_all*. These services have names which can be understood
> > + * by the application, and the application logic can decide how to allocate
> > + * cores to run the various services.
> > + *
> > + * This function is expected to be called by a DPDK component to indicate that
> > + * it require a CPU to run a specific function in order for it to perform its
> > + * processing. An example of such a component is the eventdev software PMD.
> > + *
> > + * The config struct should be filled in as appropriate by the PMD. For example
> > + * the name field should include some level of detail (e.g. "ethdev_p1_rx_q3"
> > + * might mean RX from an ethdev from port 1, on queue 3).
> > + *
> > + * @param service
> > + *   The service structure to be registered with EAL.
> > + *
> > + * @return
> > + *   On success, zero
> > + *   On failure, a negative error
> > + */
> > +int rte_eal_service_register(const struct rte_service *service);
> > +
> > +/**
> > + * Get the count of services registered in the EAL.
> > + *
> > + * @return the number of services registered with EAL.
> > + */
> > +uint32_t rte_eal_service_get_count();
> > +
> > +/**
> > + * Writes all registered services to the application supplied array.
> > + *
> > + * This function can be used by the application to understand if and what
> > + * services require running. Each service provides a config struct exposing
> > + * attributes of the service, which can be used by the application to decide on
> > + * its strategy of running services on cores.
> > + *
> > + * @param service_config
> > + *   An array of service config structures to be filled in
> > + *
> > + * @param n
> > + *   The size of the service config array
> > + *
> > + * @return 0 on successful providing all service configs
> > + *         -ENOSPC if array passed in is too small
> > + */
> > +int rte_eal_service_get_all(const struct rte_service_config *service_config,
> > +			    uint32_t n);
> 
> I'd suggest to follow snprintf() notation here: always return number of all existing services.
> Then you wouldn't need rte_eal_service_get_count() at all.
> 
> > +
> > +/**
> > + * Use *lcore_id* as a service core.
> > + *
> > + * EAL will internally remove *lcore_id* from the application accessible
> > + * core list. As a result, the application will never have its code run on
> > + * the service core, making the service cores totally transparent to an app.
> > + *
> > + * This function should be called before *rte_eal_service_set_coremask* so that
> > + * when setting the core mask the value can be error checked to ensure that EAL
> > + * has an lcore backing the coremask specified.
> > + */
> > +int rte_eal_service_use_lcore(uint16_t lcore_id);
> > +
> > +/**
> > + * Set a coremask for a service.
> > + *
> > + * A configuration for a service indicates which cores run the service, and
> > + * what other parameters are required to correclty handle the underlying device.
> > + *
> > + * Examples of advanced usage might be a HW device that supports multiple lcores
> > + * transmitting packets on the same queue - the hardware provides a mechanism
> > + * for multithreaded enqueue. In this case, the atomic_
> > + *
> > + * @return 0 Success
> > + *         -ENODEV   Device with *name* does not exist
> > + *         -ENOTSUP  Configuration is un-supported
> > + */
> > +int rte_eal_service_set_coremask(const char *name, uint64_t coremask);
> 
> Not sure why do we need that function?
> We do know a service coremask after register() is executed.
> Would it be possible to add/remove cores from the service on the fly?
> I.E without stopping the actual service scheduling function first?
> 
> >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.
> 
> Ok, if the user has to do remote_launch() manually for each service core anyway...
> Again it means user can't start/stop individual services,  it has to stop the whole service
> core first(and all services it is running) to stop a particular service.
> Sounds not very convenient for the user from my perspective.
> Wonder can we have an API like that:
> 
> /*
>  * reserves all cores in coremask as service cores.
>  * in fact it would mean eal_remote_launch(service_main_func) on each of them.
>  */
> int rte_eal_service_start(const rte_cpuset_t *coremask);
> 
> /*
>  * stops all services and returns service lcores back to the EAL.
>  */
> int rte_eal_service_stop((const rte_cpuset_t *coremask);
> 
> struct rte_service_config  {name; cb_func; flags; ...};
>     
> struct rte_service *rte_service_register(struct rte_service_config  *cfg);
> rte_service_unregister(struct rte_service *srv);
> 
> /*
>   * adds/deletes lcore from the list of service cpus to run this service on.
>   * An open question - should we allow to do add/del lcore while service is running or not.
>   * From user perspective, it would be much more convenient to allow.  
>   */
> int rte_service_add_lcore(struct rte_service *, uint32_t lcore_id, void *lcore_cb_arg);
> int rte_service_del_lcore(struct rte_service *, uint32_t lcore_id);
> 
> /*
>  * starts/stops the service.
>  */
> int rte_service_start(struct rte_service *srv, const rte_cpuset_t   *coremask);
> int rte_service_stop(struct rte_service *srv, const rte_cpuset_t   *coremask);
> 
> 
> 
> 
> 
> 
> 
>
  

Hi Bruce,

> >
> 
> Hi Konstantin,
> 
> Thanks for the feedback, the specific detail of which I'll perhaps leave
> for Harry to reply to or include in a later version of this patchset.
> However, from a higher level, I think the big difference in what we
> envisage compared to what you suggest in your comments is that these are
> not services set up by the application. If the app wants to run
> something it uses remote_launch as now. This is instead for other
> components to request threads - or a share of a thread - for their own
> use, since they cannot call remote_launch directly, as the app owns the
> threads, not the individual libraries.

Ok, thanks for clarification about who supposed to be the main consumer for that.
That makes sense.
Though  I am not sure why the API suggested wouldn't fit your purposes. 
Though I think both PMD/core libraries  and app layer might be interested in that: i.e.
app might also have some background processing tasks, that might need to be run
on several cores (or core slices).
Konstantin

> See also comments made in reply to Thomas mail.
> 
> Hope this helps clarify things a little.
> 
> /Bruce
> 
> > >
> > > 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.
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@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
> > >
> >
> > ...
> >
> > > +
> > > +/**
> > > + * Signature of callback back function to run a service.
> > > + */
> > > +typedef void (*rte_eal_service_func)(void *args);
> > > +
> > > +struct rte_service_config {
> > > +	/* name of the service */
> > > +	char name[RTE_SERVICE_NAMESIZE];
> > > +	/* cores that run this service */
> > > +	uint64_t coremask;
> >
> > Why uint64_t?
> > Even now by default we support more than 64 cores.
> > We have rte_cpuset_t for such things.
> >
> > Ok, the first main question:
> > Is that possible to register multiple services with intersecting coremasks or not?
> > If not, then I suppose there is no practical difference from what we have right now
> > with eal_remote_launch() .
> > If yes, then several questions arise:
> >    a) How the service scheduling function will going to switch from one service to another
> >        on particular core?
> >        As we don't have interrupt framework for that, I suppose the only choice is to rely
> >        on service voluntary fiving up cpu. Is that so?
> >  b) Would it be possible to specify percentage of core cycles that service can consume?
> >      I suppose the answer is 'no', at least for current iteration.
> >
> > > +	/* when set, multiple lcores can run this service simultaneously without
> > > +	 * the need for software atomics to ensure that two cores do not
> > > +	 * attempt to run the service at the same time.
> > > +	 */
> > > +	uint8_t multithread_capable;
> >
> > Ok, and what would happen if this flag is not set, and user specified more
> > then one cpu in the coremask?
> >
> > > +};
> > > +
> > > +/** @internal - this will not be visible to application, defined in a seperate
> > > + * rte_eal_service_impl.h header.
> >
> > Why not?
> > If the application  registers the service, it for sure needs to know what exactly that service
> > would do (cb func and data).
> >
> > > The application does not need to be exposed
> > > + * to the actual function pointers - so hide them. */
> > > +struct rte_service {
> > > +	/* callback to be called to run the service */
> > > +	rte_eal_service_func cb;
> > > +	/* args to the callback function */
> > > +	void *cb_args;
> >
> > If user plans to run that service on multiple cores, then he might
> > need a separate instance of cb_args for each core.
> >
> > > +	/* configuration of the service */
> > > +	struct rte_service_config config;
> > > +};
> > > +
> > > +/**
> > > + * @internal - Only DPDK internal components request "service" cores.
> > > + *
> > > + * Registers a service in EAL.
> > > + *
> > > + * Registered services' configurations are exposed to an application using
> > > + * *rte_eal_service_get_all*. These services have names which can be understood
> > > + * by the application, and the application logic can decide how to allocate
> > > + * cores to run the various services.
> > > + *
> > > + * This function is expected to be called by a DPDK component to indicate that
> > > + * it require a CPU to run a specific function in order for it to perform its
> > > + * processing. An example of such a component is the eventdev software PMD.
> > > + *
> > > + * The config struct should be filled in as appropriate by the PMD. For example
> > > + * the name field should include some level of detail (e.g. "ethdev_p1_rx_q3"
> > > + * might mean RX from an ethdev from port 1, on queue 3).
> > > + *
> > > + * @param service
> > > + *   The service structure to be registered with EAL.
> > > + *
> > > + * @return
> > > + *   On success, zero
> > > + *   On failure, a negative error
> > > + */
> > > +int rte_eal_service_register(const struct rte_service *service);
> > > +
> > > +/**
> > > + * Get the count of services registered in the EAL.
> > > + *
> > > + * @return the number of services registered with EAL.
> > > + */
> > > +uint32_t rte_eal_service_get_count();
> > > +
> > > +/**
> > > + * Writes all registered services to the application supplied array.
> > > + *
> > > + * This function can be used by the application to understand if and what
> > > + * services require running. Each service provides a config struct exposing
> > > + * attributes of the service, which can be used by the application to decide on
> > > + * its strategy of running services on cores.
> > > + *
> > > + * @param service_config
> > > + *   An array of service config structures to be filled in
> > > + *
> > > + * @param n
> > > + *   The size of the service config array
> > > + *
> > > + * @return 0 on successful providing all service configs
> > > + *         -ENOSPC if array passed in is too small
> > > + */
> > > +int rte_eal_service_get_all(const struct rte_service_config *service_config,
> > > +			    uint32_t n);
> >
> > I'd suggest to follow snprintf() notation here: always return number of all existing services.
> > Then you wouldn't need rte_eal_service_get_count() at all.
> >
> > > +
> > > +/**
> > > + * Use *lcore_id* as a service core.
> > > + *
> > > + * EAL will internally remove *lcore_id* from the application accessible
> > > + * core list. As a result, the application will never have its code run on
> > > + * the service core, making the service cores totally transparent to an app.
> > > + *
> > > + * This function should be called before *rte_eal_service_set_coremask* so that
> > > + * when setting the core mask the value can be error checked to ensure that EAL
> > > + * has an lcore backing the coremask specified.
> > > + */
> > > +int rte_eal_service_use_lcore(uint16_t lcore_id);
> > > +
> > > +/**
> > > + * Set a coremask for a service.
> > > + *
> > > + * A configuration for a service indicates which cores run the service, and
> > > + * what other parameters are required to correclty handle the underlying device.
> > > + *
> > > + * Examples of advanced usage might be a HW device that supports multiple lcores
> > > + * transmitting packets on the same queue - the hardware provides a mechanism
> > > + * for multithreaded enqueue. In this case, the atomic_
> > > + *
> > > + * @return 0 Success
> > > + *         -ENODEV   Device with *name* does not exist
> > > + *         -ENOTSUP  Configuration is un-supported
> > > + */
> > > +int rte_eal_service_set_coremask(const char *name, uint64_t coremask);
> >
> > Not sure why do we need that function?
> > We do know a service coremask after register() is executed.
> > Would it be possible to add/remove cores from the service on the fly?
> > I.E without stopping the actual service scheduling function first?
> >
> > >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.
> >
> > Ok, if the user has to do remote_launch() manually for each service core anyway...
> > Again it means user can't start/stop individual services,  it has to stop the whole service
> > core first(and all services it is running) to stop a particular service.
> > Sounds not very convenient for the user from my perspective.
> > Wonder can we have an API like that:
> >
> > /*
> >  * reserves all cores in coremask as service cores.
> >  * in fact it would mean eal_remote_launch(service_main_func) on each of them.
> >  */
> > int rte_eal_service_start(const rte_cpuset_t *coremask);
> >
> > /*
> >  * stops all services and returns service lcores back to the EAL.
> >  */
> > int rte_eal_service_stop((const rte_cpuset_t *coremask);
> >
> > struct rte_service_config  {name; cb_func; flags; ...};
> >
> > struct rte_service *rte_service_register(struct rte_service_config  *cfg);
> > rte_service_unregister(struct rte_service *srv);
> >
> > /*
> >   * adds/deletes lcore from the list of service cpus to run this service on.
> >   * An open question - should we allow to do add/del lcore while service is running or not.
> >   * From user perspective, it would be much more convenient to allow.
> >   */
> > int rte_service_add_lcore(struct rte_service *, uint32_t lcore_id, void *lcore_cb_arg);
> > int rte_service_del_lcore(struct rte_service *, uint32_t lcore_id);
> >
> > /*
> >  * starts/stops the service.
> >  */
> > int rte_service_start(struct rte_service *srv, const rte_cpuset_t   *coremask);
> > int rte_service_stop(struct rte_service *srv, const rte_cpuset_t   *coremask);
> >
> >
> >
> >
> >
> >
> >
> >
  

Hi All,


<snip> Service Cores RFC, more details and discussion </snip>


> Though I think both PMD/core libraries  and app layer might be interested in that: i.e.
> app might also have some background processing tasks, that might need to be run
> on several cores (or core slices).


Thanks everybody for the input on the RFC - appreciated! From an application point-of-view, I see merit in Konstantin's suggestions for the API, over the RFC I sent previously. I will re-work the API taking inspiration from both APIs and send an RFCv2, you'll be on CC :)


Cheers, -Harry