diff mbox

[ovs-dev,2/3] tc: Split IPs and transport layer ports unions in flower struct

Message ID 1501150800-48640-3-git-send-email-roid@mellanox.com
State Superseded
Headers show

Commit Message

Roi Dayan July 27, 2017, 10:19 a.m. UTC 
From: Paul Blakey <paulb@mellanox.com>

Split dst/src_port and ipv4/ipv6 union so we can
distingush them easily for later features.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-tc-offloads.c | 29 +++++++++++++++++++++--------
 lib/tc.c                 | 24 ++++++++++++------------
 lib/tc.h                 | 25 +++++++++++++------------
 3 files changed, 46 insertions(+), 32 deletions(-)

Comments

Simon Horman July 28, 2017, 12:49 p.m. UTC | #1 
On Thu, Jul 27, 2017 at 01:19:59PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> Split dst/src_port and ipv4/ipv6 union so we can
> distingush them easily for later features.

The implications of this change on the size of struct tc_flower_key
seem somewhat undesirable to me. Perhaps there is another way to
distinguish between protocols?

> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
Roi Dayan July 30, 2017, 7:22 a.m. UTC | #2 
On 28/07/2017 15:49, Simon Horman wrote:
> On Thu, Jul 27, 2017 at 01:19:59PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> Split dst/src_port and ipv4/ipv6 union so we can
>> distingush them easily for later features.
>
> The implications of this change on the size of struct tc_flower_key
> seem somewhat undesirable to me. Perhaps there is another way to
> distinguish between protocols?
>

the intention here was to ease implementation later of ovs action set
offloading and make it generic.
we'll take a look again on this and update.
as a general question why the size change of tc_flower_key matters in
this case?

>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
Paul Blakey July 30, 2017, 5 p.m. UTC | #3 
On 30/07/2017 10:22, Roi Dayan wrote:
> 
> 
> On 28/07/2017 15:49, Simon Horman wrote:
>> On Thu, Jul 27, 2017 at 01:19:59PM +0300, Roi Dayan wrote:
>>> From: Paul Blakey <paulb@mellanox.com>
>>>
>>> Split dst/src_port and ipv4/ipv6 union so we can
>>> distingush them easily for later features.
>>
>> The implications of this change on the size of struct tc_flower_key
>> seem somewhat undesirable to me. Perhaps there is another way to
>> distinguish between protocols?
>>
> 
> the intention here was to ease implementation later of ovs action set
> offloading and make it generic.
> we'll take a look again on this and update.
> as a general question why the size change of tc_flower_key matters in
> this case?
> 
>>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>>> Reviewed-by: Roi Dayan <roid@mellanox.com>

Hi, Yes and to add to that,
We split those so in the header rewrite (supporting ovs action set using 
tc pedit action) patches that will follow, we can use a map that 
translates flower key offsets to tc action pedit offsets.
Without this change we can't know just by looking at offsetof(struct 
tc_flower_key.ipv4.ipv4_src) if it was set because ipv6_src was set 
(union) or ipv4_src was set, we would have to look at the protocol as 
well. This can be done in a map by specifying the protocol but it 
complicates things.
Besides flower struct is allocated on the stack in 
netdev_tc_offloads_flow_put()



