Re: [RFC] graph/node: feedback and future improvements

[Jerin Jacob <jerinjacobk@gmail.com>]

On Mon, Mar 25, 2024 at 7:56 PM Robin Jarry <rjarry@redhat.com> wrote:
>
> Hi Jerin, all,
>
> I apologize in advance for the long email :)
> I am working on a project [1] that uses rte_graph extensively. In the

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.

> course of action, I have stumbled upon a few issues. I managed to work
> around them for now, but I'd like to get more insights about long term
> solutions.
>
> Per Rx/Tx queue nodes
> =====================
>
> In the in-built nodes and in the examples, there is one ethdev_rx and
> ethdev_tx node per rx/tx queue [2].
>
> Is there a technical reason for this design? Does it make sense to have

The default Rx/Tx nodes added to support l3fwd-graph example application.

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

>
> Having multiple instances of the same node in a graph complicates
> instantiation as it requires cloning the nodes with unique names. Also,
> it makes dynamic configuration of ports even more complicated without
> shutting down everything first since some nodes will be part of an
> active graph and there may be conflicts.
>
> Speaking of graph reloading, I found that the in-built ethdev_tx TX
> queue id is initialized to graph_id [3]. This seems odd. If there are
> multiple rounds of graph create/destroy, the id will become invalid.
>
> Dynamic graph and nodes construction/destruction
> ================================================
>
> I need to deal with reconfiguration of the graphs at runtime. This can
> happen on multiple occasions: port addition/suppression, number of rxq
> change, rxq size change, etc.
>
> I could not manage to reuse the in-built nodes because of the issues
> raised in the previous point.

Having a new inbuilt node as mentioned above will fix this issue.

>
> Could we change the in-built nodes to better support dynamic reloading?
> Maybe this only applies to nodes that may appear multiple times in the
> same graph (rx/tx).
>
> Node context data
> =================
>
> 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.


>
> It would be nice to pass prepared context data for every node on graph
> creation, either via a config parameter (better) or via another

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.


> mechanism. I currently do this via a hash map but it requires a global
> hash as well which may not be the best solution.
>
> I tried patching the graph library code myself but after struggling,
> I thought it would be best to discuss things first.
>
> 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.


>
> Move data pointer of packets?
> =============================
>
> This final point is more a global design question. In traditional
> networking stacks, each layer of the stack receives pbuffers that are
> "adjusted" to their respective network header in the packet (i.e. the IP
> lookup function will receive a buffer that points to the beginning of
> the IP header, regardless of what was the transport protocol, plain
> ethernet, Ethernet + VLAN, IP in IP, etc.).
>
> Would it make sense to have a similar mechanism when designing an
> application with rte_graph?

Yes. I think, we can use mbuf data offset or new dynamic mbuf filed for this.

>
> Thanks in advance for your replies.
>
> Cheers!
>
> --
> Robin
>
> [1] https://github.com/rjarry/brouter
> [2] https://github.com/DPDK/dpdk/blob/v23.11/lib/node/ethdev_rx.c#L28
> [3] https://github.com/DPDK/dpdk/blob/v23.11/lib/node/ethdev_tx.c#L56
> [4] https://github.com/DPDK/dpdk/blob/v23.11/lib/node/ethdev_ctrl.c
>