Thanks Jerin for the code review and feedback. I will make the suggested
changes in next version of the patch.
> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Thursday, January 12, 2023 5:48 PM
> To: Amit Prakash Shukla <amitpraka...@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Kiran Kumar
> Kokkilagadda <kirankum...@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpu...@marvell.com>; dev@dpdk.org
> Subject: [EXT] Re: [PATCH v3 2/3] graph: pcap capture for graph nodes
>
> External Email
>
> ----------------------------------------------------------------------
> On Thu, Jan 12, 2023 at 3:31 PM Amit Prakash Shukla
> <amitpraka...@marvell.com> wrote:
> >
> > Implementation adds support to capture packets at each node with
> > packet metadata and node name.
> >
> > Signed-off-by: Amit Prakash Shukla <amitpraka...@marvell.com>
> > ---
> > v2:
> > - Fixed code style issue
> > - Fixed CI compilation issue on github-robot
> >
> >
> > diff --git a/lib/graph/graph_pcap_trace.c
> > b/lib/graph/graph_pcap_trace.c new file mode 100644 index
> > 0000000000..f7b81a7ad1
> > --- /dev/null
> > +++ b/lib/graph/graph_pcap_trace.c
> > @@ -0,0 +1,155 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(C) 2022 Marvell International Ltd.
>
> 2023
>
> > +
> > +#define GRAPH_PCAP_BUF_SZ 128
> > +#define GRAPH_PCAP_NUM_PACKETS 1024
> > +#define GRAPH_PCAP_FILE_NAME_SZ 128
> > +#define GRAPH_PCAP_FILE_NAME "/tmp/graph_pcap_capture.pcapng"
>
> Expecting the filename from application. So no need to define here.
>
> > +
> > +static char file_name[GRAPH_PCAP_FILE_NAME_SZ];
> > +static uint32_t pkt_buf_sz = RTE_MBUF_DEFAULT_BUF_SIZE; static
> > +uint64_t packet_to_capture = GRAPH_PCAP_NUM_PACKETS; static
> > +rte_pcapng_t *pcapng_fd; static struct rte_mempool *mp; static
> > +uint16_t port_id; static uint64_t packet_captured[RTE_MAX_LCORE];
> > +static int pcap_trace_enable;
>
> See what it takes for multiprocess. If anything needs to be available in graph
> walk(which runs on secondary process) needs to be part of huge page. i.e
> part of rte_graph structure.
>
> >
> > + rte_node_process_t original_process; /**< Pcap enabled node
> > + callback */
>
> Add comment like, "Used to store the original process when pcap is enabled"
> or so
>
> > +
> > /* Fast path area */
> > #define RTE_NODE_CTX_SZ 16
> > uint8_t ctx[RTE_NODE_CTX_SZ] __rte_cache_aligned; /**< Node
> > Context. */ diff --git a/lib/graph/version.map b/lib/graph/version.map
> > index 13b838752d..36153a75b2 100644
> > --- a/lib/graph/version.map
> > +++ b/lib/graph/version.map
> > @@ -43,5 +43,12 @@ EXPERIMENTAL {
> > rte_node_next_stream_put;
> > rte_node_next_stream_move;
> >
> > + rte_pcap_trace_is_enable;
> > + rte_pcap_trace_enable;
> > + rte_graph_pcap_trace_init;
> > + rte_num_pkt_to_capture;
> > + rte_filename_to_capture_pkt;
> > + rte_graph_pcap_trace_exit;
>
> All of these public APIs are not needed. Since rte_graph is still experimental
> to make it clean, please add file name and number of packet to capture in
> struct rte_graph_param and update
> doc/guides/rel_notes/release_23_03.rst.
>
> See following example in doc/guides/rel_notes/release_22_07.rst
>
> API Changes
> -----------
>
> * Experimental structures ``struct rte_mtr_params``
> and ``struct rte_mtr_capabilities`` were updated to support
> protocol based input color for meter.
>
> > +
> > local: *;
> > };
> > --
> > 2.25.1
> >
