On Tue, Nov 13, 2012 at 11:55 PM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Nov 13, 2012 at 9:52 AM, Ansis Atteka <aatt...@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?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
