[RFC] graph/node: feedback and future improvements

[Robin Jarry]

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.