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

[Thomas Monjalon <thomas@monjalon.net>]

Please read again the comment I did below,
and try 32-bit bitfield instead of 64-bit.


18/04/2021 21:24, Gregory Etelson:
> 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;
> > > +     };
> > > +};