diff mbox series

[v2,6/9] app/procinfo: add code for debug crypto

Message ID 20181024064805.23197-6-vipin.varghese@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers
Series [v2,1/9] app/procinfo: add usage for new debug |

Checks

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

Commit Message

Varghese, Vipin Oct. 24, 2018, 6:48 a.m. UTC 
  Function debug_crypto is used for displaying the crypto PMD under
the primary process.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
 app/proc-info/main.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

Comments

Pattan, Reshma Oct. 26, 2018, 1:01 p.m. UTC | #1 
Hi

-----Original Message-----
From: Varghese, Vipin 

---
+		struct rte_cryptodev_info dev_info = {0};
+		struct rte_cryptodev_stats stats = {0};
+

Memset for initialization as mentioned in other patch.

+
+#define DSP_CRYPTO_FLAG(x) do { \
+printf("  - feature flags\n"); \
+printf("\t  -- symmetric (%c) asymmetric (%c)" \ " symmetric operation 
+chaining (%c)\n", \ (x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)?'y':'n', \ 
+(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO)?'y':'n', \ (x & 
+RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING)?'y':'n'); \ printf("\t  -- CPU 
+SSE (%c) AVX (%c) AVX2 (%c) AVX512 (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_CPU_SSE)?'y':'n', \ (x & 
+RTE_CRYPTODEV_FF_CPU_AVX)?'y':'n', \ (x & 
+RTE_CRYPTODEV_FF_CPU_AVX2)?'y':'n', \ (x & 
+RTE_CRYPTODEV_FF_CPU_AVX512)?'y':'n'); \ printf("\t  -- Acclerate CPU 
+AESNI (%c) HW (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_AESNI)?'y':'n', \ (x 
+& RTE_CRYPTODEV_FF_HW_ACCELERATED)?'y':'n'); \ printf("\t  -- INLINE 
+(%c)\n", \ (x & RTE_CRYPTODEV_FF_SECURITY)?'y':'n'); \ printf("\t  -- 
+ARM NEON (%c) CE (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_NEON)?'y':'n', \ 
+(x & RTE_CRYPTODEV_FF_CPU_ARM_CE)?'y':'n'); \ printf("  - buffer 
+offload\n"); \ printf("\t  -- IN_PLACE_SGL (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_IN_PLACE_SGL)?'y':'n'); \ printf("\t  -- 
+OOP_SGL_IN_SGL_OUT (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT)?'y':'n'); \ printf("\t  -- 
+OOP_SGL_IN_LB_OUT (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT)?'y':'n'); \ printf("\t  -- 
+OOP_LB_IN_SGL_OUT (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT)?'y':'n'); \ printf("\t  -- 
+OOP_LB_IN_LB_OUT (%c)\n", \ (x & 
+RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT)?'y':'n'); \ } while (0)

This is a very big macro, better have static function for this instead of macro.

 Thanks,
Reshma
Pattan, Reshma Oct. 26, 2018, 1:16 p.m. UTC | #2 
Hi,

-----Original Message-----
From: Varghese, Vipin 

+		       "\t  -- id (%u) flags (0x%"PRIx64") socket (%d)\n"
+		       "\t  -- queue pairs (%d)\n",
+		       rte_cryptodev_name_get(i),
+		       dev_info.driver_name, dev_info.driver_id,
+		       dev_info.feature_flags, dev_info.device->numa_node,
+		       rte_cryptodev_queue_pair_count(i));

Above 6lines have spaces+ tabs mixed, should use only tabs.

Thanks,
Reshma
Varghese, Vipin Oct. 27, 2018, 4:37 a.m. UTC | #3 
Hi Reshma,

<snipped>

> 
> +		       "\t  -- id (%u) flags (0x%"PRIx64") socket (%d)\n"
> +		       "\t  -- queue pairs (%d)\n",
> +		       rte_cryptodev_name_get(i),
> +		       dev_info.driver_name, dev_info.driver_id,
> +		       dev_info.feature_flags, dev_info.device->numa_node,
> +		       rte_cryptodev_queue_pair_count(i));
> 
> Above 6lines have spaces+ tabs mixed, should use only tabs.

<snipped>

Thanks for the feedback, can you please help me understand usage pattern in nic_stats_display, nic_stats_clear and nic_xstats_by_name_display. 

I always use tabs, but since its mixed usage it forced me to follow the same. But since the suggestion is right one, I will share the V3 with the fixes for the newly added functions. 

Let us decide format correction for nic_stats_display, nic_stats_clear and nic_xstats_by_name_display separately too.
Varghese, Vipin Oct. 27, 2018, 4:42 a.m. UTC | #4 
Hi,

<snipped>

> ---
> +		struct rte_cryptodev_info dev_info = {0};
> +		struct rte_cryptodev_stats stats = {0};
> +
> 
> Memset for initialization as mentioned in other patch.
> 

Yes, I will correct the same as certain compiler flag combination will treat this as incorrect use.

