On 27/07/2016 14:18, "Ben Pfaff" <b...@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
>> <diproiet...@vmware.com> wrote:
>> > On 27/07/2016 13:12, "Joe Stringer" <j...@ovn.org> wrote:
>> >
>> >>On 26 July 2016 at 17:58, Daniele Di Proietto <diproiet...@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 <diproiet...@vmware.com>
>> >>> Acked-by: Joe Stringer <j...@ovn.org>
>> >>> Acked-by: Flavio Leitner <f...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
