Re: [EXTERNAL] Re: [RFC PATCH 0/3] add feature arc in rte_graph

[Nitin Saxena <nsaxena16@gmail.com>]

Hi Robin,

Thanks for the review
Please see my replies inline

Thanks,
Nitin

On Wed, Oct 16, 2024 at 3:08 PM Robin Jarry <rjarry@redhat.com> wrote:
>
> Hi folks,
>
> David Marchand, Oct 16, 2024 at 11:24:
> > On Mon, Oct 14, 2024 at 1:12 PM Nitin Saxena <nsaxena@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-nsaxena@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
>