diff mbox series

[v5,11/14] net/i40e: display Flow Director packet

Message ID 1579010128-15794-12-git-send-email-bernard.iremonger@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/i40e: ESP support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Iremonger, Bernard Jan. 14, 2020, 1:55 p.m. UTC 
  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(-)

Comments

Ferruh Yigit Jan. 14, 2020, 6:52 p.m. UTC | #1 
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? If so instead of adding new config
option, can we add dynamic debugging for it?
Qi Zhang Jan. 15, 2020, 12:20 a.m. UTC | #2 
> -----Original Message-----
> From: Iremonger, Bernard <bernard.iremonger@intel.com>
> Sent: Tuesday, January 14, 2020 9:55 PM
> To: 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>;
> Iremonger, Bernard <bernard.iremonger@intel.com>
> Subject: [PATCH v5 11/14] net/i40e: display Flow Director packet
> 
> 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

Seems RTE_LIBRTE_I40E_DEBUG_FD need to be defined in config/common_base
Like RTE_LIBRTE_I40E_DEBUG_RX

> +#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;
>  }
> 
> --
> 2.7.4
Qi Zhang Jan. 15, 2020, 1:32 a.m. UTC | #3 
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Wednesday, January 15, 2020 8:20 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org; Xing,
> Beilei <beilei.xing@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: [PATCH v5 11/14] net/i40e: display Flow Director packet
> 
> 
> 
> > -----Original Message-----
> > From: Iremonger, Bernard <bernard.iremonger@intel.com>
> > Sent: Tuesday, January 14, 2020 9:55 PM
> > To: 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>; Iremonger, Bernard
> > <bernard.iremonger@intel.com>
> > Subject: [PATCH v5 11/14] net/i40e: display Flow Director packet
> >
> > 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
> 
> Seems RTE_LIBRTE_I40E_DEBUG_FD need to be defined in
> config/common_base Like RTE_LIBRTE_I40E_DEBUG_RX

Sorry, didn't see its already in patch 10/14
So
Acked-by: Qi Zhang <qi.z.zhang@intel.com>
Iremonger, Bernard Jan. 15, 2020, 9:18 a.m. UTC | #4 
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?

Regards,

Bernard
Iremonger, Bernard Jan. 15, 2020, 9:25 a.m. UTC | #5 
Hi Qi,

<snip>

> > Subject: [PATCH v5 11/14] net/i40e: display Flow Director packet
> >
> > 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
> 
> Seems RTE_LIBRTE_I40E_DEBUG_FD need to be defined in
> config/common_base Like RTE_LIBRTE_I40E_DEBUG_RX

It is defined in config/common_base in previous patch 
[v5,10/14] config: add debug to I40E Flow Director

Maybe patches 10 and 11 should be squashed

> 
> > +#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;
> >  }
> >
> > --
> > 2.7.4
> 
Regards,

Bernard.
Ferruh Yigit Jan. 15, 2020, 10:58 a.m. UTC | #6 
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?
Iremonger, Bernard Jan. 15, 2020, 3:08 p.m. UTC | #7 
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.
diff mbox series

Patch

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;
 }