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

Ansis Atteka aatteka at nicira.com
Wed Nov 14 18:52:34 UTC 2012


On Tue, Nov 13, 2012 at 11:55 PM, Jesse Gross <jesse at nicira.com> wrote:
> 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.

Do you mean something like this in the ovs_packet_cmd_execute() function:

	OVS_CB(packet)->flow = flow;
	packet->priority = flow->key.phy.priority;
+	packet->mark = flow->key.phy.mark;


>
>>
>> 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?
ok
>
>>
>> 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.
ok
>
>>
>> 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.
ok

>
> 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).

I guess you meant to add it to the struct flow in lib/flow.h, right?



More information about the dev mailing list