[ovs-dev] [PATCH 1/3] datapath: refactor flow key extract API.

Andy Zhou azhou at nicira.com
Wed Aug 6 06:26:13 UTC 2014


Looks good in general. It would be nice if we can rename "key_extract"
to "flow_key_extract" for consistency with other APIs.

Acked-by: Andy Zhou <azhou at nicira.com>


On Tue, Aug 5, 2014 at 4:46 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
> OVS flow extract is called on packet receive or packet
> execute code path.  Following patch defines separate API
> for extracting flow-key in packet execute code path.
>
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> ---
>  datapath/actions.c      |    3 +-
>  datapath/datapath.c     |   10 ++---
>  datapath/flow.c         |   82 ++++++++++++++++++++++++++++++----------------
>  datapath/flow.h         |    6 +++-
>  datapath/flow_netlink.c |   31 +++++++----------
>  datapath/flow_netlink.h |    4 +-
>  6 files changed, 78 insertions(+), 58 deletions(-)
>
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 792c3a9..5a0bfa9 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -649,11 +649,10 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>                                  const struct nlattr *a)
>  {
>         struct sw_flow_key recirc_key;
> -       const struct vport *p = OVS_CB(skb)->input_vport;
>         uint32_t hash = OVS_CB(skb)->pkt_key->ovs_flow_hash;
>         int err;
>
> -       err = ovs_flow_extract(skb, p->port_no, &recirc_key);
> +       err = ovs_flow_key_extract(skb, &recirc_key);
>         if (err) {
>                 kfree_skb(skb);
>                 return err;
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 1185f60..91c65a0 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -300,7 +300,7 @@ void ovs_dp_process_received_packet(struct sk_buff *skb)
>         struct sw_flow_key key;
>
>         /* Extract flow from 'skb' into 'key'. */
> -       error = ovs_flow_extract(skb, OVS_CB(skb)->input_vport->port_no, &key);
> +       error = ovs_flow_key_extract(skb, &key);
>         if (unlikely(error)) {
>                 kfree_skb(skb);
>                 return;
> @@ -354,7 +354,7 @@ static int queue_gso_packets(struct datapath *dp, struct sk_buff *skb,
>                 return PTR_ERR(segs);
>
>         if (gso_type & SKB_GSO_UDP) {
> -               /* The initial flow key extracted by ovs_flow_extract()
> +               /* The initial flow key extracted by ovs_flow_key_extract()
>                  * in this case is for a first fragment, so we need to
>                  * properly mark later fragments.
>                  */
> @@ -581,13 +581,11 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>         if (IS_ERR(flow))
>                 goto err_kfree_skb;
>
> -       err = ovs_flow_extract(packet, -1, &flow->key);
> +       err = ovs_flow_key_extract_userspace(a[OVS_PACKET_ATTR_KEY], packet,
> +                                            &flow->key);
>         if (err)
>                 goto err_flow_free;
>
> -       err = ovs_nla_get_flow_metadata(flow, a[OVS_PACKET_ATTR_KEY]);
> -       if (err)
> -               goto err_flow_free;
>         acts = ovs_nla_alloc_flow_actions(nla_len(a[OVS_PACKET_ATTR_ACTIONS]));
>         err = PTR_ERR(acts);
>         if (IS_ERR(acts))
> diff --git a/datapath/flow.c b/datapath/flow.c
> index e234796..7fdce6a 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -16,8 +16,6 @@
>   * 02110-1301, USA
>   */
>
> -#include "flow.h"
> -#include "datapath.h"
>  #include <linux/uaccess.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
> @@ -45,6 +43,10 @@
>  #include <net/ipv6.h>
>  #include <net/ndisc.h>
>
> +#include "datapath.h"
> +#include "flow.h"
> +#include "flow_netlink.h"
> +
>  #include "mpls.h"
>  #include "vlan.h"
>
> @@ -425,10 +427,9 @@ invalid:
>  }
>
>  /**
> - * ovs_flow_extract - extracts a flow key from an Ethernet frame.
> + * key_extract - extracts a flow key from an Ethernet frame.
>   * @skb: sk_buff that contains the frame, with skb->data pointing to the
>   * Ethernet header
> - * @in_port: port number on which @skb was received.
>   * @key: output flow key
>   *
>   * The caller must ensure that skb->len >= ETH_HLEN.
> @@ -447,35 +448,11 @@ invalid:
>   *      of a correct length, otherwise the same as skb->network_header.
>   *      For other key->eth.type values it is left untouched.
>   */
> -int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
> +static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>  {
>         int error;
>         struct ethhdr *eth;
>
> -       if (OVS_CB(skb)->tun_info) {
> -               struct ovs_tunnel_info *tun_info = OVS_CB(skb)->tun_info;
> -               memcpy(&key->tun_key, &tun_info->tunnel,
> -                       sizeof(key->tun_key));
> -               if (tun_info->options) {
> -                       BUILD_BUG_ON((1 << (sizeof(tun_info->options_len) * 8)) - 1
> -                                       > sizeof(key->tun_opts));
> -                       memcpy(GENEVE_OPTS(key, tun_info->options_len),
> -                               tun_info->options, tun_info->options_len);
> -                       key->tun_opts_len = tun_info->options_len;
> -               } else {
> -                       key->tun_opts_len = 0;
> -               }
> -       } else {
> -               key->tun_opts_len = 0;
> -               memset(&key->tun_key, 0, sizeof(key->tun_key));
> -       }
> -
> -       key->phy.priority = skb->priority;
> -       key->phy.in_port = in_port;
> -       key->phy.skb_mark = skb->mark;
> -       key->ovs_flow_hash = 0;
> -       key->recirc_id = 0;
> -
>         /* Flags are always used as part of stats. */
>         key->tp.flags = 0;
>
> @@ -694,3 +671,50 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port, struct sw_flow_key *key)
>
>         return 0;
>  }
> +
> +int ovs_flow_key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +       /* Extract metadata from packet. */
> +
> +       if (OVS_CB(skb)->tun_info) {
> +               struct ovs_tunnel_info *tun_info = OVS_CB(skb)->tun_info;
> +
> +               memcpy(&key->tun_key, &tun_info->tunnel, sizeof(key->tun_key));
> +
> +               BUILD_BUG_ON(((1 << (sizeof(tun_info->options_len) * 8)) - 1) >
> +                            sizeof(key->tun_opts));
> +
> +               if (tun_info->options) {
> +                       memcpy(GENEVE_OPTS(key, tun_info->options_len),
> +                               tun_info->options, tun_info->options_len);
> +                       key->tun_opts_len = tun_info->options_len;
> +               } else {
> +                       key->tun_opts_len = 0;
> +               }
> +       } else {
> +               key->tun_opts_len = 0;
> +               memset(&key->tun_key, 0, sizeof(key->tun_key));
> +       }
> +
> +       key->phy.priority = skb->priority;
> +       key->phy.in_port = OVS_CB(skb)->input_vport->port_no;
> +       key->phy.skb_mark = skb->mark;
> +       key->ovs_flow_hash = 0;
> +       key->recirc_id = 0;
> +
> +       return key_extract(skb, key);
> +}
> +
> +int ovs_flow_key_extract_userspace(const struct nlattr *attr,
> +                                  struct sk_buff *skb,
> +                                  struct sw_flow_key *key)
> +{
> +       int err;
> +
> +       /* Extract metadata from netlink attributes. */
> +       err = ovs_nla_get_flow_metadata(attr, key);
> +       if (err)
> +               return err;
> +
> +       return key_extract(skb, key);
> +}
> diff --git a/datapath/flow.h b/datapath/flow.h
> index f6afa48..69664cd 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -217,6 +217,10 @@ void ovs_flow_stats_get(const struct sw_flow *, struct ovs_flow_stats *,
>  void ovs_flow_stats_clear(struct sw_flow *);
>  u64 ovs_flow_used_time(unsigned long flow_jiffies);
>
> -int ovs_flow_extract(struct sk_buff *, u16 in_port, struct sw_flow_key *);
> +int ovs_flow_key_extract(struct sk_buff *skb, struct sw_flow_key *key);
> +/* Extract key from packet coming from userspace. */
> +int ovs_flow_key_extract_userspace(const struct nlattr *attr,
> +                                  struct sk_buff *skb,
> +                                  struct sw_flow_key *key);
>
>  #endif /* flow.h */
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 37646e4..ada21ae 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -992,7 +992,7 @@ free_newmask:
>
>  /**
>   * ovs_nla_get_flow_metadata - parses Netlink attributes into a flow key.
> - * @flow: Receives extracted in_port, priority, tun_key and skb_mark.
> + * @key: Receives extracted in_port, priority, tun_key and skb_mark.
>   * @attr: Netlink attribute holding nested %OVS_KEY_ATTR_* Netlink attribute
>   * sequence.
>   *
> @@ -1001,35 +1001,30 @@ free_newmask:
>   * get the metadata, that is, the parts of the flow key that cannot be
>   * extracted from the packet itself.
>   */
> -
> -int ovs_nla_get_flow_metadata(struct sw_flow *flow,
> -                             const struct nlattr *attr)
> +int ovs_nla_get_flow_metadata(const struct nlattr *attr,
> +                             struct sw_flow_key *key)
>  {
> -       struct ovs_key_ipv4_tunnel *tun_key = &flow->key.tun_key;
>         const struct nlattr *a[OVS_KEY_ATTR_MAX + 1];
> +       struct sw_flow_match match;
>         u64 attrs = 0;
>         int err;
> -       struct sw_flow_match match;
> -
> -       flow->key.phy.in_port = DP_MAX_PORTS;
> -       flow->key.phy.priority = 0;
> -       flow->key.phy.skb_mark = 0;
> -       flow->key.ovs_flow_hash = 0;
> -       flow->key.recirc_id = 0;
> -       memset(tun_key, 0, sizeof(flow->key.tun_key));
>
>         err = parse_flow_nlattrs(attr, a, &attrs);
>         if (err)
>                 return -EINVAL;
>
>         memset(&match, 0, sizeof(match));
> -       match.key = &flow->key;
> +       match.key = key;
>
> -       err = metadata_from_nlattrs(&match, &attrs, a, false);
> -       if (err)
> -               return err;
> +       key->tun_opts_len = 0;
> +       memset(&key->tun_key, 0, sizeof(key->tun_key));
> +       key->phy.priority = 0;
> +       key->phy.skb_mark = 0;
> +       key->phy.in_port = DP_MAX_PORTS;
> +       key->ovs_flow_hash = 0;
> +       key->recirc_id = 0;
>
> -       return 0;
> +       return metadata_from_nlattrs(&match, &attrs, a, false);
>  }
>
>  int ovs_nla_put_flow(struct datapath *dp, const struct sw_flow_key *swkey,
> diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
> index 0c20e86..16da264 100644
> --- a/datapath/flow_netlink.h
> +++ b/datapath/flow_netlink.h
> @@ -42,8 +42,8 @@ void ovs_match_init(struct sw_flow_match *match,
>
>  int ovs_nla_put_flow(struct datapath *dp, const struct sw_flow_key *,
>                      const struct sw_flow_key *, struct sk_buff *);
> -int ovs_nla_get_flow_metadata(struct sw_flow *flow,
> -                             const struct nlattr *attr);
> +int ovs_nla_get_flow_metadata(const struct nlattr *, struct sw_flow_key *);
> +
>  int ovs_nla_get_match(struct sw_flow_match *match,
>                       const struct nlattr *,
>                       const struct nlattr *);
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list