> +
> +#define DSP_CRYPTO_FLAG(x) do { \
> +printf("  - feature flags\n"); \
> +printf("\t  -- symmetric (%c) asymmetric (%c)" \ " symmetric operation
> +chaining (%c)\n", \ (x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)?'y':'n', \
> +(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO)?'y':'n', \ (x &
> +RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING)?'y':'n'); \ printf("\t  -- CPU
> +SSE (%c) AVX (%c) AVX2 (%c) AVX512 (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_CPU_SSE)?'y':'n', \ (x &
> +RTE_CRYPTODEV_FF_CPU_AVX)?'y':'n', \ (x &
> +RTE_CRYPTODEV_FF_CPU_AVX2)?'y':'n', \ (x &
> +RTE_CRYPTODEV_FF_CPU_AVX512)?'y':'n'); \ printf("\t  -- Acclerate CPU
> +AESNI (%c) HW (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_AESNI)?'y':'n', \ (x
> +& RTE_CRYPTODEV_FF_HW_ACCELERATED)?'y':'n'); \ printf("\t  -- INLINE
> +(%c)\n", \ (x & RTE_CRYPTODEV_FF_SECURITY)?'y':'n'); \ printf("\t  --
> +ARM NEON (%c) CE (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_NEON)?'y':'n', \
> +(x & RTE_CRYPTODEV_FF_CPU_ARM_CE)?'y':'n'); \ printf("  - buffer
> +offload\n"); \ printf("\t  -- IN_PLACE_SGL (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_IN_PLACE_SGL)?'y':'n'); \ printf("\t  --
> +OOP_SGL_IN_SGL_OUT (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT)?'y':'n'); \ printf("\t  --
> +OOP_SGL_IN_LB_OUT (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT)?'y':'n'); \ printf("\t  --
> +OOP_LB_IN_SGL_OUT (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT)?'y':'n'); \ printf("\t  --
> +OOP_LB_IN_LB_OUT (%c)\n", \ (x &
> +RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT)?'y':'n'); \ } while (0)
> 
> This is a very big macro, better have static function for this instead of macro.
> 

There are two thoughts in choosing MACRO over function.
1. The information need to display in certain format within the same context. 
2. As the API are modified, co locating all at same scope is easier to clean up and correct in future.

So I feel use of MACRO over function in this instance.
Stephen Hemminger Oct. 28, 2018, 3:29 p.m. UTC | #5 
On Sat, 27 Oct 2018 04:42:19 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:

> Hi,
> 
> <snipped>
> 
> > ---
> > +		struct rte_cryptodev_info dev_info = {0};
> > +		struct rte_cryptodev_stats stats = {0};
> > +
> > 
> > Memset for initialization as mentioned in other patch.
> >   
> 
> Yes, I will correct the same as certain compiler flag combination will treat this as incorrect use.
> 
> > +
> > +#define DSP_CRYPTO_FLAG(x) do { \
> > +printf("  - feature flags\n"); \
> > +printf("\t  -- symmetric (%c) asymmetric (%c)" \ " symmetric operation
> > +chaining (%c)\n", \ (x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)?'y':'n', \
> > +(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO)?'y':'n', \ (x &
> > +RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING)?'y':'n'); \ printf("\t  -- CPU
> > +SSE (%c) AVX (%c) AVX2 (%c) AVX512 (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_CPU_SSE)?'y':'n', \ (x &
> > +RTE_CRYPTODEV_FF_CPU_AVX)?'y':'n', \ (x &
> > +RTE_CRYPTODEV_FF_CPU_AVX2)?'y':'n', \ (x &
> > +RTE_CRYPTODEV_FF_CPU_AVX512)?'y':'n'); \ printf("\t  -- Acclerate CPU
> > +AESNI (%c) HW (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_AESNI)?'y':'n', \ (x
> > +& RTE_CRYPTODEV_FF_HW_ACCELERATED)?'y':'n'); \ printf("\t  -- INLINE
> > +(%c)\n", \ (x & RTE_CRYPTODEV_FF_SECURITY)?'y':'n'); \ printf("\t  --
> > +ARM NEON (%c) CE (%c)\n", \ (x & RTE_CRYPTODEV_FF_CPU_NEON)?'y':'n', \
> > +(x & RTE_CRYPTODEV_FF_CPU_ARM_CE)?'y':'n'); \ printf("  - buffer
> > +offload\n"); \ printf("\t  -- IN_PLACE_SGL (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_IN_PLACE_SGL)?'y':'n'); \ printf("\t  --
> > +OOP_SGL_IN_SGL_OUT (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT)?'y':'n'); \ printf("\t  --
> > +OOP_SGL_IN_LB_OUT (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT)?'y':'n'); \ printf("\t  --
> > +OOP_LB_IN_SGL_OUT (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT)?'y':'n'); \ printf("\t  --
> > +OOP_LB_IN_LB_OUT (%c)\n", \ (x &
> > +RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT)?'y':'n'); \ } while (0)
> > 
> > This is a very big macro, better have static function for this instead of macro.
> >   
> 
> There are two thoughts in choosing MACRO over function.
> 1. The information need to display in certain format within the same context. 
> 2. As the API are modified, co locating all at same scope is easier to clean up and correct in future.
> 
> So I feel use of MACRO over function in this instance.


