From: "Robin Jarry" <rjarry@redhat.com>
To: "Jerin Jacob" <jerinjacobk@gmail.com>
Cc: <dev@dpdk.org>, "Jerin Jacob" <jerinj@marvell.com>,
"Kiran Kumar K" <kirankumark@marvell.com>,
"Nithin Dabilpuram" <ndabilpuram@marvell.com>,
"Pavan Nikhilesh" <pbhagavatula@marvell.com>,
"Hongbo Zheng" <zhenghongbo3@huawei.com>,
"Zhirun Yan" <zhirun.yan@intel.com>,
"Thomas Monjalon" <thomas@monjalon.net>
Subject: Re: [RFC] graph/node: feedback and future improvements
Date: Sun, 14 Apr 2024 12:35:49 +0200 [thread overview]
Message-ID: <D0JS25NFTAQS.263ITZNU6Y7B3@redhat.com> (raw)
In-Reply-To: <CALBAE1MgbOpk+EH8Z5mms=5qizbPe1=EY9ssi-A2AiaTk-CWuw@mail.gmail.com>
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.
next prev parent reply other threads:[~2024-04-14 10:35 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 14:17 Robin Jarry
2024-04-05 23:11 ` Jerin Jacob
2024-04-14 10:35 ` Robin Jarry [this message]
2024-04-24 13:24 ` Robin Jarry
2024-05-04 10:03 ` Jerin Jacob
2024-05-08 22:04 ` Robin Jarry
2024-05-09 8:24 ` Jerin Jacob
2024-05-15 8:10 ` Robin Jarry
2024-05-15 9:02 ` Bruce Richardson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D0JS25NFTAQS.263ITZNU6Y7B3@redhat.com \
--to=rjarry@redhat.com \
--cc=dev@dpdk.org \
--cc=jerinj@marvell.com \
--cc=jerinjacobk@gmail.com \
--cc=kirankumark@marvell.com \
--cc=ndabilpuram@marvell.com \
--cc=pbhagavatula@marvell.com \
--cc=thomas@monjalon.net \
--cc=zhenghongbo3@huawei.com \
--cc=zhirun.yan@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).