Patchwork [2/4] Move execute_set_action to lib/odp-util.c

login
register
mail settings
Submitter Simon Horman
Date April 8, 2013, 6:43 a.m.
Message ID <1365403431-18102-3-git-send-email-horms@verge.net.au>
Download mbox | patch
Permalink /patch/234587/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Simon Horman - April 8, 2013, 6:43 a.m.
Move execute_set_action from lib/dpif-netedev.c to lib/odp-util.c

This is in preparation for using execute_set_action()
in lib/odp-util.c to handle recirculation/

Signed-off-by: Simon Horman <horms@verge.net.au>

---

packet.c might be a better place for execute_set_action()
but I'm unsure if accessing struct ovs_key_ethernet would
lead to a layering violation.

This patch depends on the patch "Add packet recirculation"

v5
* No change

rfc4
* make use of skb_mark

rfc2 - rfc3
* omitted

rfc1
* Initial post

Conflicts:
	lib/dpif-netdev.c
---
 lib/dpif-netdev.c |   76 -----------------------------------------------------
 lib/odp-util.c    |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/odp-util.h    |    3 +++
 3 files changed, 79 insertions(+), 76 deletions(-)
Jesse Gross - April 8, 2013, 8:29 p.m.
On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote:
> Move execute_set_action from lib/dpif-netedev.c to lib/odp-util.c
>
> This is in preparation for using execute_set_action()
> in lib/odp-util.c to handle recirculation/
>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
>
> packet.c might be a better place for execute_set_action()
> but I'm unsure if accessing struct ovs_key_ethernet would
> lead to a layering violation.

I'd be tempted to just put this in it's own file.  As you say, it
doesn't really fit in either of the two existing ones.

> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index e18e109..ad5873c 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2420,3 +2420,79 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
>      commit_set_priority_action(flow, base, odp_actions);
>      commit_set_skb_mark_action(flow, base, odp_actions);
>  }
> +
> +static void
> +dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)

I think this function should be given a more generic name and possibly
moved to packet.c.

> +void
> +execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
> +                   uint32_t *skb_mark)
> +{
> +    enum ovs_key_attr type = nl_attr_type(a);
> +    const struct ovs_key_ipv4 *ipv4_key;
> +    const struct ovs_key_ipv6 *ipv6_key;
> +    const struct ovs_key_tcp *tcp_key;
> +    const struct ovs_key_udp *udp_key;
> +
> +    switch (type) {
> +    case OVS_KEY_ATTR_PRIORITY:
> +    case OVS_KEY_ATTR_TUNNEL:
> +        /* not implemented */
> +        break;

Don't we need to carry this information along as well similar to skb->mark?

Also, is there a reason to not have the code for push/pop actions here as well?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman - April 9, 2013, 3:11 a.m.
On Mon, Apr 08, 2013 at 01:29:52PM -0700, Jesse Gross wrote:
> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote:
> > Move execute_set_action from lib/dpif-netedev.c to lib/odp-util.c
> >
> > This is in preparation for using execute_set_action()
> > in lib/odp-util.c to handle recirculation/
> >
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> >
> > packet.c might be a better place for execute_set_action()
> > but I'm unsure if accessing struct ovs_key_ethernet would
> > lead to a layering violation.
> 
> I'd be tempted to just put this in it's own file.  As you say, it
> doesn't really fit in either of the two existing ones.

perhaps execute-action.c ?

> 
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index e18e109..ad5873c 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -2420,3 +2420,79 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
> >      commit_set_priority_action(flow, base, odp_actions);
> >      commit_set_skb_mark_action(flow, base, odp_actions);
> >  }
> > +
> > +static void
> > +dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)
> 
> I think this function should be given a more generic name and possibly
> moved to packet.c.

Sure, how about eth_set_src_and_dst()

