Hi Robin/David and all, We realized the feature arc patch series is difficult to understand as a new concept. Our objectives are following with feature arc changes
1. Allow reusability of standard DPDK nodes (defined in lib/nodes/*) with out-of-tree applications (like grout). Currently out-of-tree graph applications are duplicating standard nodes but not reusing the standard ones which are available. In the long term, we would like to mature standard DPDK nodes with flexibility of hooking them to out-of-tree application nodes. 2. Flexibility to enable/disable sub-graphs per interface based on the runtime configuration updates. Protocol sub-graphs can be selectively enabled for few (or all interfaces) at runtime 3. More than one sub-graphs/features can be enabled on an interface. So a packet has to follow a sequential ordering node path on worker cores. Packets may need to move from one sub-graph to another sub-graph per interface 4. Last but not least, an optimized implementation which does not (or minimally) stop worker cores for any control plane runtime updates. Any performance regression should also be avoided I am planning to create a draft presentation on feature arc which I can share, when ready, to discuss. If needed, I can also plan to present that in one of the DPDK community meetings. Their we can also discuss if there are any alternatives of achieving above objectives Thanks, Nitin . On Wed, Oct 16, 2024 at 7:20 PM Nitin Saxena <nsaxen...@gmail.com> wrote: > > 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 > >
