Hi Robin, Thanks for the review Please see my replies inline
Thanks, Nitin On Wed, Oct 16, 2024 at 3:08 PM Robin Jarry <rja...@redhat.com> wrote: > > Hi folks, > > David Marchand, Oct 16, 2024 at 11:24: > > On Mon, Oct 14, 2024 at 1:12 PM Nitin Saxena <nsax...@marvell.com> wrote: > >> I had pushed non RFC patch series before -rc1 date (11th oct). > >> We have an ABI change in this patch series > >> https://patches.dpdk.org/project/dpdk/patch/20241010133111.2764712-3-nsax...@marvell.com/ > >> Could you help merge this patch series in rc2 otherwise it has to wait for > >> next LTS > > > > Just read through the series, I am not confident with this addition. > > It requires a lot of changes in the node code for supporting it, where > > it should be something handled in/facilitated by the graph library > > itself. > > As far as I can tell, it will be very complicated (if not impossible) to > determine in a generic manner whether a packet must be steered towards > a sub tree or not. The decision *must* come from the originating node in > some way or another. Nitin> I am not sure if it *must* always be from the originating node? What about a control plane which wants to enable "IP4 feature" on interface 'X' by assigning IP address? A originating node (say: ip4-input) *must not* activate IP4 lookup sub-graph for interface "X " until control plane assigns any IP address to it. Regarding the complexity of adopting feature arc changes in fast path, - a sub-optimal change for feature-arc would be simple and trivial but at the cost of performance. - Complexity increases when feature arc changes are optimally integrated (like "ip4_rewrite" changes in the patch) with no performance regression > > > I did not read much from Robin or Christophe who have been writing > > more node code than me. > > I would prefer their opinion before going forward. > > This series is indeed very dense. I like the concept of having > extensible sub trees in the graph but it feels like the implementation > is more complex than it should be. > > Lacking of another solution, we went for a naive approach in grout. > Basically, some nodes have undefined next nodes which are extended using > a dedicated API. Nitin> With an initial glance, it looks like "grout" is trying to solve a use-case where a child is being added to the parent's undefined next node. This is trying to create a runtime parent-child relationship On the other hand, feature arc not just create parent-child relationships but also sibling-sibling relationships as well. Also enabling sub-graph per interface is critical functionality in feature arc that adds complexity Let's assume a use-case in ingress direction, at the IPv4 layer, where IPv4-input is the *originating node* and - On interface X, IPsec-policy, IP4-classify() and IPv4-lookup sub-graphs are enabled in a sequential order - On interface Y, IP4-classify() and IPv4-lookup sub-graphs are enabled. in a sequential order. i.e. IPsec-policy is *disabled* on interface Y In fast path, following processing should happen for "mbuf0" which is received on interface "X" - "ipv4-input" sends mbuf0 to the first enabled sub-graph node for interface X, "IPsec-policy" - In "IPsec-policy" node processing, if policy action results in "bypass" action for mbuf0, it must then be sent to next enabled sub-graph i.e. "IPv4-classify" (from "IPsec-policy" node) - In "IPv4-classify" node processing, if classify fails for mbuf0 then it should finally be sent to "IPv4-lookup" node (from "IPv4-classify" node) whereas for "mbuf1" received on interface Y following fast path processing must happen - "Ipv4-input" sends mbuf1 to the first enabled sub-graph node for interface Y, "IPv4-classify" - If "IPv4-classify" fails for mbuf1, then it should finally be sent to IPv4-lookup node To behave differently for interface X and interface Y as above - First of all, IPsec-policy/IPv4-classify/IPv4-lookup must be connected to "ipv4-input" node (Parent-Child relationship) - Also, IPsec-policy/IPv4-classify/IPv4-lookup must also be connected with each other (Sibling-Sibling relationship) - Fast path APIs provide *rte_edges_t* to send mbuf from one node to another node 1. Based on interface (either Interface X or Interface Y) 2. Based on which node, fast path APIs are called. Next enabled feature/sub-graph can only be determined from previous enabled feature/sub-graph in fast path Not sure if grout handles above use-cases in the same manner. AFAIR , for any control plane change grout re-creates "graph" objects which may not be required with feature arc. > > https://github.com/DPDK/grout/blob/v0.2/modules/infra/datapath/eth_input.c#L23-L31 > > This API can be used by other nodes to attach themselves to these > extensible nodes: > > https://github.com/DPDK/grout/blob/v0.2/modules/ip/datapath/arp_input.c#L143 > https://github.com/DPDK/grout/blob/v0.2/modules/ip/datapath/ip_input.c#L124 > https://github.com/DPDK/grout/blob/v0.2/modules/ip6/datapath/ip6_input.c#L122 > > After which, the extensible nodes can steer the packets towards the > correct downstream edge based on the dedicated classifier field: > > https://github.com/DPDK/grout/blob/v0.2/modules/infra/datapath/eth_input.c#L79 > > Obviously, this does not natively support a per-interface sub tree > traversal, but it can be done in the originating node based on packet > private context data. Nitin> Expressing per interface sub-tree traversal is the key functionality of feature arc. > > This raises a more important question: how can we standardize the way > private application data is passed from node to node? And how could we > enforce this declaratively in the node register API? Nitin> What you are suggesting here (node to node application data exchange) can be done in rte_node_register API but, IMO, this is not related to feature arc. Feature arc is not just between parent and child nodes but also between siblings (as explained above) > > Do you think we could find some middle ground that would not require > such extensive changes? Nitin> If you are pointing to ipv4-rewrite changes, I had an internal review comment of adding those changes without any *performance regression*. A sub-optimal code would be much simpler but at the cost of performance. > > Cheers, > Robin >
