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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/15/2020 9:18 AM, Iremonger, Bernard wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Tuesday, January 14, 2020 6:53 PM
>> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
>> Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>> Doherty, Declan <declan.doherty@intel.com>
>> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Byrne, Stephen1
>> <stephen1.byrne@intel.com>; Zhang, Helin <helin.zhang@intel.com>
>> 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?