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

[Jerin Jacob <jerinjacobk@gmail.com>]

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.

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.

High level cosmetic comment
----------------------------------------------------
1)Missing doxygen connection- See doc/api/doxy-api-index.md

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.



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


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


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