On Wed, Nov 26, 2014 at 2:43 PM, Jarno Rajahalme <[email protected]> wrote:
> On Nov 25, 2014, at 7:33 PM, Jesse Gross <[email protected]> wrote:
> On Fri, Sep 5, 2014 at 4:05 PM, Jarno Rajahalme <[email protected]>
> wrote:
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 43ca2a0..243b672 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6 *key,
> + const struct ovs_key_ipv6 *mask)
> {
> @@ -483,38 +525,56 @@ static int set_ipv6(struct sk_buff *skb, const struct
> ovs_key_ipv6 *ipv6_key)
> - if (memcmp(ipv6_key->ipv6_dst, daddr, sizeof(ipv6_key->ipv6_dst))) {
> + mask_ipv6_addr(saddr, key->ipv6_src, mask->ipv6_src,
> masked);
> +
> + if (unlikely(memcmp(saddr, masked, sizeof masked))) {
> + set_ipv6_addr(skb, key->ipv6_proto, saddr, masked,
> + true);
>
>
> set_ipv6_addr() does a memcpy as well, I think that's now unnecessary
> given that we are masking into the packet directly.
>
>
> The above actually masks to a temporary first (‘masked’), then compares the
> ‘masked’ against packet’s address (‘saddr’), and only modifies the packet if
> needed, avoiding checksum update and skb_clear_hash in the common case
> (recall that the userspace currently will ‘set’ all fields that are being
> matched as a side effect of using flow wildcards mask for keeping track of
> both matches and sets).
>
> The common case could be faster if I used a masked compare first, and mask
> the address and call set_ipv6_addr(). inet_proto_csum_replace16() currently
> needs full old and new addresses so I see no obvious way to masking in-place
> without additional copying.
You're right - I was thinking that mask_ipv6_addr() worked slightly
differently than it actually does.
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 69d1919..ec32a00 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1682,12 +1743,45 @@ static int validate_set(const struct nlattr *a,
> + /* Convert non-masked non-tunnel set actions to masked set actions.
> */
> + if (!masked && key_type != OVS_KEY_ATTR_TUNNEL) {
> + int start, len = key_len * 2;
> + struct nlattr *at;
> +
> + *skip_copy = true;
> +
> + start = add_nested_action_start(sfa,
> OVS_ACTION_ATTR_SET_MASKED);
> + if (start < 0)
> + return start;
> +
> + at = __add_action(sfa, key_type, NULL, len);
> + if (IS_ERR(at))
> + return PTR_ERR(at);
> +
> + memcpy(nla_data(at), nla_data(ovs_key), key_len); /* Key. */
> + /* Even though all-ones mask includes non-writeable fields,
> + * which we do not allow above, we will not actually set
> them
> + * when we execute the masked set action. */
> + memset(nla_data(at) + key_len, 0xff, key_len); /* Mask.
> */
>
>
> What about IPv6 flow label? I think we need to actually unmask those
> bits since we want to retain the original value in the packet instead
> of zeroing them.
>
>
> Right, my comment above was wrong regarding the bits 21-24. How about this
> incremental:
>
> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 0fc4c61..0363dc2 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1802,11 +1802,13 @@ static int validate_set(const struct nlattr *a,
> return PTR_ERR(at);
>
> memcpy(nla_data(at), nla_data(ovs_key), key_len); /* Key. */
> - /* Even though all-ones mask includes non-writeable fields,
> - * which we do not allow above, we will not actually set them
> - * when we execute the masked set action. */
> memset(nla_data(at) + key_len, 0xff, key_len); /* Mask. */
> + /* Clear non-writeable bits from otherwise writeable fields. */
> + if (key_type == OVS_KEY_ATTR_IPV6) {
> + struct ovs_key_ipv6 *mask = nla_data(at) + key_len;
>
> + mask->ipv6_label &= htonl(0x000FFFFF);
> + }
> add_nested_action_end(*sfa, start);
> } else if (masked) {
> /* Reject fully masked masked set actions so that the above
I'm a little worried about forgetting to update this with new fields
(probably it if it was in the code where we check the invariants it
would be more obvious) but it's probably good enough for now.
> + } else if (masked) {
> + /* Reject fully masked masked set actions so that the above
> + * conversion can be unabiguously reverted when sending
> + * actions back to userspace. */
> + if (is_all_ones(nla_data(ovs_key) + key_len, key_len))
> + return -EINVAL;
>
>
> It kind of pains me to have these types of exceptions from the
> perspective of the new protocol that we are introducing. Is there any
> way to achieve this effect without introducing a user-visible
> interface quirk?
>
>
> We could have another, kernel internal action code (e.g.,
> OVS_ACTION_ATTR_SET_TO_MASKED), that would execute just like
> OVS_ACTION_ATTR_SET_MASKED, but would retain the fact that it was
> translated, so we could translate it back when needed.
That seems like a reasonable solution to me.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
