Re: [dpdk-dev] [PATCH v6 1/2] ethdev: add packet integrity checks

[Gregory Etelson <getelson@nvidia.com>]

Hello Thomas,

I modified the following drivers/net/mlx5/mlx5_flow_age.c compilation command
to produce pre-processed source code output:

     1 # 1 "../drivers/net/mlx5/mlx5_flow_age.c"
     2 # 1 "/.autodirect/mtrswgwork/getelson/src/dpdk/stable/build-dev//"
     3 # 1 "<built-in>"
     4 #define __STDC__ 1
** 5 #define __STDC_VERSION__ 201112L
     6 #define __STDC_UTF_16__ 1

According to the result, the built-in __STDC_VERSION__ macro was set to 201112L.
Therefore, in rte_common.h, RTE_STD_C11 macro was evaluated as empty value:

Source code:
 30 #ifndef typeof
 31 #define typeof __typeof__
 32 #endif
 33
 34 #ifndef asm
 35 #define asm __asm__
 36 #endif
 37
 38 /** C extension macro for environments lacking C11 features. */
 39 #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
 40 #define RTE_STD_C11 __extension__
 41 #else
 42 #define RTE_STD_C11
 43 #endif

Preprocessor output:
# 29 "../lib/librte_eal/include/rte_common.h" 2
#define typeof __typeof__
#define asm __asm__
#define RTE_STD_C11

According to these results, RTE_STD_C11 location in code has no significance,
because it will always be replaced with empty string.
After I changed RTE_STD_C11 condition like this: 

-  #if !defined(__STDC_VERSION__) || __STDC_VERSION__ < 201112L
+ #if !defined(__STDC_VERSION__) || __STDC_VERSION__ <= 201112L

-__extension__
+RTE_STD_C11
 struct rte_flow_item_integrity {

the compilation completed successfully both for 32 and 64 bits value.

Regards,
Gregory. 

The compilation command was copied from `ninja --verbose` output:
cc -Idrivers/libtmp_rte_net_mlx5.a.p -Idrivers -I../drivers -Idrivers/net/mlx5 -I../drivers/net/mlx5 \
-Idrivers/net/mlx5/linux -I../drivers/net/mlx5/linux -Ilib/librte_ethdev -I../lib/librte_ethdev \
-I. -I.. -Iconfig -I../config -Ilib/librte_eal/include -I../lib/librte_eal/include -Ilib/librte_eal/linux/include \
-I../lib/librte_eal/linux/include -Ilib/librte_eal/x86/include -I../lib/librte_eal/x86/include \
-Ilib/librte_eal/common -I../lib/librte_eal/common -Ilib/librte_eal -I../lib/librte_eal \
-Ilib/librte_kvargs -I../lib/librte_kvargs -Ilib/librte_metrics -I../lib/librte_metrics \
-Ilib/librte_telemetry -I../lib/librte_telemetry -Ilib/librte_net -I../lib/librte_net \
-Ilib/librte_mbuf -I../lib/librte_mbuf -Ilib/librte_mempool -I../lib/librte_mempool \
-Ilib/librte_ring -I../lib/librte_ring -Ilib/librte_meter -I../lib/librte_meter -Idrivers/bus/pci \
-I../drivers/bus/pci -I../drivers/bus/pci/linux -Ilib/librte_pci -I../lib/librte_pci \
-Idrivers/bus/vdev -I../drivers/bus/vdev -Ilib/librte_hash -I../lib/librte_hash \
-Ilib/librte_rcu -I../lib/librte_rcu -Idrivers/common/mlx5 -I../drivers/common/mlx5 \
-Idrivers/common/mlx5/linux -I../drivers/common/mlx5/linux -I/usr//usr/include \
-I/usr/include/libnl3 -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -g \
-include rte_config.h -Wextra -Wcast-qual -Wdeprecated -Wformat \
-Wformat-nonliteral -Wformat-security -Wmissing-declarations -Wmissing-prototypes \
-Wnested-externs -Wold-style-definition -Wpointer-arith -Wsign-compare -Wstrict-prototypes \
-Wundef -Wwrite-strings -Wno-missing-field-initializers -D_GNU_SOURCE -fPIC \
-march=native -DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -std=c11 \
-Wno-strict-prototypes -D_BSD_SOURCE -D_DEFAULT_SOURCE -D_XOPEN_SOURCE=600 \
-pedantic -DPEDANTIC -MD -MQ drivers/libtmp_rte_net_mlx5.a.p/net_mlx5_mlx5_flow_age.c.o \
-MF drivers/libtmp_rte_net_mlx5.a.p/net_mlx5_mlx5_flow_age.c.o.d \
-o drivers/libtmp_rte_net_mlx5.a.p/net_mlx5_mlx5_flow_age.c.o -c ../drivers/net/mlx5/mlx5_flow_age.c 

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Sunday, April 18, 2021 21:12
> To: Gregory Etelson <getelson@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; ajit.khaparde@broadcom.com;
> andrew.rybchenko@oktetlabs.ru; dev@dpdk.org; ferruh.yigit@intel.com;
> jerinj@marvell.com; jerinjacobk@gmail.com; olivier.matz@6wind.com;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Gregory Etelson
> <getelson@nvidia.com>; Matan Azrad <matan@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>
> Subject: Re: [PATCH v6 1/2] ethdev: add packet integrity checks
> 
> External email: Use caution opening links or attachments
> 
> 
> 18/04/2021 17:51, Gregory Etelson:
> > +__extension__
> 
> That still doesn't make sense, as in v5.
> The things which require a macro are anonymous union, anonymous struct
> and some bit fields with special sizes.
> 
> > +struct rte_flow_item_integrity {
> > +     /**< Tunnel encapsulation level the item should apply to.
> > +      * @see rte_flow_action_rss
> > +      */
> > +     uint32_t level;
> 
> Should have RTE_STD_C11 here.
> 
> > +     union {
> 
> Should have RTE_STD_C11 here.
> 
> > +             struct {
> > +                     /**< The packet is valid after passing all HW checks. */
> > +                     uint64_t packet_ok:1;
> > +                     /**< L2 layer is valid after passing all HW checks. */
> > +                     uint64_t l2_ok:1;
> > +                     /**< L3 layer is valid after passing all HW checks. */
> > +                     uint64_t l3_ok:1;
> > +                     /**< L4 layer is valid after passing all HW checks. */
> > +                     uint64_t l4_ok:1;
> > +                     /**< L2 layer CRC is valid. */
> > +                     uint64_t l2_crc_ok:1;
> > +                     /**< IPv4 layer checksum is valid. */
> > +                     uint64_t ipv4_csum_ok:1;
> > +                     /**< L4 layer checksum is valid. */
> > +                     uint64_t l4_csum_ok:1;
> > +                     /**< The l3 length is smaller than the frame length. */
> > +                     uint64_t l3_len_ok:1;
> > +                     uint64_t reserved:56;
> 
> The reserved space looks useless since it is in an union.
> 
> > +             };
> 
> I'm not sure about the 64-bit bitfields.
> Maybe that's why you need __extension__.
> I feel 32 bits are enough.
> 
> > +             uint64_t value;
> > +     };
> > +};
> 
>