[ovs-dev] [PATCH v5 13/16] system-tests: Run conntrack tests with userspace.

[Daniele Di Proietto]

On 27/07/2016 14:18, "Ben Pfaff" <blp at ovn.org> wrote:

>On Wed, Jul 27, 2016 at 01:51:00PM -0700, Jesse Gross wrote:
>> On Wed, Jul 27, 2016 at 1:40 PM, Daniele Di Proietto
>> <diproiettod at vmware.com> wrote:
>> > On 27/07/2016 13:12, "Joe Stringer" <joe at ovn.org> wrote:
>> >
>> >>On 26 July 2016 at 17:58, Daniele Di Proietto <diproiettod at vmware.com> wrote:
>> >>> The userspace connection tracker doesn't support ALGs, frag reassembly
>> >>> or NAT yet, so skip those tests.
>> >>>
>> >>> Also, connection tracking state input from a local port is not possible
>> >>> in userspace.
>> >>>
>> >>> The userspace datapath pads all frames with 0, to make them at
>> >>> least 64 bytes.
>> >>>
>> >>> Finally, the userspace datapath checks for the IPv4 header checksum, so
>> >>> fix those in the hardcoded packets.
>> >>>
>> >>> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
>> >>> Acked-by: Joe Stringer <joe at ovn.org>
>> >>> Acked-by: Flavio Leitner <fbl at sysclose.org>
>> >>> ---
>> >>
>> >><snip>
>> >>
>> >>> @@ -1324,11 +1327,11 @@ dnl UDP packets from ns0->ns1 should solicit "destination unreachable" response.
>> >>>  NS_CHECK_EXEC([at_ns0], [bash -c "echo a | nc $NC_EOF_OPT -u 10.1.1.2 10000"])
>> >>>
>> >>>  AT_CHECK([ovs-appctl revalidator/purge], [0])
>> >>> -AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v drop], [0], [dnl
>> >>> - n_packets=1, n_bytes=44, priority=100,udp,in_port=1 actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
>> >>> - n_packets=1, n_bytes=72, priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 actions=output:1
>> >>> - n_packets=1, n_bytes=72, priority=100,ct_state=-trk,icmp,in_port=2 actions=ct(table=0)
>> >>> - n_packets=2, n_bytes=84, priority=10,arp actions=NORMAL
>> >>> +AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort | grep -v drop | sed -e 's/n_bytes=[[0-9]]*/n_bytes=<cleared>/g'], [0], [dnl
>> >>> + n_packets=1, n_bytes=<cleared>, priority=100,udp,in_port=1 actions=ct(commit,exec(load:0x1->NXM_NX_CT_MARK[[]])),output:2
>> >>> + n_packets=1, n_bytes=<cleared>, priority=100,ct_state=+rel+trk,ct_mark=0x1,icmp,in_port=2 actions=output:1
>> >>> + n_packets=1, n_bytes=<cleared>, priority=100,ct_state=-trk,icmp,in_port=2 actions=ct(table=0)
>> >>> + n_packets=2, n_bytes=<cleared>, priority=10,arp actions=NORMAL
>> >>>  NXST_FLOW reply:
>> >>>  ])
>> >>
>> >>I think this is a completely orthogonal point, but it's still a bit
>> >>surprising to me that the n_bytes would differ when receiving short
>> >>packets in kernel vs. userspace datapaths. I follow that userspace
>> >>pads shorter packets on receive, but shouldn't we be able to attribute
>> >>these stats consistently, regardless of the datapath?
>> >
>> > That's a good point.
>> >
>> > We call dp_packet_pad() in netdev_linux_recv().  That used to be in netdev_recv() and can be traced back to the initial OVS commit.  Here's a comment in netdev_recv():
>> >
>> >         COVERAGE_INC(netdev_received);
>> >         buffer->size += n_bytes;
>> >
>> >         /* When the kernel internally sends out an Ethernet frame on an
>> >          * interface, it gives us a copy *before* padding the frame to the
>> >          * minimum length.  Thus, when it sends out something like an ARP
>> >          * request, we see a too-short frame.  So pad it out to the minimum
>> >          * length. */
>> >         pad_to_minimum_length(buffer);
>> 
>> I wonder if anything in OVS actually cares about this anymore? I don't
>> know the history of that comment.
>
>I don't remember the origin anymore but it was probably my comment.
>It's possible that some code in OVS assumed that packets were at least
>64 bytes long at some point.  For example, flow_extract() might have
>been able to be slightly simplified on the basis that access to the IPv4
>header couldn't be beyond the end of the buffer.
>
>I doubt we do that kind of optimization any longer.

Thanks for the clarifying.  I agree, it doesn't look like it's needed anymore.

I've sent a couple of patches to remove that, along with the connection tracker system tests for userspace here:

http://openvswitch.org/pipermail/dev/2016-July/076659.html