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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
