Re: dpdk16.11 RC2 package ipv4 reassembly example can't work

Re: dpdk16.11 RC2 package ipv4 reassembly example can't work

[Adrien Mazarguil]

Hi all,

On Wed, Nov 02, 2016 at 08:39:31AM +0000, Lu, Wenzhuo wrote:
> Correct the typo of receiver.
> 
> Hi Adrien,
> The change from struct ip_frag_pkt pkt[0]  to struct ip_frag_pkt pkt[] will make IP reassembly not working. I think this is not the root cause. Maybe Konstantin can give us some idea.
> But I notice one thing, you change some from [0] to [], but others just add '__extension__'. I believe if you add '__extension__' for struct ip_frag_pkt pkt[0], we'll not hit this issue. Just curious why you use 2 ways to resolve the same problem.

I've used the __extension__ method whenever the C99 syntax could not work
due to invalid usage in the code, e.g. a flexible array cannot be the only
member of a struct, you cannot make arrays out of structures that contain
such fields, while there is no such constraint with the GNU syntax.

For example see __extension__ uint8_t action_data[0] in struct
rte_pipeline_table_entry. The C99 could not be used because of
test_table_acl.c:

  struct rte_pipeline_table_entry entries[5];

If replacing ip_frag_pkt[] with __extension__ ip_frag_pkt pkt[0] in
rte_ip_frag.h solves the issue, either some code is breaking some constraint
somewhere or this change broke the ABI (unlikely considering a simple
recompilation should have taken care of the issue). I did not notice any
change in sizeof(struct rte_ip_frag_tbl) nor offsetof(struct
rte_ip_frag_tbl, pkt) on my setup, perhaps the compilation flags used in
your test affect them somehow.

Can you confirm whether only reverting this particular field solves the
issue?

> From: Xu, HuilongX
> Sent: Wednesday, November 2, 2016 4:29 PM
> To: drien.mazarguil@6wind.com
> Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Lu, Wenzhuo; Xu, HuilongX
> Subject: dpdk16.11 RC2 package ipv4 reassembly example can't work
> 
> Hi mazarguil,
> I find ip reassembly example can't work with dpdk16.11 rc2 package.
> But when I reset dpdk code before 347a1e037fd323e6c2af55d17f7f0dc4bfe1d479, it works ok.
> Could you have time to check this issue, thanks  a lot.
> Unzip password: intel123
> 
> Test detail info:
> 
> os&kernel:4.2.3-300.fc23.x86_64
> gcc version:5.3.1 20160406 (Red Hat 5.3.1-6) (GCC)
> NIC:03:00.0 Ethernet controller [0200]: Intel Corporation Ethernet Connection X552/X557-AT 10GBASE-T [8086:15ad] and
> 84:00.0 Ethernet controller [0200]: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection [8086:10fb] (rev 01)
> package: dpdk16.11.rc2.tar.gz
> test steps:
> 1. build and install dpdk
> 2. build ip_reassembly example
> 3. run ip_reassembly
> ./examples/ip_reassembly/build/ip_reassembly -c 0x2 -n 4 - -p 0x1 --maxflows=1024 --flowttl=10s
> 4. set tester port mtu
> ip link set mtu 9000 dev ens160f1
> 5. setup scapy on tester and send packet
> scapy
> pcap = rdpcap("file.pcap")
> sendp(pcap, iface="ens160f1")
> 6. sniff packet on tester and check packet
> test result:
> dpdk16.04 reassembly packet successful but dpdk16.11 reassembly pack failed.
> 
> comments:
> file.pcap: send packets pcap file
> tcpdump_16.04_reassembly_successful.pcap: sniff packets by tcpdump on 16.04.
> tcpdump_reset_code_reassembly_failed.pcap: sniff packets by tcpdump on 16.11
> reset_code_reassembly_successful_.jpg: reassembly a packets successful detail info
> dpdk16.11_reassembly_failed.jpg: reassembly a packets failed detail info
> 

-- 
Adrien Mazarguil
6WIND