I don't agree with your arguments. Macros, are ugly and error prone. This is not performance
critical so it should be a function. The only reason to use macro's is if it is not possible to
write it as a function (as in a template for code generation).
Varghese, Vipin Oct. 29, 2018, 2:29 p.m. UTC | #6 
Hi Stepehen,

<snipped>

> > > This is a very big macro, better have static function for this instead of
> macro.
> > >
> >
> > There are two thoughts in choosing MACRO over function.
> > 1. The information need to display in certain format within the same context.
> > 2. As the API are modified, co locating all at same scope is easier to clean up
> and correct in future.
> >
> > So I feel use of MACRO over function in this instance.
> 
> 
> I don't agree with your arguments. Macros, are ugly and error prone. This is
> not performance critical so it should be a function. The only reason to use
> macro's is if it is not possible to write it as a function (as in a template for code
> generation).

Thanks for the sharing your feedback. But In my comments I never stated the MACRO were used for performance or critical blocks. As mentioned in my explanation, the MACRO is used for printing the desired format and format the code. 

Since I have received 2 request for change, I will do the same and release v3.

Thanks
Vipin Varghese
diff mbox series

Patch

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index f518935a2..d48334bd0 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -999,7 +999,75 @@  x.cman_wred_context_shared_n_max); \
 static void
 debug_crypto(void)
 {
-	printf(" crypto");
+	uint8_t crypto_dev_count = rte_cryptodev_count(), i;
+
+	snprintf(bdr_str, 100, "debug - CRYPTO PMD %"PRIu64, rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	for (i = 0; i < crypto_dev_count; i++) {
+		struct rte_cryptodev_info dev_info = {0};
+		struct rte_cryptodev_stats stats = {0};
+
+		rte_cryptodev_info_get(i, &dev_info);
+
+		printf("  - device (%u)\n", i);
+		printf("\t  -- name (%s) driver (%s)\n"
+		       "\t  -- id (%u) flags (0x%"PRIx64") socket (%d)\n"
+		       "\t  -- queue pairs (%d)\n",
+		       rte_cryptodev_name_get(i),
+		       dev_info.driver_name, dev_info.driver_id,
+		       dev_info.feature_flags, dev_info.device->numa_node,
+		       rte_cryptodev_queue_pair_count(i));
+
+#define DSP_CRYPTO_FLAG(x) do { \
+printf("  - feature flags\n"); \
+printf("\t  -- symmetric (%c) asymmetric (%c)" \
+" symmetric operation chaining (%c)\n", \
+(x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING)?'y':'n'); \
+printf("\t  -- CPU SSE (%c) AVX (%c) AVX2 (%c) AVX512 (%c)\n", \
+(x & RTE_CRYPTODEV_FF_CPU_SSE)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_CPU_AVX)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_CPU_AVX2)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_CPU_AVX512)?'y':'n'); \
+printf("\t  -- Acclerate CPU AESNI (%c) HW (%c)\n", \
+(x & RTE_CRYPTODEV_FF_CPU_AESNI)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_HW_ACCELERATED)?'y':'n'); \
+printf("\t  -- INLINE (%c)\n", \
+(x & RTE_CRYPTODEV_FF_SECURITY)?'y':'n'); \
+printf("\t  -- ARM NEON (%c) CE (%c)\n", \
+(x & RTE_CRYPTODEV_FF_CPU_NEON)?'y':'n', \
+(x & RTE_CRYPTODEV_FF_CPU_ARM_CE)?'y':'n'); \
+printf("  - buffer offload\n"); \
+printf("\t  -- IN_PLACE_SGL (%c)\n", \
+(x & RTE_CRYPTODEV_FF_IN_PLACE_SGL)?'y':'n'); \
+printf("\t  -- OOP_SGL_IN_SGL_OUT (%c)\n", \
+(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT)?'y':'n'); \
+printf("\t  -- OOP_SGL_IN_LB_OUT (%c)\n", \
+(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT)?'y':'n'); \
+printf("\t  -- OOP_LB_IN_SGL_OUT (%c)\n", \
+(x & RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT)?'y':'n'); \
+printf("\t  -- OOP_LB_IN_LB_OUT (%c)\n", \
+(x & RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT)?'y':'n'); \
+} while (0)
+
+		DSP_CRYPTO_FLAG(dev_info.feature_flags);
+
+		printf("  - stats\n");
+		if (rte_cryptodev_stats_get(i, &stats) == 0) {
+			printf("\t  -- enqueue count (%"PRIu64")"
+			       " error (%"PRIu64")\n",
+			       stats.enqueued_count,
+			       stats.enqueue_err_count);
+			printf("\t  -- dequeue count (%"PRIu64")"
+			       " error (%"PRIu64")\n",
+			       stats.dequeued_count,
+			       stats.dequeue_err_count);
+		}
+	}
+
+	STATS_BDR_STR(50, "");
 }
 
 static void