Re: [PATCH v3 1/3] lib: introduce dispatcher library

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

On 2023-09-19 12:58, Jerin Jacob wrote:
> On Mon, Sep 4, 2023 at 6:39 PM Mattias Rönnblom
> <mattias.ronnblom@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.ronnblom@ericsson.com>
>> Tested-by: Peter Nilsson <peter.j.nilsson@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).

Another option for a traditional app calling rte_eth_rx_burst() directly 
is to start using eventdev. :)

> 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

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

> 
>> 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 
implementation incur. 10 cc is not a lot though. Your eventdev will need 
something like 10x that.

With link-time optimizations, the matching function call will go away. 
I'm not sure how much auto-vectorization will happen. (The number quoted 
above is without LTO.)

The event dispatcher as a separate library will result in an extra 
function call across a shared object boundary, which is not exactly for 
free. (The 10 cc are for static linking.)

> 
> [1]
> https://developer.arm.com/documentation/den0018/a/NEON-and-VFP-Instruction-Summary/NEON-general-data-processing-instructions/VTBL
> 
>> +
>> +/**
>> + * Function prototype for process callbacks.
>> + *
>> + * The process callbacks are used by the dispatcher to deliver
>> + * events for processing.
>> + *
>> + * @param event_dev_id
>> + *  The originating event device id.
>> + *
>> + * @param event_port_id
>> + *  The originating event port.
>> + *
>> + * @param events
>> + *  Pointer to an array of events.
>> + *
>> + * @param num
>> + *  The number of events in the @p events array.
>> + *
>> + * @param cb_data
>> + *  The pointer supplied by the application in
>> + *  rte_dispatcher_register().
>> + */
>> +
>> +typedef void
>> +(*rte_dispatcher_process_t)(uint8_t event_dev_id, uint8_t event_port_id,
>> +                                 struct rte_event *events, uint16_t num,
>> +                                 void *cb_data);
> 
> Same as above comment, can event_port_id can be change to source_id?
> 
> 
>> +/**
>> + * Create a dispatcher with the specified id.
>> + *
>> + * @param id
>> + *  An application-specified, unique (across all dispatcher
>> + *  instances) identifier.
>> + *
>> + * @param event_dev_id
>> + *  The identifier of the event device from which this dispatcher
>> + *  will dequeue events.
>> + *
>> + * @return
>> + *   - 0: Success
>> + *   - <0: Error code on failure
>> + */
>> +__rte_experimental
>> +int
>> +rte_dispatcher_create(uint8_t id, uint8_t event_dev_id);
> 
> Following could be used to abstract more dispatcher sources, like
> 
> enum rte_dispatcher_source {
>           RTE_DISPATCHER_SOURCE_EVENTDEV, // Use rte_event_dequeue() to
> pull the packet
>           RTE_DISPATCHER_SOURCE_ETHDEV, // Use rte_ethdev_rx_burst() to
> pull the packet
> };
> 
> struct rte_dispatcher_params {
>              enum rte_dispatcher_source source;
>              union {
>                     /* Valid when source == RTE_DISPATCHER_SOURCE_EVENTDEV */
>                      struct event_source {
>                               uint8_t event_dev_id;
>                               uin8_t event_port_id;
>                      };
>                     /* Valid when source == RTE_DISPATCHER_SOURCE_ETHDEV*/
>                      struct ethdev_source {
>                               uint16_t ethdev__dev_id;
>                               uin16_t ethdev_rx_queue_id;
>                      };
>               }
> };
> 
> rte_dispatcher_create(uint8_t id,  struct rte_dispatcher_params *parms);
> 
> I will stop reviewing at this point. Will review based on direction agree on.

Thanks!