We have a map like so:
+struct tc_pedit_key_ex {
+    enum pedit_header_type htype;
+    enum pedit_cmd cmd;
+};
+
+struct flower_key_to_pedit {
+    enum pedit_header_type htype;
+    int offset;
+    int size;
+};
+
+static struct flower_key_to_pedit flower_pedit_map[] = {
+    [offsetof(struct tc_flower_key, ipv4.ipv4_src)] = {
+        TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+        12,
+        MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
+    },
+    [offsetof(struct tc_flower_key, ipv4.ipv4_dst)] = {
+        TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+        16,
+        MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
+    },
+    [offsetof(struct tc_flower_key, ipv4.rewrite_ttl)] = {
+        TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
+        8,
+        MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
+    },
....
diff mbox

Patch

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index 6214023..e2aea60 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -319,8 +319,15 @@  parse_tc_flower_to_match(struct tc_flower *flower,
     match_set_ipv6_dst_masked(match,
                               &key->ipv6.ipv6_dst, &mask->ipv6.ipv6_dst);
 
-    match_set_tp_dst_masked(match, key->dst_port, mask->dst_port);
-    match_set_tp_src_masked(match, key->src_port, mask->src_port);
+    if (is_ip_any(&match->flow)) {
+        if (key->ip_proto == IPPROTO_TCP) {
+            match_set_tp_dst_masked(match, key->tcp_dst, mask->tcp_dst);
+            match_set_tp_src_masked(match, key->tcp_src, mask->tcp_src);
+        } else if (key->ip_proto == IPPROTO_UDP) {
+            match_set_tp_dst_masked(match, key->udp_dst, mask->udp_dst);
+            match_set_tp_src_masked(match, key->udp_src, mask->udp_src);
+        }
+    }
 
     if (flower->tunnel.tunnel) {
         match_set_tun_id(match, flower->tunnel.id);
@@ -747,12 +754,18 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     if (is_ip_any(key)) {
         flower.key.ip_proto = key->nw_proto;
         flower.mask.ip_proto = mask->nw_proto;
-
-        if (key->nw_proto == IPPROTO_TCP || key->nw_proto == IPPROTO_UDP) {
-            flower.key.dst_port = key->tp_dst;
-            flower.mask.dst_port = mask->tp_dst;
-            flower.key.src_port = key->tp_src;
-            flower.mask.src_port = mask->tp_src;
+        if (key->nw_proto == IPPROTO_TCP) {
+            flower.key.tcp_dst = key->tp_dst;
+            flower.mask.tcp_dst = mask->tp_dst;
+            flower.key.tcp_src = key->tp_src;
+            flower.mask.tcp_src = mask->tp_src;
+            mask->tp_src = 0;
+            mask->tp_dst = 0;
+        } else if (key->nw_proto == IPPROTO_UDP) {
+            flower.key.udp_dst = key->tp_dst;
+            flower.mask.udp_dst = mask->tp_dst;
+            flower.key.udp_src = key->tp_src;
+            flower.mask.udp_src = mask->tp_src;
             mask->tp_src = 0;
             mask->tp_dst = 0;
         }
diff --git a/lib/tc.c b/lib/tc.c
index 563aba4..d724d8a 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -305,26 +305,26 @@  nl_parse_flower_ip(struct nlattr **attrs, struct tc_flower *flower) {
 
     if (ip_proto == IPPROTO_TCP) {
         if (attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]) {
-            key->src_port =
+            key->tcp_src =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC]);
-            mask->src_port =
+            mask->tcp_src =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]);
         }
         if (attrs[TCA_FLOWER_KEY_TCP_DST_MASK]) {
-            key->dst_port =
+            key->tcp_dst =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST]);
-            mask->dst_port =
+            mask->tcp_dst =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST_MASK]);
         }
     } else if (ip_proto == IPPROTO_UDP) {
         if (attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]) {
-            key->src_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]);
-            mask->src_port =
+            key->udp_src = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]);
+            mask->udp_src =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]);
         }
         if (attrs[TCA_FLOWER_KEY_UDP_DST_MASK]) {
-            key->dst_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]);
-            mask->dst_port =
+            key->udp_dst = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]);
+            mask->udp_dst =
                 nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST_MASK]);
         }
     }
@@ -994,11 +994,11 @@  nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
         }
 
         if (flower->key.ip_proto == IPPROTO_UDP) {
-            FLOWER_PUT_MASKED_VALUE(src_port, TCA_FLOWER_KEY_UDP_SRC);
-            FLOWER_PUT_MASKED_VALUE(dst_port, TCA_FLOWER_KEY_UDP_DST);
+            FLOWER_PUT_MASKED_VALUE(udp_src, TCA_FLOWER_KEY_UDP_SRC);
+            FLOWER_PUT_MASKED_VALUE(udp_dst, TCA_FLOWER_KEY_UDP_DST);
         } else if (flower->key.ip_proto == IPPROTO_TCP) {
-            FLOWER_PUT_MASKED_VALUE(src_port, TCA_FLOWER_KEY_TCP_SRC);
-            FLOWER_PUT_MASKED_VALUE(dst_port, TCA_FLOWER_KEY_TCP_DST);
+            FLOWER_PUT_MASKED_VALUE(tcp_src, TCA_FLOWER_KEY_TCP_SRC);
+            FLOWER_PUT_MASKED_VALUE(tcp_dst, TCA_FLOWER_KEY_TCP_DST);
         }
     }
 
diff --git a/lib/tc.h b/lib/tc.h
index 1cc7362..571ec9f 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -76,24 +76,25 @@  struct tc_flower_key {
     struct eth_addr dst_mac;
     struct eth_addr src_mac;
 
-    ovs_be16 src_port;
-    ovs_be16 dst_port;
+    ovs_be16 tcp_src;
+    ovs_be16 tcp_dst;
+
+    ovs_be16 udp_src;
+    ovs_be16 udp_dst;
 
     uint16_t vlan_id;
     uint8_t vlan_prio;
 
     ovs_be16 encap_eth_type;
 
-    union {
-        struct {
-            ovs_be32 ipv4_src;
-            ovs_be32 ipv4_dst;
-        } ipv4;
-        struct {
-            struct in6_addr ipv6_src;
-            struct in6_addr ipv6_dst;
-        } ipv6;
-    };
+    struct {
+        ovs_be32 ipv4_src;
+        ovs_be32 ipv4_dst;
+    } ipv4;
+    struct {
+        struct in6_addr ipv6_src;
+        struct in6_addr ipv6_dst;
+    } ipv6;
 };
 
 struct tc_flower {