Re: dpdk16.11 RC2 package ipv4 reassembly example can't work

[Lu, Wenzhuo]

Hi Adrien,

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Wednesday, November 2, 2016 11:21 PM
> To: Lu, Wenzhuo
> Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Xu, HuilongX;
> dev@dpdk.org
> Subject: Re: dpdk16.11 RC2 package ipv4 reassembly example can't work
> 
> Hi all,
> 
> On Wed, Nov 02, 2016 at 08:39:31AM +0000, Lu, Wenzhuo wrote:
> > Correct the typo of receiver.
> >
> > Hi Adrien,
> > The change from struct ip_frag_pkt pkt[0]  to struct ip_frag_pkt pkt[] will
> make IP reassembly not working. I think this is not the root cause. Maybe
> Konstantin can give us some idea.
> > But I notice one thing, you change some from [0] to [], but others just add
> '__extension__'. I believe if you add '__extension__' for struct ip_frag_pkt pkt[0],
> we'll not hit this issue. Just curious why you use 2 ways to resolve the same
> problem.
> 
> I've used the __extension__ method whenever the C99 syntax could not work
> due to invalid usage in the code, e.g. a flexible array cannot be the only member
> of a struct, you cannot make arrays out of structures that contain such fields,
> while there is no such constraint with the GNU syntax.
> 
> For example see __extension__ uint8_t action_data[0] in struct
> rte_pipeline_table_entry. The C99 could not be used because of
> test_table_acl.c:
> 
>   struct rte_pipeline_table_entry entries[5];
> 
> If replacing ip_frag_pkt[] with __extension__ ip_frag_pkt pkt[0] in rte_ip_frag.h
> solves the issue, either some code is breaking some constraint somewhere or
> this change broke the ABI (unlikely considering a simple recompilation should
> have taken care of the issue). I did not notice any change in sizeof(struct
> rte_ip_frag_tbl) nor offsetof(struct rte_ip_frag_tbl, pkt) on my setup, perhaps
> the compilation flags used in your test affect them somehow.
Thanks for your explanation. I also checked sizeof(struct rte_ip_frag_tbl). I don't see any change either.

