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