On Fri, Apr 11, 2014 at 4:29 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Masked set action allows more megaflow wildcarding. Masked set action
> is now supported for all writeable key types, except for the tunnel
> key.
The logic for the tunnel restriction is that it is always cleared at
the start of the pipeline and therefore doesn't make sense to mask it?
I think it is good to avoid exception cases where possible even if it
doesn't necessarily make sense for the new action. For example, if
userspace generates masked actions then it could be convenient to do
that for all actions, even if the mask is always all bits set for
tunnels.
In any case, if we do need to make such a restriction then it should
be flagged at flow installation time, rather than runtime as appears
to be the case now.
> It is not clear whether masked set is useful for skb_priority.
> However, we already use the LSB of pkt_mark for IPSec in tunnels, so
> it might be useful to be able to set individual bits on pkt_mark.
I think it's good to support it for the reasons above.
The other main thing that I noticed is that this results in a fair
amount of code duplication in actions - particularly with things like
checking for appropriate length and updating checksums (and having two
copies of the IPv6 code is really painful). It seems like the ideal
thing to do is to translate non-masked actions into masked at
validation time, although I know that could potentially be a pain.
Otherwise, I also like what you did with SCTP.
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 0b66e7c..7cbd73c 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> @@ -197,21 +228,28 @@ static void set_ipv6_addr(struct sk_buff *skb, u8
> l4_proto,
> __be32 addr[4], const __be32 new_addr[4],
> bool recalculate_csum)
> {
> - if (recalculate_csum)
> + if (likely(recalculate_csum))
> update_ipv6_checksum(skb, l4_proto, addr, new_addr);
I would be somewhat more judicious with the use of likely() - it
doesn't do that much so I'm not sure that it really makes sense to use
it in places where we are essentially guessing what the user will do
or not do. It also makes it more difficult to review the patch because
there are additional extraneous changes (similarly with some of the
renaming).
> @@ -235,22 +301,26 @@ static int set_ipv4(struct sk_buff *skb, const struct
> ovs_key_ipv4 *ipv4_key)
>
> nh = ip_hdr(skb);
>
> - if (ipv4_key->ipv4_src != nh->saddr)
> - set_ip_addr(skb, nh, &nh->saddr, ipv4_key->ipv4_src);
> + if (unlikely(mask->ipv4_src))
> + set_ip_addr(skb, nh, &nh->saddr,
> + key->ipv4_src | (nh->saddr & ~mask->ipv4_src));
>
> - if (ipv4_key->ipv4_dst != nh->daddr)
> - set_ip_addr(skb, nh, &nh->daddr, ipv4_key->ipv4_dst);
> + if (unlikely(mask->ipv4_dst))
> + set_ip_addr(skb, nh, &nh->daddr,
> + key->ipv4_dst | (nh->daddr & ~mask->ipv4_dst));
It might be worthwhile to have a u32 mask function/macro as it seems
to come up a lot (maybe u16 as well for the ports).
> - if (ipv4_key->ipv4_tos != nh->tos)
> - ipv4_change_dsfield(nh, 0, ipv4_key->ipv4_tos);
> + if (mask->ipv4_tos)
> + ipv4_change_dsfield(nh, 0,
> + key->ipv4_tos | (nh->tos &
> ~mask->ipv4_tos));
The second argument of ipv4_change_dsfield() is actually a mask, so we
can simplify this slightly.
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index ea3cb79..ef6b569 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -585,7 +585,13 @@ struct ovs_action_recirc {
> * indicate the new packet contents. This could potentially still be
> * %ETH_P_MPLS if the resulting MPLS label stack is not empty. If there
> * is no MPLS label stack, as determined by ethertype, no action is taken.
> - * @OVS_ACTION_RECIRC: Recirculate within the data path.
> + * @OVS_ACTION_ATTR_RECIRC: Recirculate within the data path.
> + * @OVS_ACTION_ATTR_SET_MASKED: Replaces the contents of an existing header.
> A
> + * nested %OVS_KEY_ATTR_* attribute specifies a header to modify, its value,
> + * and a mask. For every bit set to one in the mask, the corresponding
> header
> + * field bit is set to the one in value, rest of the bits are left unchanged.
I think the last sentence is a little confusing - when I read "to the
one in value", it sounded like the value must be one at first.
> + * These non-significant bits must be passed in as zeroes, though.
The zeroed non-significant bits are in the value, right?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
