[ovs-dev] [PATCH] datapath: add skb mark matching and set action

[Jesse Gross]

On Tue, Nov 13, 2012 at 9:52 AM, Ansis Atteka <aatteka at nicira.com> wrote:

> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index e359ac0..44828c6 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -590,6 +590,7 @@ static int validate_set(const struct nlattr *a,
>         const struct ovs_key_ipv4_tunnel *tun_key;
>         const struct ovs_key_ipv6 *ipv6_key;
>
> +       case OVS_KEY_ATTR_SKB_MARK:
>         case OVS_KEY_ATTR_PRIORITY:
>         case OVS_KEY_ATTR_TUN_ID:
>         case OVS_KEY_ATTR_ETHERNET:
>

Father down in this file, in ovs_packet_cmd_execute(), we should also mark
the skb with anything that we receive in the metadata.


> diff --git a/datapath/flow.h b/datapath/flow.h
> index 54f0fcd..fb69ac5 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -45,6 +45,7 @@ struct sw_flow_key {
>         struct {
>                 u32     priority;       /* Packet QoS priority. */
>                 u16     in_port;        /* Input switch port (or
> DP_MAX_PORTS). */
> +               u32     skb_mark;       /* SKB mark. */
>

Can you move skb_mark up by one line so that we order struct members by
size so all holes are at the bottom?


> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index c4823d9..9898f6e 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -281,6 +281,7 @@ enum ovs_key_attr {
>         OVS_KEY_ATTR_ARP,       /* struct ovs_key_arp */
>         OVS_KEY_ATTR_ND,        /* struct ovs_key_nd */
>         OVS_KEY_ATTR_IPV4_TUNNEL,  /* struct ovs_key_ipv4_tunnel */
> +       OVS_KEY_ATTR_SKB_MARK,  /* u32 skb->mark */
>

Let's put this before IPV4_TUNNEL.  Nothing has shipped yet with these two
values and if we make this one next then I can sent it upstream immediately.


> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 08823e2..3d0a1c7 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -93,6 +93,7 @@ ovs_key_attr_to_string(enum ovs_key_attr attr)
>      case OVS_KEY_ATTR_UNSPEC: return "unspec";
>      case OVS_KEY_ATTR_ENCAP: return "encap";
>      case OVS_KEY_ATTR_PRIORITY: return "priority";
> +    case OVS_KEY_ATTR_SKB_MARK: return "skb_mark";
>      case OVS_KEY_ATTR_TUN_ID: return "tun_id";
>      case OVS_KEY_ATTR_IPV4_TUNNEL: return "ipv4_tunnel";
>      case OVS_KEY_ATTR_IN_PORT: return "in_port";
> @@ -602,6 +603,7 @@ odp_flow_key_attr_len(uint16_t type)
>      switch ((enum ovs_key_attr) type) {
>      case OVS_KEY_ATTR_ENCAP: return -2;
>      case OVS_KEY_ATTR_PRIORITY: return 4;
> +    case OVS_KEY_ATTR_SKB_MARK: return 4;
>      case OVS_KEY_ATTR_TUN_ID: return 8;
>      case OVS_KEY_ATTR_IPV4_TUNNEL: return sizeof(struct
> ovs_key_ipv4_tunnel);
>      case OVS_KEY_ATTR_IN_PORT: return 4;
> @@ -813,6 +815,7 @@ format_odp_key_attr(const struct nlattr *a, struct ds
> *ds)
>          break;
>      }
>
> +    case OVS_KEY_ATTR_SKB_MARK:
>      case OVS_KEY_ATTR_UNSPEC:
>      case __OVS_KEY_ATTR_MAX:


We should probably define a normal method for printing out skb_mark rather
than relying on the default generic one.

Can you also plumb skb_mark through userspace a little more?  We don't need
to be able to match on it using OpenFlow but internally we should be able
to store the value in the flow and translate to and from the kernel.  This
should be very similar to the code for skb_priority (excluding any logic
that is actually related to QoS priorities, of course).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121113/c2d38d80/attachment-0003.html>