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

[Bruce Richardson]

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