On Thu, Nov 15, 2012 at 11:07 AM, Ansis Atteka <aatt...@nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index e359ac0..778f8e3 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -595,6 +595,12 @@ static int validate_set(const struct nlattr *a,
> case OVS_KEY_ATTR_ETHERNET:
> break;
>
> + case OVS_KEY_ATTR_SKB_MARK:
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20) &&
> !defined(CONFIG_NETFILTER)
> + return -EINVAL;
> +#endif
+ break;
>
We could add the additional constraint that we only reject if the mark is
not zero.
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 41c624e..d82fa9c 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1024,6 +1026,10 @@ int ovs_flow_from_nlattrs(struct sw_flow_key
> *swkey, int *key_lenp,
> } else {
> swkey->phy.in_port = DP_MAX_PORTS;
> }
> + if (attrs & (1 << OVS_KEY_ATTR_SKB_MARK)) {
> + swkey->phy.skb_mark =
> nla_get_u32(a[OVS_KEY_ATTR_SKB_MARK]);
> + attrs &= ~(1 << OVS_KEY_ATTR_SKB_MARK);
> + }
>
> if (attrs & (1ULL << OVS_KEY_ATTR_TUN_ID) &&
> attrs & (1ULL << OVS_KEY_ATTR_IPV4_TUNNEL)) {
> @@ -1203,6 +1209,7 @@ int ovs_flow_metadata_from_nlattrs(struct sw_flow
> *flow, int key_len, const stru
>
> flow->key.phy.in_port = DP_MAX_PORTS;
> flow->key.phy.priority = 0;
> + flow->key.phy.skb_mark = 0;
> memset(tun_key, 0, sizeof(flow->key.tun_key));
>
> nla_for_each_nested(nla, attr, rem) {
> @@ -1254,6 +1261,10 @@ int ovs_flow_metadata_from_nlattrs(struct sw_flow
> *flow, int key_len, const stru
> return -EINVAL;
> flow->key.phy.in_port = nla_get_u32(nla);
> break;
> +
> + case OVS_KEY_ATTR_SKB_MARK:
> + flow->key.phy.skb_mark = nla_get_u32(nla);
> + break;
> }
> }
> }
>
In these two situations we probably should reject in the same conditions as
in datapath.c (bad kernel version and mark != 0.
> diff --git a/lib/flow.h b/lib/flow.h
> index 5f4b8cb..40a6a98 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -87,6 +87,7 @@ struct flow {
> uint32_t in_port; /* Input port. OpenFlow port number
> unless in DPIF code, in which case it
> is the datapath port number. */
> + uint32_t skb_mark; /* Packet mark. */
> ovs_be16 vlan_tci; /* If 802.1Q, TCI | VLAN_CFI; otherwise
> 0. */
> ovs_be16 dl_type; /* Ethernet frame type. */
> ovs_be16 tp_src; /* TCP/UDP source port. */
> @@ -105,7 +106,7 @@ BUILD_ASSERT_DECL(sizeof(struct flow) % 4 == 0);
> #define FLOW_U32S (sizeof(struct flow) / 4)
>
> /* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
> -BUILD_ASSERT_DECL(sizeof(struct flow) == sizeof(struct flow_tnl) + 144 &&
> +BUILD_ASSERT_DECL(sizeof(struct flow) == sizeof(struct flow_tnl) + 152 &&
> FLOW_WC_SEQ == 17);
>
We should update the userspace version of the Netlink buffer as well in
odp-util.h
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 08823e2..6aedbc2 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -697,6 +699,10 @@ format_odp_key_attr(const struct nlattr *a, struct ds
> *ds)
> ds_put_format(ds, "(%"PRIu32")", nl_attr_get_u32(a));
> break;
>
> + case OVS_KEY_ATTR_SKB_MARK:
> + ds_put_format(ds, "(skb_mark=%"PRIu32")", nl_attr_get_u32(a));
> + break;
>
You shouldn't need the field name here since it is printed automatically,
just the value inside the parentheses. We should also add mark to some of
the unit tests in odp.at to make sure that it can be serialized correctly
(it should have caught this).
> @@ -1302,6 +1318,10 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const
> struct flow *flow,
> nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, flow->skb_priority);
> }
>
> + if (flow->skb_mark) {
> + nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, flow->skb_mark);
> + }
> +
> if (flow->tunnel.tun_id != htonll(0)) {
> nl_msg_put_be64(buf, OVS_KEY_ATTR_TUN_ID, flow->tunnel.tun_id);
> }
> @@ -1798,6 +1818,11 @@ odp_flow_key_to_flow(const struct nlattr *key,
> size_t key_len,
> expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_PRIORITY;
> }
>
> + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_SKB_MARK)) {
> + flow->skb_mark = nl_attr_get_u32(attrs[OVS_KEY_ATTR_SKB_MARK]);
> + expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_SKB_MARK;
> + }
I think there's a few other places where we need to add the mark. Two that
come to mind are in match.c to clear out the value since we don't allow
OpenFlow matching and in flow_extract() where we initialize the metadata.
That's probably it but I would grep for skb_priority and PRIORITY to find
all the places where the existing priority field is use, this should be
roughly the same.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
