Hi Ferruh <snip>
> >> Subject: Re: [dpdk-dev] [PATCH v5 11/14] net/i40e: display Flow > >> Director packet > >> > >> On 1/14/2020 1:55 PM, Bernard Iremonger wrote: > >>> include rte_config.h in i40e_fdir.c > >>> In debug mode call rte_hexdump in i40e_flow_fdir_construct_pkt() and > >>> in i40e_fdir_construct_pkt() > >>> > >>> Signed-off-by: Bernard Iremonger <bernard.iremon...@intel.com> > >>> --- > >>> drivers/net/i40e/i40e_fdir.c | 12 ++++++++++-- > >>> 1 file changed, 10 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/i40e/i40e_fdir.c > >>> b/drivers/net/i40e/i40e_fdir.c index 5f85703..67bb28c 100644 > >>> --- a/drivers/net/i40e/i40e_fdir.c > >>> +++ b/drivers/net/i40e/i40e_fdir.c > >>> @@ -21,6 +21,10 @@ > >>> #include <rte_tcp.h> > >>> #include <rte_sctp.h> > >>> #include <rte_hash_crc.h> > >>> +#include <rte_config.h> > >>> +#ifdef RTE_LIBRTE_I40E_DEBUG_FD > >>> +#include <rte_hexdump.h> > >>> +#endif > >>> > >>> #include "i40e_logs.h" > >>> #include "base/i40e_type.h" > >>> @@ -954,7 +958,9 @@ i40e_fdir_construct_pkt(struct i40e_pf *pf, > >>> &fdir_input->flow_ext.flexbytes[dst], > >>> size * sizeof(uint16_t)); > >>> } > >>> - > >>> +#ifdef RTE_LIBRTE_I40E_DEBUG_FD > >>> +rte_hexdump(stdout, NULL, raw_pkt, len); #endif > >>> return 0; > >>> } > >>> > >>> @@ -1415,7 +1421,9 @@ i40e_flow_fdir_construct_pkt(struct i40e_pf > *pf, > >>> &fdir_input->flow_ext.flexbytes[dst], > >>> size * sizeof(uint16_t)); > >>> } > >>> - > >>> +#ifdef RTE_LIBRTE_I40E_DEBUG_FD > >>> +rte_hexdump(stdout, NULL, raw_pkt, len); #endif > >>> return 0; > >>> } > >>> > >>> > >> > >> Hi Bernard, > >> > >> These are not data path functions, right? > > This code is only used when adding flow rules, so not in data path. > > > > If so instead of adding new config option, can we add dynamic debugging > for it? > > Adding new config option seems ok to me, why change to dynamic > debugging? > > > > Compile time options are OK for developer but bad for deployment, when > you are deploying your product you have to disable this config option, and on > the target platform there if a problem occurs there won't be any way to > enable the debug to see what is happening. > So it is useful only on the machine that you are both compiling and running > application, even that case it is a trouble to terminate application, > re-compile > it and run again, most probably same loop again to turn off the debug back. > > Also it is bad for testing, if you want to do the all code path, someone needs > to test both enabling and disabling this config option. If not, it is > possible by > time enabling this config option even won't compile and nobody will detect > it. > > Since we already have dynamic log support, why not add a new logtype, lets > say "pmd.net.i40e.fd" and have ability to enable/disable it without > terminating app? > This patch has already been acked by Qi Zhang, the I40e PMD maintainer. The logtype for example "pmd.net.i40e.fd" also require CONFIG_RTE_LIBRTE_I40E_DEBUG_RX=y in config/common_base. Given that today is the merge deadline, I would like to leave this patch as is in the v6 patchset. Regards, Bernard.
