[ovs-dev] [#8024v2 2/5] Don't overload IP TOS with the frag matching bits.

[Justin Pettit]

On Nov 8, 2011, at 8:12 PM, Jesse Gross wrote:

> On Tue, Nov 8, 2011 at 3:57 PM, Justin Pettit <jpettit at nicira.com> wrote:
>> diff --git a/datapath/flow.h b/datapath/flow.h
>> index ba8c66a..f12b11a 100644
>> --- a/datapath/flow.h
>> +++ b/datapath/flow.h
>> @@ -147,7 +143,7 @@ u64 flow_used_time(unsigned long flow_jiffies);
>>  *  OVS_KEY_ATTR_ETHERNET     12    --     4     16
>>  *  OVS_KEY_ATTR_8021Q         4    --     4      8
>>  *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8
>> - *  OVS_KEY_ATTR_IPV6         38     2     4     44
>> + *  OVS_KEY_ATTR_IPV6         39     1     4     44
> 
> I think this is just a mistake since this patch has nothing IPv6
> specific in it but this list is for the userspace interface, which
> this doesn't change.

I don't understand this or your follow-up about it being incorrect before this patch.  Both the IPv4 and IPv6 "ipvX_tos" fields that expand the struct size by one.  Are you stating that it's inconsistent with "lib/odp-util.h"?  If so, then I've updated them to be in sync.  

> In userspace there's a fair amount of masking with IP_DSCP_MASK even
> though it is unnecessary because the userspace flow shouldn't contain
> the ECN bits.  I'm guessing that it all gets resolved (or becomes
> necessary) in the patch where ECN support is added, so it doesn't
> really matter.

Correct.  Or, at least, that's the hope.  :-)

> I don't have any further comments beyond Ben's


At the end of the message,  I've included an incremental that includes Ben's suggestions.

--Justin


diff --git a/datapath/flow.c b/datapath/flow.c
index 285006d..5080ee9 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -693,12 +693,12 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct 
 
                offset = nh->frag_off & htons(IP_OFFSET);
                if (offset) {
-                       key->ip.frag |= OVS_FRAG_TYPE_LATER;
+                       key->ip.frag = OVS_FRAG_TYPE_LATER;
                        goto out;
                }
                if (nh->frag_off & htons(IP_MF) ||
                         skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-                       key->ip.frag |= OVS_FRAG_TYPE_FIRST;
+                       key->ip.frag = OVS_FRAG_TYPE_FIRST;
 
                /* Transport layer. */
                if (key->ip.proto == IPPROTO_TCP) {
@@ -765,7 +765,7 @@ int flow_extract(struct sk_buff *skb, u16 in_port, struct sw
                if (key->ip.frag == OVS_FRAG_TYPE_LATER)
                        goto out;
                if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
-                       key->ip.frag |= OVS_FRAG_TYPE_FIRST;
+                       key->ip.frag = OVS_FRAG_TYPE_FIRST;
 
                /* Transport layer. */
                if (key->ip.proto == NEXTHDR_TCP) {
diff --git a/datapath/flow.h b/datapath/flow.h
index 7e3dcf9..77a955f 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -45,7 +45,7 @@ struct sw_flow_key {
        struct {
                u8     proto;           /* IP protocol or lower 8 bits of ARP op
                u8     tos;         /* IP ToS DSCP in high 6 bits. */
-               u8     frag;        /* OVS_FRAG_TYPE_* in low 2 bits. */
+               u8     frag;        /* OVS_FRAG_TYPE_* flags. */
        } ip;
        union {
                struct {
diff --git a/lib/flow.c b/lib/flow.c
index bc53bd8..99cef1e 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -371,7 +371,7 @@ flow_extract(struct ofpbuf *packet, uint32_t priority, ovs_b
 
             flow->tos = nh->ip_tos & IP_DSCP_MASK;
             if (IP_IS_FRAGMENT(nh->ip_frag_off)) {
-                flow->frag |= FLOW_FRAG_ANY;
+                flow->frag = FLOW_FRAG_ANY;
                 if (nh->ip_frag_off & htons(IP_FRAG_OFF_MASK)) {
                     flow->frag |= FLOW_FRAG_LATER;
                 }
diff --git a/lib/flow.h b/lib/flow.h
index 248233e..af634fb 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -71,9 +71,9 @@ struct flow {
     uint8_t dl_dst[6];          /* Ethernet destination address. */
     uint8_t nw_proto;           /* IP protocol or low 8 bits of ARP opcode. */
     uint8_t tos;                /* IP ToS. */
-    uint8_t frag;               /* FLOW_FRAG_*. */
     uint8_t arp_sha[6];         /* ARP/ND source hardware address. */
     uint8_t arp_tha[6];         /* ARP/ND target hardware address. */
+    uint8_t frag;               /* FLOW_FRAG_* flags. */
     uint8_t reserved[7];        /* Reserved for 64-bit packing. */
 };
 
@@ -81,8 +81,8 @@ struct flow {
  * flow", followed by FLOW_PAD_SIZE bytes of padding. */
 #define FLOW_SIG_SIZE (109 + FLOW_N_REGS * 4)
 #define FLOW_PAD_SIZE 7
-BUILD_ASSERT_DECL(offsetof(struct flow, arp_tha) == FLOW_SIG_SIZE - 6);
-BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->arp_tha) == 6);
+BUILD_ASSERT_DECL(offsetof(struct flow, frag) == FLOW_SIG_SIZE - 1);
+BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->frag) == 1);
 BUILD_ASSERT_DECL(sizeof(struct flow) == FLOW_SIG_SIZE + FLOW_PAD_SIZE);
 
 /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 8a7fbc6..73c3362 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -72,7 +72,7 @@ void format_odp_actions(struct ds *, const struct nlattr *odp_
  *  OVS_KEY_ATTR_ETHERNET     12    --     4     16
  *  OVS_KEY_ATTR_8021Q         4    --     4      8
  *  OVS_KEY_ATTR_ETHERTYPE     2     2     4      8
- *  OVS_KEY_ATTR_IPV6         38     2     4     44
+ *  OVS_KEY_ATTR_IPV6         39     1     4     44
  *  OVS_KEY_ATTR_ICMPV6        2     2     4      8
  *  OVS_KEY_ATTR_ND           28    --     4     32
  *  -------------------------------------------------