> 
> Can you confirm whether only reverting this particular field solves the issue?
Yes. ip_frag_pkt pkt[0] or even ip_frag_pkt pkt[1] can work but ip_frag_pkt pkt[] cannot :(
Do you like the idea of changing the ip_frag_pkt[] to __extension__ ip_frag_pkt pkt[0]?

> 
> > From: Xu, HuilongX
> > Sent: Wednesday, November 2, 2016 4:29 PM
> > To: drien.mazarguil@6wind.com
> > Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Lu, Wenzhuo; Xu,
> > HuilongX
> > Subject: dpdk16.11 RC2 package ipv4 reassembly example can't work
> >
> > Hi mazarguil,
> > I find ip reassembly example can't work with dpdk16.11 rc2 package.
> > But when I reset dpdk code before
> 347a1e037fd323e6c2af55d17f7f0dc4bfe1d479, it works ok.
> > Could you have time to check this issue, thanks  a lot.
> > Unzip password: intel123
> >
> > Test detail info:
> >
> > os&kernel:4.2.3-300.fc23.x86_64
> > gcc version:5.3.1 20160406 (Red Hat 5.3.1-6) (GCC)
> > NIC:03:00.0 Ethernet controller [0200]: Intel Corporation Ethernet
> > Connection X552/X557-AT 10GBASE-T [8086:15ad] and
> > 84:00.0 Ethernet controller [0200]: Intel Corporation 82599ES
> > 10-Gigabit SFI/SFP+ Network Connection [8086:10fb] (rev 01)
> > package: dpdk16.11.rc2.tar.gz
> > test steps:
> > 1. build and install dpdk
> > 2. build ip_reassembly example
> > 3. run ip_reassembly
> > ./examples/ip_reassembly/build/ip_reassembly -c 0x2 -n 4 - -p 0x1
> > --maxflows=1024 --flowttl=10s 4. set tester port mtu ip link set mtu
> > 9000 dev ens160f1 5. setup scapy on tester and send packet scapy pcap
> > = rdpcap("file.pcap") sendp(pcap, iface="ens160f1") 6. sniff packet on
> > tester and check packet test result:
> > dpdk16.04 reassembly packet successful but dpdk16.11 reassembly pack failed.
> >
> > comments:
> > file.pcap: send packets pcap file
> > tcpdump_16.04_reassembly_successful.pcap: sniff packets by tcpdump on
> 16.04.
> > tcpdump_reset_code_reassembly_failed.pcap: sniff packets by tcpdump on
> > 16.11
> > reset_code_reassembly_successful_.jpg: reassembly a packets successful
> > detail info
> > dpdk16.11_reassembly_failed.jpg: reassembly a packets failed detail
> > info
> >
> 
> --
> Adrien Mazarguil
> 6WIND

Re: dpdk16.11 RC2 package ipv4 reassembly example can't work

[Adrien Mazarguil]

On Fri, Nov 04, 2016 at 06:36:30AM +0000, Lu, Wenzhuo wrote:
> Hi Adrien,
> 
> > -----Original Message-----
> > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> > Sent: Wednesday, November 2, 2016 11:21 PM
> > To: Lu, Wenzhuo
> > Cc: Ananyev, Konstantin; Liu, Yu Y; Chen, WeichunX; Xu, HuilongX;
> > dev@dpdk.org
> > Subject: Re: dpdk16.11 RC2 package ipv4 reassembly example can't work
> > 
> > Hi all,
> > 
> > On Wed, Nov 02, 2016 at 08:39:31AM +0000, Lu, Wenzhuo wrote:
> > > Correct the typo of receiver.
> > >
> > > Hi Adrien,
> > > The change from struct ip_frag_pkt pkt[0]  to struct ip_frag_pkt pkt[] will
> > make IP reassembly not working. I think this is not the root cause. Maybe
> > Konstantin can give us some idea.
> > > But I notice one thing, you change some from [0] to [], but others just add
> > '__extension__'. I believe if you add '__extension__' for struct ip_frag_pkt pkt[0],
> > we'll not hit this issue. Just curious why you use 2 ways to resolve the same
> > problem.
> > 
> > I've used the __extension__ method whenever the C99 syntax could not work
> > due to invalid usage in the code, e.g. a flexible array cannot be the only member
> > of a struct, you cannot make arrays out of structures that contain such fields,
> > while there is no such constraint with the GNU syntax.
> > 
> > For example see __extension__ uint8_t action_data[0] in struct
> > rte_pipeline_table_entry. The C99 could not be used because of
> > test_table_acl.c:
> > 
> >   struct rte_pipeline_table_entry entries[5];
> > 
> > If replacing ip_frag_pkt[] with __extension__ ip_frag_pkt pkt[0] in rte_ip_frag.h
> > solves the issue, either some code is breaking some constraint somewhere or
> > this change broke the ABI (unlikely considering a simple recompilation should
> > have taken care of the issue). I did not notice any change in sizeof(struct
> > rte_ip_frag_tbl) nor offsetof(struct rte_ip_frag_tbl, pkt) on my setup, perhaps
> > the compilation flags used in your test affect them somehow.
> Thanks for your explanation. I also checked sizeof(struct rte_ip_frag_tbl). I don't see any change either.
> 
> > 
> > Can you confirm whether only reverting this particular field solves the issue?
> Yes. ip_frag_pkt pkt[0] or even ip_frag_pkt pkt[1] can work but ip_frag_pkt pkt[] cannot :(
> Do you like the idea of changing the ip_frag_pkt[] to __extension__ ip_frag_pkt pkt[0]?

Yes, restoring the original code (with __extension__) as a workaround until
we understand what is going on is safer, that's fine by me. The commit log
should explicitly state that weirdness occurs for an unknown reason with the
C99 syntax though (compiler bug is also a possibility).

-- 
Adrien Mazarguil
6WIND