Re: [PATCH v2 1/3] eventdev: introduce event dispatcher

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2023-08-18 08:09, Jerin Jacob wrote:
> On Fri, Jun 16, 2023 at 1:17 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>>
>> The purpose of the event dispatcher is to help reduce coupling in an
>> Eventdev-based DPDK application.
>>
>> In addition, the event 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.ronnblom@ericsson.com>
>> Tested-by: Peter Nilsson <peter.j.nilsson@ericsson.com>
>> Reviewed-by: Heng Wang <heng.wang@ericsson.com>
> 
> Adding eventdev maintainers and tech board,
> 
> Hi Mattais,
> 
> Finally, got some time to review this series, and thanks for excellent
> documentation.
> 
> I understand the use case for the dispatcher, But following are some
> of my concern
> 
> 1) To decouple the application specific business logic, one need to
> use two function pointers to access per packet (match and process)
> function.

The API design is based on community feedback, which suggested more 
flexibility was required than the initial 
"dispatching-based-on-queue-id-only" functionality the first RFC provided.

Where I expected to land was a design where I would have something like 
the RFC v2 design with match+process callbacks, and then a supplementary 
"hard-coded" dispatch-internal match API as well, where only the process 
function would be used (much like how RFC v1 worked).

It turned out the special-case API was not performing better (rather the 
opposite) for the evaluated use cases, so I dropped that idea.

That said, it could always be a future extension to re-introduce 
dispatcher-internal matching.

> 2) Need to enforce service core for its usage.
> 

Well, Eventdev does that already, except on systems where all required 
event adapters have the appropriate INTERNAL_PORT capability.

> IMO, Both are a given application's choice, All the application does
> not need to use this scheme. Keeping the code in lib/eventdev has the
> following issue.
> 
> 1)It is kind of enforcing above scheme for all the application
> modeling, which may not applicable for application use cases and
> eventdev device does not dictate a specific framework model.
> 2) The framework code, we never kept in device class library. i.e.,
> public APIs are implemented through device class API and public API
> don't have any no hook to PMD API.
> For example, we never kept lib/distributor/ code in lib/ethdev.
> 
> Other than the placement of this code, I agree with use case and
> solution at high level . The following could option for placement of
> this library. Based on that, we can have next level review.
> 

I'm fine with keeping this as a separate library, although I also don't 
see the harm in having some utility-type functionality in eventdev itself.

> 1) It is possible to plug in this to lib/graph by adding new graph
> model(@zhirun.yan@intel.com recently added
> RTE_GRAPH_MODEL_MCORE_DISPATCH)
> 
> Based on my understanding, That can translate to
> a)  Adding new graph model which allows to have it on graph walk
> (Graph walk is nothing but calling registered dispatcher routines)
> b) It is possible to add model specific APIs via
> rte_graph_model_model_name_xxxx()
> c) Graph library is not using match callback kind of scheme. Instead,
> nodes will process the packet and find its downstream node and enqueue
> to it and then graph_walk() calls the downstream node specific process
> function.
> With that, we can meet the original goal of business logic decoupling.
> However, Currently, nodes are not aware of what kind of graph model it
> is running, that could be one issue here as eventdev has more
> scheduling properties
> like schedule type etc., to overcome that issue, it may be possible to
> introduce nodes to graph model compatibility (where nodes can
> advertise the supported graph models)
> d) Currently we are planning to make graph API as stable, if we are
> taking this path, we need to hold
> https://patches.dpdk.org/project/dpdk/patch/20230810180515.113700-1-stephen@networkplumber.org/
> as
> we may need to update some public APIs.
> 
> 2) Have new library lib/event_dispatcher
> 
> 3) Move to example directory to showcase the framework
> 
> 4) Move to app/test-eventdev directory  to show the case of the framework.
> 
> 
> Thoughts?

I'm not sure I follow. Are you suggesting rte_graph could use 
rte_event_dispatcher, or that an application could use rte_graph to 
solve the same problem as rte_event_dispatcher solves?

I didn't review rte_graph in detail, but if it's anything like fd.io 
VPP's graph model, it's not a programming model that you switch to 
without significant application code impact.