On Thu, Sep 21, 2023 at 10:17 PM Mattias Rönnblom <hof...@lysator.liu.se> wrote: > > On 2023-09-19 12:58, Jerin Jacob wrote: > > On Mon, Sep 4, 2023 at 6:39 PM Mattias Rönnblom > > <mattias.ronnb...@ericsson.com> wrote: > >> > >> The purpose of the dispatcher library is to help reduce coupling in an > >> Eventdev-based DPDK application. > >> > >> In addition, the dispatcher also provides a convenient and flexible > >> way for the application to use service cores for application-level > >> processing. > >> > >> Signed-off-by: Mattias Rönnblom <mattias.ronnb...@ericsson.com> > >> Tested-by: Peter Nilsson <peter.j.nils...@ericsson.com> > > > > High level architecture comment > > -------------------------------- > > > > 1) I think, we don't need tie this library ONLY to event dev > > application. It can be used with poll mode as well, > > that way traditiona pipeline application with ethdev as source could > > use this library dispatch the packets. > > > > They could potentially use a library *like this*, but I'm not sure it > should be this library, or at least I think it should be a different API > (better type checking, plus no obvious benefit of being more generic).
The only reason why I thought of this, It is cheap to do as all the logic for comparing match actions, packet/event aggregation and calling the action function is _same_ and better type checking can be added by separate callback for each source. and allow more user to use this library. I don't have a strong opinion of API semantic on this library API other than the placement. Feel free to ignore. > Another option for a traditional app calling rte_eth_rx_burst() directly > is to start using eventdev. :) Yes. Those who can afford extra SW cores to emulate eventdev or has evendev HW. > > > We dont need to implement that first version but API can make room for > > such abstractions. > > > > Based on my understanding in fast-path it has means to > > a)Pull out the events using rte_event_dequeue() > > b)Compare with registered match functions and call process upon match. > > > > if we abstract (a) as rte_dispatcher_source, We could pull from ethdev > > via rte_eth_rx_burst() or > > from ring via dequeue_burst API or so based on rte_dispatcher_source > > selected for dispatch configuration > > and we can use different sevice function pointers to have different service > > core > > implementation without effecting performance each sources. > > > > It could be generalized, for sure. I don't think it should be, at this > point at least. > > Non-event dev events could - and at this point I'm leaning toward > *should* - be consumed as a different DPDK service, but potentially on > the same lcore. > > If you would want to prepare for a future with several different event > sources, one could consider reintroducing the word "event" somewhere in > the dispatcher's name. So you would have > rte_event_dispatcher.h > rte_eth_dispatcher.h > > or > > rte_dispatcher_event.h > rte_dispatcher_eth.h Yes. > > High level cosmetic comment > > ---------------------------------------------------- > > 1)Missing doxygen connection- See doc/api/doxy-api-index.md > > > > rte_dispatcher.h is listed under **classification**, but this change is > in the programming guide patch. I'll move it to the patch containing the > header file. > > > > Process related comment > > ------------------------------------ > > 1) Documentation does not need need separate patch. All recent library > > changes documentation in same file. > > You could have doc and API header file as first patch and > > implementation as subsequent patches. > > > > > > I'm not sure how this is an improvement. Can you elaborate? For me, it > just seems like a change. > > Are there some guidelines on how to split a larger change into a patch > set? A section on this matter in the contribution guide would be great. In general, more patches easy review and attract more reviewers. Last library was added to dpdk is lib/mldev. You can see git log lib/mldev/ There operations like _create/free() etc made as separate patches. I leave it up to you and Thomas as this library will be merged though main tree. No strong opinion. > > > > >> diff --git a/lib/dispatcher/rte_dispatcher.h > >> b/lib/dispatcher/rte_dispatcher.h > >> new file mode 100644 > >> index 0000000000..6712687a08 > >> --- /dev/null > >> +++ b/lib/dispatcher/rte_dispatcher.h > >> @@ -0,0 +1,480 @@ > >> +/* SPDX-License-Identifier: BSD-3-Clause > >> + * Copyright(c) 2023 Ericsson AB > >> + */ > >> + > >> +#ifndef __RTE_DISPATCHER_H__ > >> +#define __RTE_DISPATCHER_H__ > >> + > > > > > > All new API should be experimental. See > > https://elixir.bootlin.com/dpdk/latest/source/lib/graph/rte_graph.h#L12 > > example. > > > > Noted. > > > > >> +/** > >> + * @file > >> + * > >> + * RTE Dispatcher > >> + * > >> + * The purpose of the dispatcher is to help decouple different parts > >> + * of an application (e.g., modules), sharing the same underlying > >> + * event device. > >> + > >> +/** > >> + * Function prototype for match callbacks. > >> + * > >> + * Match callbacks are used by an application to decide how the > >> + * dispatcher distributes events to different parts of the > >> + * application. > >> + * > >> + * The application is not expected to process the event at the point > >> + * of the match call. Such matters should be deferred to the process > >> + * callback invocation. > >> + * > >> + * The match callback may be used as an opportunity to prefetch data. > >> + * > >> + * @param event > >> + * Pointer to event > >> + * > >> + * @param cb_data > >> + * The pointer supplied by the application in > >> + * rte_dispatcher_register(). > >> + * > >> + * @return > >> + * Returns true in case this events should be delivered (via > >> + * the process callback), and false otherwise. > >> + */ > >> +typedef bool > >> +(*rte_dispatcher_match_t)(const struct rte_event *event, void *cb_data); > > > > > > a) Can we use void* event, so that it can be used with mbuf or other > > type by casting in the call back implementer. > > > > b) I was thinking, How we can avoid this function pointer and enable > > more have better performance at architecture level. > > > > Both x86, ARM has vector instructions[1] to form a vector from various > > offset from memory and compare N events > > in one shot. That is, if express match data like offset = X has value > > is Y and offset = X has value = A. > > I know, it may not good existing application using this APIs. But I > > believe, it will be more performance > > effective. If make sense, you can adapt to this.(Something to think about) > > > > There may be a future development where you try to shave off a few of > the circa 10 clock cycles per event of overhead that the current OK, as you wish.
