[v4,04/29] graph: implement node debug routines
Checks
Commit Message
From: Jerin Jacob <jerinj@marvell.com>
Adding node debug API implementation support to dump
single or all the node objects to the given file.
Signed-off-by: Jerin Jacob <jerinj@marvell.com>
Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---
lib/librte_graph/Makefile | 1 +
lib/librte_graph/graph_debug.c | 25 ++++++++++++++++++++
lib/librte_graph/graph_private.h | 12 ++++++++++
lib/librte_graph/meson.build | 2 +-
lib/librte_graph/node.c | 32 ++++++++++++++++++++++++++
lib/librte_graph/rte_graph_version.map | 1 +
6 files changed, 72 insertions(+), 1 deletion(-)
create mode 100644 lib/librte_graph/graph_debug.c
Comments
On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> From: Jerin Jacob <jerinj@marvell.com>
>
> Adding node debug API implementation support to dump
> single or all the node objects to the given file.
>
> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
[...]
> diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
> index d04a0fce0..8592c1221 100644
> --- a/lib/librte_graph/node.c
> +++ b/lib/librte_graph/node.c
> @@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
> return rc;
> }
>
> +static void
> +node_scan_dump(FILE *f, rte_node_t id, bool all)
> +{
> + struct node *node;
> +
> + RTE_ASSERT(f != NULL);
Why the assert? Below this is used in public (I guess) functions so
user can provide wrong input - in that case I'd expect warning/error not
an assert.
> + NODE_ID_CHECK(id);
> +
> + STAILQ_FOREACH(node, &node_list, next) {
> + if (all == true) {
> + node_dump(f, node);
> + } else if (node->id == id) {
> + node_dump(f, node);
> + return;
> + }
> + }
> +fail:
> + return;
> +}
> +
> +void
> +rte_node_dump(FILE *f, rte_node_t id)
> +{
> + node_scan_dump(f, id, false);
> +}
> +
> +void
> +rte_node_list_dump(FILE *f)
> +{
> + node_scan_dump(f, 0, true);
> +}
> +
[...]
With regards
Andrzej Ostruszka
On Mon, Apr 6, 2020 at 11:47 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>
> On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> > From: Jerin Jacob <jerinj@marvell.com>
> >
> > Adding node debug API implementation support to dump
> > single or all the node objects to the given file.
> >
> > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> [...]
> > diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
> > index d04a0fce0..8592c1221 100644
> > --- a/lib/librte_graph/node.c
> > +++ b/lib/librte_graph/node.c
> > @@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
> > return rc;
> > }
> >
> > +static void
> > +node_scan_dump(FILE *f, rte_node_t id, bool all)
> > +{
> > + struct node *node;
> > +
> > + RTE_ASSERT(f != NULL);
>
> Why the assert? Below this is used in public (I guess) functions so
> user can provide wrong input - in that case I'd expect warning/error not
> an assert.
Public API rte_node_dump() and node_scan_dump() calls this API without
any check.
>
> > + NODE_ID_CHECK(id);
> > +
> > + STAILQ_FOREACH(node, &node_list, next) {
> > + if (all == true) {
> > + node_dump(f, node);
> > + } else if (node->id == id) {
> > + node_dump(f, node);
> > + return;
> > + }
> > + }
> > +fail:
> > + return;
> > +}
> > +
> > +void
> > +rte_node_dump(FILE *f, rte_node_t id)
> > +{
> > + node_scan_dump(f, id, false);
> > +}
> > +
> > +void
> > +rte_node_list_dump(FILE *f)
> > +{
> > + node_scan_dump(f, 0, true);
> > +}
> > +
> [...]
>
> With regards
> Andrzej Ostruszka
On 4/7/20 12:22 PM, Jerin Jacob wrote:
> On Mon, Apr 6, 2020 at 11:47 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>>
>> On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
>>> From: Jerin Jacob <jerinj@marvell.com>
>>>
>>> Adding node debug API implementation support to dump
>>> single or all the node objects to the given file.
>>>
>>> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
>>> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
>> [...]
>>> diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
>>> index d04a0fce0..8592c1221 100644
>>> --- a/lib/librte_graph/node.c
>>> +++ b/lib/librte_graph/node.c
>>> @@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
>>> return rc;
>>> }
>>>
>>> +static void
>>> +node_scan_dump(FILE *f, rte_node_t id, bool all)
>>> +{
>>> + struct node *node;
>>> +
>>> + RTE_ASSERT(f != NULL);
>>
>> Why the assert? Below this is used in public (I guess) functions so
>> user can provide wrong input - in that case I'd expect warning/error not
>> an assert.
>
> Public API rte_node_dump() and node_scan_dump() calls this API without
> any check.
That was my point. I would expect either there or here to have a check
for arg instead of assert. I'd say that asserts are very good for
checking internal logic, but not so for checking if user input is OK.
But I'm fine if you ignore this.
With regards
Andrzej Ostruszka
On Tue, Apr 7, 2020 at 5:20 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>
> On 4/7/20 12:22 PM, Jerin Jacob wrote:
> > On Mon, Apr 6, 2020 at 11:47 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
> >>
> >> On 4/5/20 10:55 AM, jerinj@marvell.com wrote:
> >>> From: Jerin Jacob <jerinj@marvell.com>
> >>>
> >>> Adding node debug API implementation support to dump
> >>> single or all the node objects to the given file.
> >>>
> >>> Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> >>> Signed-off-by: Kiran Kumar K <kirankumark@marvell.com>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> >> [...]
> >>> diff --git a/lib/librte_graph/node.c b/lib/librte_graph/node.c
> >>> index d04a0fce0..8592c1221 100644
> >>> --- a/lib/librte_graph/node.c
> >>> +++ b/lib/librte_graph/node.c
> >>> @@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
> >>> return rc;
> >>> }
> >>>
> >>> +static void
> >>> +node_scan_dump(FILE *f, rte_node_t id, bool all)
> >>> +{
> >>> + struct node *node;
> >>> +
> >>> + RTE_ASSERT(f != NULL);
> >>
> >> Why the assert? Below this is used in public (I guess) functions so
> >> user can provide wrong input - in that case I'd expect warning/error not
> >> an assert.
> >
> > Public API rte_node_dump() and node_scan_dump() calls this API without
> > any check.
>
> That was my point. I would expect either there or here to have a check
> for arg instead of assert. I'd say that asserts are very good for
> checking internal logic, but not so for checking if user input is OK.
All DPDK _dump() functions returns void. I thought, We will keep the same here.
Another option is. if it is NULL we can return.
i.e
-RTE_ASSERT(f != NULL);
+if (f == NULL)
+ return
Either scheme is OK with me, Let me know your preference, I will
change accordingly.
>
> But I'm fine if you ignore this.
>
> With regards
> Andrzej Ostruszka
On 4/7/20 2:09 PM, Jerin Jacob wrote:
> On Tue, Apr 7, 2020 at 5:20 PM Andrzej Ostruszka <amo@semihalf.com> wrote:
>>
>> On 4/7/20 12:22 PM, Jerin Jacob wrote:
[...]
>>>>> +static void
>>>>> +node_scan_dump(FILE *f, rte_node_t id, bool all)
>>>>> +{
>>>>> + struct node *node;
>>>>> +
>>>>> + RTE_ASSERT(f != NULL);
>>>>
>>>> Why the assert? Below this is used in public (I guess) functions so
>>>> user can provide wrong input - in that case I'd expect warning/error not
>>>> an assert.
>>>
>>> Public API rte_node_dump() and node_scan_dump() calls this API without
>>> any check.
>>
>> That was my point. I would expect either there or here to have a check
>> for arg instead of assert. I'd say that asserts are very good for
>> checking internal logic, but not so for checking if user input is OK.
>
> All DPDK _dump() functions returns void. I thought, We will keep the same here.
> Another option is. if it is NULL we can return.
> i.e
> -RTE_ASSERT(f != NULL);
> +if (f == NULL)
> + return
>
> Either scheme is OK with me, Let me know your preference, I will
> change accordingly.
No, I don't want to silently skip that. Have an error message here and
return error but don't call rte_panic() which would abort application.
That would be my preference, but if you don't want to change return type
here (and where this is called) then I'm fine with what it is now.
With regards
Andrzej Ostruszka
@@ -16,6 +16,7 @@ EXPORT_MAP := rte_graph_version.map
# all source are stored in SRCS-y
SRCS-$(CONFIG_RTE_LIBRTE_GRAPH) += node.c
SRCS-$(CONFIG_RTE_LIBRTE_GRAPH) += graph.c
+SRCS-$(CONFIG_RTE_LIBRTE_GRAPH) += graph_debug.c
# install header files
SYMLINK-$(CONFIG_RTE_LIBRTE_GRAPH)-include += rte_graph.h
new file mode 100644
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(C) 2020 Marvell International Ltd.
+ */
+
+#include <rte_common.h>
+#include <rte_debug.h>
+
+#include "graph_private.h"
+
+void
+node_dump(FILE *f, struct node *n)
+{
+ rte_edge_t i;
+
+ fprintf(f, "node <%s>\n", n->name);
+ fprintf(f, " id=%" PRIu32 "\n", n->id);
+ fprintf(f, " flags=0x%" PRIx64 "\n", n->flags);
+ fprintf(f, " addr=%p\n", n);
+ fprintf(f, " process=%p\n", n->process);
+ fprintf(f, " nb_edges=%d\n", n->nb_edges);
+
+ for (i = 0; i < n->nb_edges; i++)
+ fprintf(f, " edge[%d] <%s>\n", i, n->next_nodes[i]);
+}
+
@@ -82,4 +82,16 @@ void graph_spinlock_lock(void);
*/
void graph_spinlock_unlock(void);
+/**
+ * @internal
+ *
+ * Dump internal node object data.
+ *
+ * @param f
+ * FILE pointer to dump the info.
+ * @param g
+ * Pointer to the internal node object.
+ */
+void node_dump(FILE *f, struct node *n);
+
#endif /* _RTE_GRAPH_PRIVATE_H_ */
@@ -4,7 +4,7 @@
name = 'graph'
-sources = files('node.c', 'graph.c')
+sources = files('node.c', 'graph.c', 'graph_debug.c')
headers = files('rte_graph.h')
allow_experimental_apis = true
@@ -377,6 +377,38 @@ rte_node_edge_get(rte_node_t id, char *next_nodes[])
return rc;
}
+static void
+node_scan_dump(FILE *f, rte_node_t id, bool all)
+{
+ struct node *node;
+
+ RTE_ASSERT(f != NULL);
+ NODE_ID_CHECK(id);
+
+ STAILQ_FOREACH(node, &node_list, next) {
+ if (all == true) {
+ node_dump(f, node);
+ } else if (node->id == id) {
+ node_dump(f, node);
+ return;
+ }
+ }
+fail:
+ return;
+}
+
+void
+rte_node_dump(FILE *f, rte_node_t id)
+{
+ node_scan_dump(f, id, false);
+}
+
+void
+rte_node_list_dump(FILE *f)
+{
+ node_scan_dump(f, 0, true);
+}
+
rte_node_t
rte_node_max_count(void)
{
@@ -4,6 +4,7 @@ EXPERIMENTAL {
__rte_node_register;
rte_node_clone;
+ rte_node_dump;
rte_node_edge_count;
rte_node_edge_get;
rte_node_edge_shrink;