[ovs-dev] [PATCH 1/7] userspace: Add packet_type in dp_packet and flow

[Ben Pfaff]

On Fri, Feb 03, 2017 at 10:38:31AM +0000, Jan Scheurich wrote:
> This commit adds a packet_type attribute to the structs dp_packet and flow
> to explicitly carry the type of the packet as preparation for the
> introduction of the so-called packet type-aware pipeline (PTAP) in OVS.
> 
> The packet_type is a big-endian 32 bit integer with the encoding as
> specified in OpenFlow verion 1.5.

This fails to build because:

    include/openvswitch/flow.h:#include "byte-order.h"
    include/openvswitch/flow.h:#include "lib/packets.h"
    See above for list of violations of the rule that
    public openvswitch header file should not include internal library.
    Makefile:5662: recipe for target 'config-h-check' failed

It also causes "sparse" warnings.  The first one is easily fixed by
0xffff to OVS_BE16_MAX:

    ../lib/flow.c:572:24: warning: incorrect type in initializer (different base types)
    ../lib/flow.c:572:24:    expected restricted ovs_be16 [usertype] dl_type
    ../lib/flow.c:572:24:    got int

The second seems to be due to confused macros PACKET_TYPE and PT_NS.
The definition of PT_NS doesn't make any sense to me, and neither does
PT_NS_TYPE_BE.  Actually, PACKET_TYPE and the enums derived from it
don't fit the usual OVS style of having constants in host byte order:

    ../lib/flow.c:645:19: warning: restricted ovs_be32 degrades to integer
    ../lib/flow.c:645:17: warning: incorrect type in assignment (different base types)
    ../lib/flow.c:645:17:    expected restricted ovs_be16 [assigned] [usertype] dl_type
    ../lib/flow.c:645:17:    got unsigned int

The final one is reporting that an ethertype in network byte order is
being byteswapped as part of an htonl (embedded in PACKET_TYPE):

    ../lib/dpif-netlink.c:2049:52: warning: restricted ovs_be16 degrades to integer

I don't see any uses of PT_NS* that couldn't be implemented as inline
functions instead of macros.

This introduces extra padding in the metadata area of struct flow, to
bring it to 8 bytes of padding total.  Why doesn't it replace pad1[] by
the new field?  Then the size of struct flow would not increase at all.

I'm not sure of the value of adding comments that packet-in/outs can
only carry Ethernet.

I'm not sure of the value of dp_packet_packet_type() and
dp_packet_set_packet_type().  They don't seem to provide any useful
abstraction.

dp_packet_is_l3() seems weird.  Why is a packet whose type is specified
by Ethertype an L3 packet?

The names of dp_packet_is_l2() and dp_packet_l2() are now pretty
deceptive.  Ethernet is not the only L2 protocol, and these are
specifically about Ethernet packets, so perhaps they should be renamed
with "eth" or "ethernet" in their names.

This patch makes the design choice of adding a separate packet_type to
dp_packet, instead of putting the packet_type inside struct
pkt_metadata.  Did you explore the trade off here?  What motivated the
decision?

Thanks,

Ben.