Re: [dpdk-dev] [PATCH] port: add file descriptor SWX port

["P, Venkata Suresh Kumar" <venkata.suresh.kumar.p@intel.com>]

Thanks a lot for reviewing the code and providing your comments.

I have addressed below comments in V2 patch.

Thanks & Regards,
Suresh.

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Tuesday, March 23, 2021 9:52 PM
To: P, Venkata Suresh Kumar <venkata.suresh.kumar.p@intel.com>
Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Khangar, Churchill <churchill.khangar@intel.com>; Jangra, Yogesh <yogesh.jangra@intel.com>
Subject: Re: [dpdk-dev] [PATCH] port: add file descriptor SWX port

19/03/2021 13:02, Venkata Suresh Kumar P:
> Add the file descriptor input/output port type for the SWX pipeline.

I think it deserves a bit more explanation about what is FD I/O port. -- [Suresh] - Addressed in V2 patch

>  /*
> + * tap
> + */
> +#define TAP_DEV                                            "/dev/net/tun"

Spaces are free :) -- [Suresh] - Addressed in V2 patch


> +#ifndef TRACE_LEVEL
> +#define TRACE_LEVEL 0
> +#endif
> +
> +#if TRACE_LEVEL
> +#define TRACE(...) printf(__VA_ARGS__) #else #define TRACE(...) 
> +#endif

Would you consider rte_trace?

> --- /dev/null
> +++ b/lib/librte_port/rte_swx_port_fd.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016 Intel Corporation

I guess you did not create it in 2016. -- [Suresh] - Addressed in V2 patch

> + */
> +
> +#ifndef __INCLUDE_RTE_SWX_PORT_FD_H__ #define 
> +__INCLUDE_RTE_SWX_PORT_FD_H__
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/**
> + * @file
> + * RTE SWX FD Input and Output Ports
> + *
> + ***/

Useless blank line. -- [Suresh] - Addressed in V2 patch

[...]
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif

A comment after such a far #endif is better: -- [Suresh] - Addressed in V2 patch
	/* __INCLUDE_RTE_SWX_PORT_FD_H__ */

> --- a/lib/librte_port/version.map
> +++ b/lib/librte_port/version.map
> @@ -48,4 +48,6 @@ EXPERIMENTAL {
>         #added in 21.02

In 21.05

>         rte_swx_port_ring_reader_ops;
>         rte_swx_port_ring_writer_ops;
> +       rte_swx_port_fd_reader_ops;
> +       rte_swx_port_fd_writer_ops;

Please sort in alphabetical order. -- [Suresh] - Addressed in V2 patch