RE: [EXT] Re: [PATCH v3 2/3] graph: pcap capture for graph nodes

[Amit Prakash Shukla <amitprakashs@marvell.com>]

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 <jerinjacobk@gmail.com>
> Sent: Thursday, January 12, 2023 5:48 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Kiran Kumar
> Kokkilagadda <kirankumark@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@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
> <amitprakashs@marvell.com> wrote:
> >
> > Implementation adds support to capture packets at each node with
> > packet metadata and node name.
> >
> > Signed-off-by: Amit Prakash Shukla <amitprakashs@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
> >