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

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, March 23, 2021 6:39 PM
> To: P, Venkata Suresh Kumar <venkata.suresh.kumar.p@intel.com>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Khangar, Churchill <churchill.khangar@intel.com>; Jangra,
> Yogesh <yogesh.jangra@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] port: add file descriptor SWX port
> 
> 23/03/2021 19:07, P, Venkata Suresh Kumar:
> > Thanks a lot for reviewing the code and providing your comments.
> >
> > I have addressed below comments in V2 patch.
> 
> OK thanks.
> 
> What about the question about rte_trace?
> Opinions?
> 

Hi Thomas,

All rte_swx_port ports are currently following this pattern, so it makes sense to have this one do the same for now.

I am not that familiar with the (relatively new) rte_trace mechanism, so I am not sure if it has any run-time performance (I am assuming that it doesn't). We will take the AR to take a look at rte_trace and come back with a patch to convert traces for all ports to rte_trace, most likely in the 21.05 time frame. Is this OK for you?

Regards,
Cristian

> 
> 
> > From: Thomas Monjalon <thomas@monjalon.net>
> > 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
> 
> 
>