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

[Ben Pfaff]

On Sun, Mar 05, 2017 at 11:45:58PM +0000, Jan Scheurich wrote:
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp at ovn.org]
> > Sent: Friday, 03 March, 2017 19:14
> > To: Jan Scheurich <jan.scheurich at ericsson.com>
> > Cc: dev at openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH 1/7] userspace: Add packet_type in dp_packet and flow
> > 
> > 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.
> > 
> > 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:
> 
> We were not aware of that convention. Will change the constant
> definitions to be in host byte order.  BTW: What compiler did you use
> to get these warnings? gcc doesn't complain.

As I said, these warnings are from "sparse", which is integrated into
the OVS build.  See the install guide for more information.

> > I'm not sure of the value of adding comments that packet-in/outs can
> > only carry Ethernet.
> 
> It was more a reminder for us that these will require update when
> implementing packet type-aware pipeline. They would be
> rephrased/removed with the second patch series. We can remove them
> now.

OK.

> > 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.
> 
> We added them to hide whether packet_type was stored in struct
> dp_packet or the pkt_metadata inside (see below). If we agree to keep
> it in dp_packet, there is indeed little value having them. If not, we
> should better keep them.

I'm OK with that distinction.

> > 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.
> 
> I'm not sure. dp_packet_l2() is a legacy function that is used widely
> and has always returned the head of the packet buffer (providing that
> the packet had been parsed and thus l3_offset set) assuming that it
> pointed to an Ethernet header. I don't think the introduction of
> packet_type means OVS will start supporting other L2 protocols.

It's always returned the start of the Ethernet header, which has always
been one and the same as the start of the L2 header.  It seems that,
following this change, it should either always return the start of the
L2 header (regardless of the type of that L2 header) or the start of the
Ethernet header.  This patch makes the latter choice, which is
reasonable, but then the name is confusing and I think that we should
change it.  I only see 16 references to this function so changing those
references along with the rename seems perfectly reasonable to me.

By the way, when you start talking about "legacy" functions, etc., that
just means to me that we should fix things to be internally consistent.
If there are visible legacies in our code (and I'm sure there are) then
that just means that we need to work harder at this.

> > 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?
> 
> We initially had it in pkt_metadata, but that was awkward because pkt_metadata is initialized through invoking function pkt_metadata_init() in the dpif-netdev datapath after reception of a packet from a netdev, while the packet_type is part of the metadata returned from the netdev, similar to the port number. Since a versatile tunnel netdev can return packets of different packet_type in a single batch, the packet_type had to be included in struct dp_packet somehow. Passing it as an out-of-band parameter as done for the port_no was not an option. Making the packet_type part of struct dp_packet solved those problems. 
> 
> Conceptually the packet_type can be considered as packet metadata, but we didn't want to store it both in dp_packet and pkt_metadata, especially since another copy is stored in struct (mini)flow after parsing.
> 
> Only storing it in pkt_metadata, would require netdevs to write the packet_type into the non-initialized struct pkt_metadata, and dpif-netdev to initialize all pkt_metadata except the packet_type. For sure doable but not quite clean in my eyes.

OK, you thought through the alternatives, great.  Really I wanted to
make sure that it was a conscious decision and not just accidental.  So,
in that case, let's stick with that choice (and we can drop the
dp_packet_get/set_packet_type() functions).

Thanks a lot!

Ben