Hi Ben, Thanks for the review. Please find some answers below. Jan > -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Friday, 03 March, 2017 19:14 > To: Jan Scheurich <[email protected]> > Cc: [email protected] > 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. > > 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
We found and fixed that during the latest rebase. > > 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. > > ../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. Will try to change to inline functions. > 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. Good point. Will fix that. > > 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. > 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. > dp_packet_is_l3() seems weird. Why is a packet whose type is specified > by Ethertype an L3 packet? Actually this function is not used. We'll remove it. > 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. > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