> > +void
> > +execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
> > +                   uint32_t *skb_mark)
> > +{
> > +    enum ovs_key_attr type = nl_attr_type(a);
> > +    const struct ovs_key_ipv4 *ipv4_key;
> > +    const struct ovs_key_ipv6 *ipv6_key;
> > +    const struct ovs_key_tcp *tcp_key;
> > +    const struct ovs_key_udp *udp_key;
> > +
> > +    switch (type) {
> > +    case OVS_KEY_ATTR_PRIORITY:
> > +    case OVS_KEY_ATTR_TUNNEL:
> > +        /* not implemented */
> > +        break;
> 
> Don't we need to carry this information along as well similar to skb->mark?

Most likely, sorry for missing that.

> Also, is there a reason to not have the code for push/pop actions here as well?

Good point.

With that in mind perhaps execute_set_or_mpls_action() would
be a good name for the function?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Gross - April 9, 2013, 3:17 p.m.
On Mon, Apr 8, 2013 at 8:11 PM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Apr 08, 2013 at 01:29:52PM -0700, Jesse Gross wrote:
>> On Sun, Apr 7, 2013 at 11:43 PM, Simon Horman <horms@verge.net.au> wrote:
>> > Move execute_set_action from lib/dpif-netedev.c to lib/odp-util.c
>> >
>> > This is in preparation for using execute_set_action()
>> > in lib/odp-util.c to handle recirculation/
>> >
>> > Signed-off-by: Simon Horman <horms@verge.net.au>
>> >
>> > ---
>> >
>> > packet.c might be a better place for execute_set_action()
>> > but I'm unsure if accessing struct ovs_key_ethernet would
>> > lead to a layering violation.
>>
>> I'd be tempted to just put this in it's own file.  As you say, it
>> doesn't really fit in either of the two existing ones.
>
> perhaps execute-action.c ?

Sure.

>>
>> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> > index e18e109..ad5873c 100644
>> > --- a/lib/odp-util.c
>> > +++ b/lib/odp-util.c
>> > @@ -2420,3 +2420,79 @@ commit_odp_actions(const struct flow *flow, struct flow *base,
>> >      commit_set_priority_action(flow, base, odp_actions);
>> >      commit_set_skb_mark_action(flow, base, odp_actions);
>> >  }
>> > +
>> > +static void
>> > +dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)
>>
>> I think this function should be given a more generic name and possibly
>> moved to packet.c.
>
> Sure, how about eth_set_src_and_dst()

That sounds fine.

>> > +void
>> > +execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
>> > +                   uint32_t *skb_mark)
>> > +{
>> > +    enum ovs_key_attr type = nl_attr_type(a);
>> > +    const struct ovs_key_ipv4 *ipv4_key;
>> > +    const struct ovs_key_ipv6 *ipv6_key;
>> > +    const struct ovs_key_tcp *tcp_key;
>> > +    const struct ovs_key_udp *udp_key;
>> > +
>> > +    switch (type) {
>> > +    case OVS_KEY_ATTR_PRIORITY:
>> > +    case OVS_KEY_ATTR_TUNNEL:
>> > +        /* not implemented */
>> > +        break;
>>
>> Don't we need to carry this information along as well similar to skb->mark?
>
> Most likely, sorry for missing that.
>
>> Also, is there a reason to not have the code for push/pop actions here as well?
>
> Good point.
>
> With that in mind perhaps execute_set_or_mpls_action() would
> be a good name for the function?

I'm not sure that this is specific to MPLS.  Won't we basically just
have the execute loop from dpif-netdev.c here?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 31255f6..e698e1e 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1115,15 +1115,6 @@  dpif_netdev_wait(struct dpif *dpif)
 }
 
 static void
-dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)
-{
-    struct eth_header *eh = packet->l2;
-
-    memcpy(eh->eth_src, eth_key->eth_src, sizeof eh->eth_src);
-    memcpy(eh->eth_dst, eth_key->eth_dst, sizeof eh->eth_dst);
-}
-
-static void
 dp_netdev_output_port(struct dp_netdev *dp, struct ofpbuf *packet,
                       uint32_t out_port)
 {
@@ -1228,73 +1219,6 @@  dp_netdev_action_userspace(struct dp_netdev *dp,
     dp_netdev_output_userspace(dp, packet, DPIF_UC_ACTION, key, userdata);
 }
 
-static void
-execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
-                   uint32_t *skb_mark)
-{
-    enum ovs_key_attr type = nl_attr_type(a);
-    const struct ovs_key_ipv4 *ipv4_key;
-    const struct ovs_key_ipv6 *ipv6_key;
-    const struct ovs_key_tcp *tcp_key;
-    const struct ovs_key_udp *udp_key;
-
-    switch (type) {
-    case OVS_KEY_ATTR_PRIORITY:
-    case OVS_KEY_ATTR_TUNNEL:
-        /* not implemented */
-        break;
-
-    case OVS_KEY_ATTR_SKB_MARK:
-        *skb_mark = nl_attr_get_u32(a);
-        break;
-
-    case OVS_KEY_ATTR_ETHERNET:
-        dp_netdev_set_dl(packet,
-                   nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));
-        break;
-
-    case OVS_KEY_ATTR_IPV4:
-        ipv4_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_ipv4));
-        packet_set_ipv4(packet, ipv4_key->ipv4_src, ipv4_key->ipv4_dst,
-                        ipv4_key->ipv4_tos, ipv4_key->ipv4_ttl);
-        break;
-
-    case OVS_KEY_ATTR_IPV6:
-        ipv6_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_ipv6));
-        packet_set_ipv6(packet, ipv6_key->ipv6_proto, ipv6_key->ipv6_src,
-                        ipv6_key->ipv6_dst, ipv6_key->ipv6_tclass,
-                        ipv6_key->ipv6_label, ipv6_key->ipv6_hlimit);
-        break;
-
-    case OVS_KEY_ATTR_TCP:
-        tcp_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_tcp));
-        packet_set_tcp_port(packet, tcp_key->tcp_src, tcp_key->tcp_dst);
-        break;
-
-     case OVS_KEY_ATTR_UDP:
-        udp_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_udp));
-        packet_set_udp_port(packet, udp_key->udp_src, udp_key->udp_dst);
-        break;
-
-     case OVS_KEY_ATTR_MPLS:
-         set_mpls_lse(packet, nl_attr_get_be32(a));
-         break;
-
-     case OVS_KEY_ATTR_UNSPEC:
-     case OVS_KEY_ATTR_ENCAP:
-     case OVS_KEY_ATTR_ETHERTYPE:
-     case OVS_KEY_ATTR_IN_PORT:
-     case OVS_KEY_ATTR_VLAN:
-     case OVS_KEY_ATTR_ICMP:
-     case OVS_KEY_ATTR_ICMPV6:
-     case OVS_KEY_ATTR_ARP:
-     case OVS_KEY_ATTR_ND:
-     case __OVS_KEY_ATTR_MAX:
-     default:
-        NOT_REACHED();
-    }
-}
-
 static bool
 dp_netdev_execute_actions(struct dp_netdev *dp,
                           struct ofpbuf *packet, struct flow *key,
diff --git a/lib/odp-util.c b/lib/odp-util.c
index e18e109..ad5873c 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2420,3 +2420,79 @@  commit_odp_actions(const struct flow *flow, struct flow *base,
     commit_set_priority_action(flow, base, odp_actions);
     commit_set_skb_mark_action(flow, base, odp_actions);
 }
+
+static void
+dp_netdev_set_dl(struct ofpbuf *packet, const struct ovs_key_ethernet *eth_key)
+{
+    struct eth_header *eh = packet->l2;
+
+    memcpy(eh->eth_src, eth_key->eth_src, sizeof eh->eth_src);
+    memcpy(eh->eth_dst, eth_key->eth_dst, sizeof eh->eth_dst);
+}
+
+void
+execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
+                   uint32_t *skb_mark)
+{
+    enum ovs_key_attr type = nl_attr_type(a);
+    const struct ovs_key_ipv4 *ipv4_key;
+    const struct ovs_key_ipv6 *ipv6_key;
+    const struct ovs_key_tcp *tcp_key;
+    const struct ovs_key_udp *udp_key;
+
+    switch (type) {
+    case OVS_KEY_ATTR_PRIORITY:
+    case OVS_KEY_ATTR_TUNNEL:
+        /* not implemented */
+        break;
+
+    case OVS_KEY_ATTR_SKB_MARK:
+        *skb_mark = nl_attr_get_u32(a);
+        break;
+
+    case OVS_KEY_ATTR_ETHERNET:
+        dp_netdev_set_dl(packet,
+                   nl_attr_get_unspec(a, sizeof(struct ovs_key_ethernet)));
+        break;
+
+    case OVS_KEY_ATTR_IPV4:
+        ipv4_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_ipv4));
+        packet_set_ipv4(packet, ipv4_key->ipv4_src, ipv4_key->ipv4_dst,
+                        ipv4_key->ipv4_tos, ipv4_key->ipv4_ttl);
+        break;
+
+    case OVS_KEY_ATTR_IPV6:
+        ipv6_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_ipv6));
+        packet_set_ipv6(packet, ipv6_key->ipv6_proto, ipv6_key->ipv6_src,
+                        ipv6_key->ipv6_dst, ipv6_key->ipv6_tclass,
+                        ipv6_key->ipv6_label, ipv6_key->ipv6_hlimit);
+        break;
+
+    case OVS_KEY_ATTR_TCP:
+        tcp_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_tcp));
+        packet_set_tcp_port(packet, tcp_key->tcp_src, tcp_key->tcp_dst);
+        break;
+
+     case OVS_KEY_ATTR_UDP:
+        udp_key = nl_attr_get_unspec(a, sizeof(struct ovs_key_udp));
+        packet_set_udp_port(packet, udp_key->udp_src, udp_key->udp_dst);
+        break;
+
+     case OVS_KEY_ATTR_MPLS:
+         set_mpls_lse(packet, nl_attr_get_be32(a));
+         break;
+
+     case OVS_KEY_ATTR_UNSPEC:
+     case OVS_KEY_ATTR_ENCAP:
+     case OVS_KEY_ATTR_ETHERTYPE:
+     case OVS_KEY_ATTR_IN_PORT:
+     case OVS_KEY_ATTR_VLAN:
+     case OVS_KEY_ATTR_ICMP:
+     case OVS_KEY_ATTR_ICMPV6:
+     case OVS_KEY_ATTR_ARP:
+     case OVS_KEY_ATTR_ND:
+     case __OVS_KEY_ATTR_MAX:
+     default:
+        NOT_REACHED();
+    }
+}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index da62aa5..637d6a5 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -159,6 +159,9 @@  void odp_put_tunnel_action(const struct flow_tnl *tunnel,
 void odp_put_skb_mark_action(const uint32_t skb_mark,
                              struct ofpbuf *odp_actions);
 
+void execute_set_action(struct ofpbuf *packet, const struct nlattr *a,
+                        uint32_t *skb_mark);
+
 /* Reasons why a subfacet might not be fast-pathable. */
 enum slow_path_reason {
     /* These reasons are mutually exclusive. */