[v4,04/29] graph: implement node debug routines
diff mbox series

Message ID 20200405085613.1336841-5-jerinj@marvell.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • graph: introduce graph subsystem
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Jerin Jacob Kollanukkaran April 5, 2020, 8:55 a.m. UTC
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

Andrzej Ostruszka April 6, 2020, 6:17 p.m. UTC | #1
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
Jerin Jacob April 7, 2020, 10:22 a.m. UTC | #2
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
Andrzej Ostruszka April 7, 2020, 11:50 a.m. UTC | #3
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
Jerin Jacob April 7, 2020, 12:09 p.m. UTC | #4
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
Andrzej Ostruszka April 7, 2020, 12:50 p.m. UTC | #5
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

Patch
diff mbox series

diff --git a/lib/librte_graph/Makefile b/lib/librte_graph/Makefile
index 933d0ee49..2a6d86933 100644
--- a/lib/librte_graph/Makefile
+++ b/lib/librte_graph/Makefile
@@ -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
diff --git a/lib/librte_graph/graph_debug.c b/lib/librte_graph/graph_debug.c
new file mode 100644
index 000000000..75238e7ca
--- /dev/null
+++ b/lib/librte_graph/graph_debug.c
@@ -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]);
+}
+
diff --git a/lib/librte_graph/graph_private.h b/lib/librte_graph/graph_private.h
index 7ed6d01b6..6db04cee7 100644
--- a/lib/librte_graph/graph_private.h
+++ b/lib/librte_graph/graph_private.h
@@ -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_ */
diff --git a/lib/librte_graph/meson.build b/lib/librte_graph/meson.build
index 5754ac23b..01512182f 100644
--- a/lib/librte_graph/meson.build
+++ b/lib/librte_graph/meson.build
@@ -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
 
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);
+	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)
 {
diff --git a/lib/librte_graph/rte_graph_version.map b/lib/librte_graph/rte_graph_version.map
index 412386356..f2c2139c5 100644
--- a/lib/librte_graph/rte_graph_version.map
+++ b/lib/librte_graph/rte_graph_version.map
@@ -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;