Re: [dpdk-dev] [PATCH v5 11/14] net/i40e: display Flow Director packet

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

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.iremonger@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.