Hi Jerin,
Sorry for the delayed reply. Thanks a lot for your comments! By the way,
I completely forgot to say that the rte_graph design you created is
awesome. It makes it a breeze to write a clean data path implementation.
Kudos!
Please see my remarks inline.
Jerin Jacob, Apr 06, 2024 at 01:11:
Great.
You may consider improving and/or adding inbuilt nodes for generic
protocol processing. Furthermore, consider contributing on app/graph.
I think, most likely, you should be able to leverage app/graph.
Thanks! I am definitely planning to upstream nodes into DPDK as much as
possible. I still need to determine what is the correct level of data
path node public API so that the nodes can be agnostic of the control
plane implementation.
> only one of each ethdev_rx and ethdev_tx nodes per graph? For
> simplicity and to make dynamic rxq changes possible, I chose to have
> a single rx & tx node per graph. Do you think we could change the
> in-built nodes to support both modes ?
In terms of performance, the current scheme will be more performant.
I would suggest, we can add another inbuilt node for this. This is to
avoid additional checks in fast path to enable dynamic behavior.
Probably need to use rcu as control thread updates port/queue
configuration changes and fast path needs to adapt to it.
Ack, I'll think about adding a variant of the ethdev_rx/tx nodes.
> There is no way to prepare node data context when calling
> rte_graph_create(). The current implementation uses global variables
> [4] but this makes it very "static".
Since the those are node and it is private. I think it is OK.
Also using, rte_graph_node_get_*() one can get the node and its ctx at
anytime.
I had not thought about accessing the nodes private data outside of fast
path. This would simplify nodes data update by a huge margin.
I think, rte_graph_create() will be complicated, e.s.p it supports
loading nodes with regex pattern. I think, we can weigh in pros and
cons if you have patch.
I agree that it would make that function very complex. Probably this is
not required as you said before.
> Pluggable nodes
> ===============
>
> Currently, the declaration of next nodes is static. In order to
> support plugins (e.g. via dlopen() or similar), could we introduce
> a way to dynamically insert a node in the graph?
>
> I have done this using a post-init callback system but we could
> think of a different way.
>
> Also, could we allow overriding nodes with RTE_NODE_REGISTER()? So
> users can replace the default implementation with their own if they
> need to.
I think, if we document the inbuilt node's downstream node ID. After
rte_graph_create(), one can use rte_node_edge_update() to dynamically
add custom/user defined nodes in between.
I thought of adding more helper functions (leveraging existing
rte_node_edge_update() for this) It will be more like "feature arch"
in VPP. Provided, node cannot add dynamically after
rte_graph_create(), but one changes the downstream node connection
dynamically.
After I get something satisfactory, I will send an RFC patch with some
helper functions to allow flexibility when registering nodes.
Yes. I think, we can use mbuf data offset or new dynamic mbuf filed
for this.
Ack, I'll send a patch.
Cheers.
