RE: [EXTERNAL] Re: [PATCH v3 0/5] add feature arc in rte_graph

[Nitin Saxena <nsaxena@marvell.com>]

Hi Christophe,

Thanks for the review. See my comments inline

Thanks,
Nitin

> -----Original Message-----
> From: Christophe Fontaine <cfontain@redhat.com>
> Sent: Wednesday, October 9, 2024 7:51 PM
> To: Nitin Saxena <nsaxena@marvell.com>
> Cc: Jerin Jacob <jerinj@marvell.com>; Kiran Kumar Kokkilagadda
> <kirankumark@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Zhirun Yan <yanzhirun_163@163.com>; Robin
> Jarry <rjarry@redhat.com>; dev@dpdk.org; Nitin Saxena
> <nsaxena16@gmail.com>
> Subject: [EXTERNAL] Re: [PATCH v3 0/5] add feature arc in rte_graph
> 
> > On 9 Oct 2024, at 15:29, Nitin Saxena <nsaxena@marvell.com> wrote:
> >
> > Feature arc represents an ordered list of features/protocols at a
> > given networking layer. It is a high level abstraction to connect
> > various rte_graph nodes, as feature nodes, and allow packets steering
> > across these nodes in a generic manner.
> >
> > Features (or feature nodes) are nodes which handles partial or
> > complete handling of a protocol in fast path. Like ipv4-rewrite node,
> > which adds rewrite data to an outgoing IPv4 packet.
> >
> > However in above example, outgoing interface(say "eth0") may have
> > outbound IPsec policy enabled, hence packets must be steered from
> > ipv4-rewrite node to ipsec-outbound-policy node for outbound IPsec
> > policy lookup. On the other hand, packets routed to another interface
> > (eth1) will not be sent to ipsec-outbound-policy node as IPsec feature
> > is disabled on eth1. Feature-arc allows rte_graph applications to
> > manage such constraints easily
> >
> > Feature arc abstraction allows rte_graph based application to
> >
> > 1. Seamlessly steer packets across feature nodes based on whether
> > feature is enabled or disabled on an interface. Features enabled on
> > one interface may not be enabled on another interface with in a same
> > feature arc.
> >
> > 2. Allow enabling/disabling of features on an interface at runtime, so
> > that if a feature is disabled, packets associated with that interface
> > won't be steered to corresponding feature node.
> >
> > 3. Provides mechanism to hook custom/user-defined nodes to a feature
> > node and allow packet steering from feature node to custom node
> > without changing former's fast path function
> 
> Hi,
> 
> As a general comment, I would have expected a modification on
> rte_graph_walk so the nodes would *not* need to be aware of the enabled
> feature arcs.

If your point is that nodes has to adopt rte_feature_xxx() APIs in its "process_func()" to take advantage of feature arc, then your assessment is true.
So it is true that each feature node knows on which *arc* it sits on. However *unaware* part comes from the fact that once a node is integrated with feature_arc APIs, then the node does not need to know what is the next node to which current mbuf needs to be sent. So unaware functionality comes after integrating feature arc APIs. 

Regarding your rte_graph_walk() comment: IMO, decision of determining next_edge ( or next node) lies with each *node* and not with rte_graph_walk(). Feature arc is extending the functionality of determining next_edge to control plane as well (as control plane enables/disable features). This way without changing feature node code, control plane is able to steer packets to different nodes. This is the major functionality feature arc is trying to simplify

> Yet, in the series, the nodes need to be aware if a feature arc is enabled or
> not.
> Isn’t this a contradiction or did I miss something ?
> 
> Christophe
> 
> >
> > 4. Allow expressing features in a particular sequential order so that
> > packets are steered in an ordered way across nodes in fast path. For
> > eg: if IPsec and IPv4 features are enabled on an ingress interface,
> > packets must be sent to IPsec inbound policy node first and then to
> > ipv4 lookup node.
> >
> > This patch series adds feature arc library in rte_graph and also adds
> > "ipv4-output" feature arc handling in "ipv4-rewrite" node.
> >
> > Changes in v3:
> > - rte_graph_feature_arc_t typedef from uint64_t to uintptr_t to fix
> > compilation on 32-bit machine
> > - Updated images in .png format
> > - Added ABI change section in release notes
> > - Fixed DPDK CI failures
> >
> > Changes in v2:
> > - Added unit tests for feature arc
> > - Fixed issues found in testing
> > - Added new public APIs rte_graph_feature_arc_feature_to_node(),
> >  rte_graph_feature_arc_feature_to_name(),
> >  rte_graph_feature_arc_num_features()
> > - Added programming guide for feature arc
> > - Added release notes for feature arc
> >
> > Nitin Saxena (5):
> >  graph: add feature arc support
> >  graph: add feature arc option in graph create
> >  graph: add IPv4 output feature arc
> >  test/graph_feature_arc: add functional tests
> >  docs: add programming guide for feature arc
> >
> > app/test/meson.build                        |    1 +
> > app/test/test_graph_feature_arc.c           | 1410 +++++++++++++++++++
> > doc/guides/prog_guide/graph_lib.rst         |  288 ++++
> > doc/guides/prog_guide/img/feature_arc-1.png |  Bin 0 -> 61532 bytes
> > doc/guides/prog_guide/img/feature_arc-2.png |  Bin 0 -> 155806 bytes
> > doc/guides/prog_guide/img/feature_arc-3.png |  Bin 0 -> 143697 bytes
> > doc/guides/rel_notes/release_24_11.rst      |   13 +
> > lib/graph/graph.c                           |    1 +
> > lib/graph/graph_feature_arc.c               | 1223 ++++++++++++++++
> > lib/graph/graph_populate.c                  |    7 +-
> > lib/graph/graph_private.h                   |    3 +
> > lib/graph/meson.build                       |    2 +
> > lib/graph/node.c                            |    2 +
> > lib/graph/rte_graph.h                       |    3 +
> > lib/graph/rte_graph_feature_arc.h           |  431 ++++++
> > lib/graph/rte_graph_feature_arc_worker.h    |  674 +++++++++
> > lib/graph/version.map                       |   20 +
> > lib/node/ip4_rewrite.c                      |  476 +++++--
> > lib/node/ip4_rewrite_priv.h                 |   15 +-
> > lib/node/node_private.h                     |   20 +-
> > lib/node/rte_node_ip4_api.h                 |    3 +
> > 21 files changed, 4494 insertions(+), 98 deletions(-) create mode
> > 100644 app/test/test_graph_feature_arc.c create mode 100644
> > doc/guides/prog_guide/img/feature_arc-1.png
> > create mode 100644 doc/guides/prog_guide/img/feature_arc-2.png
> > create mode 100644 doc/guides/prog_guide/img/feature_arc-3.png
> > create mode 100644 lib/graph/graph_feature_arc.c create mode 100644
> > lib/graph/rte_graph_feature_arc.h create mode 100644
> > lib/graph/rte_graph_feature_arc_worker.h
> >
> > --
> > 2.43.0
> >