[ovs-dev] [PATCH] datapath: Refactor actions in terms of match fields.

Wed Oct 19 00:50:58 UTC 2011

On Tue, Oct 18, 2011 at 3:47 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Thu, Oct 13, 2011 at 3:23 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> Almost all current actions can be expressed in the form of
>> push/pop/set <field>, where field is one of the match fields. We can
>> create three base actions and take a field. This has both a nice
>> symmetry and avoids inconsistencies where we can match on the vlan
>> TPID but not set it.
>> Following patch converts all actions to this new format.
>>
>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>> Bug #7115
>
> Overall, this looks pretty good to me.  I have a few miscellaneous
> comments but in general I like the structure of it.  Ben, can you take
> a look at it as well (although you might as well wait until Pravin has
> had a chance to address my comments here)?
>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index a28e986..c348daa 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -99,6 +99,8 @@ static int pop_vlan(struct sk_buff *skb)
>>
>>  static int push_vlan(struct sk_buff *skb, __be16 new_tci)
>>  {
>> +       new_tci &= ~htons(VLAN_CFI_MASK);
>
> It's better to use VLAN_TAG_PRESENT instead of VLAN_CFI_MASK.  They
> have the same value but the former better indicates why you're doing
> it.
>
> However, looking at the flow setup code more closely, I now see that I
> misled you here - we actually don't set VLAN_TAG_PRESENT in the
> communication between userspace and kernel - it's instead implied by
> the presence of the Netlink attribute.  Both sides use this convention
> internally but it is masked out when serializing the data.
>
> Given that, I would mask it out in userspace and then add a check to
> the validation code for it.
>
ok,
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index b3e2442..f520502 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> +static int validate_set_actions(const struct nlattr *ovs_key,
>> +               const struct sw_flow_key *flow_key)
>> +{
> [...]
>> +       default:
>> +               return -EOPNOTSUPP;
>
> I think it's better to just return -EINVAL here (and in the main
> validation_actions() function as well).  Otherwise you end up with
> different error codes when trying to set an IPv4 protocol type vs. an
> IPv6 type.
>
ok

>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index c077f62..2bc7a00 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> +/**
>> + * OVS datapath actions are expressed using enum ovs_action_type and enum
>> + * ovs_key_type.  enum ovs_action_type is like base action and ovs_key_type
>> + * specifies which field to act on.  i.e. push/pop/set <field>.
>> + * E.g. @OVS_ACTION_ATTR_SET could have nested netlink attribute of type
>> + * %OVS_KEY_ATTR_TUN_ID, %OVS_KEY_ATTR_ETHERNET, %OVS_KEY_ATTR_IPV4,
>> + * %OVS_KEY_ATTR_TCP or %OVS_KEY_ATTR_UDP.
>> + */
>
> I found this comment somewhat confusing to read.  Maybe you could just
> give a single example of what an action would look like instead of
> talking about the two pieces separately?
>
ok
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 9967322..05f72e3 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> +dp_netdev_set_udp_port(struct ofpbuf *packet, const struct flow *flow,
>> +                       const struct ovs_key_udp *udp_key)
>> +{
>> +    /* Check for trasport header. */
>> +    if (packet->l7 && flow->nw_proto == IPPROTO_UDP) {
>
> I don't think any of the tests of this form (with the exception of the
> pseudoheader changing when the IP address changes) are necessary
> because the main userspace should be able to tell whether the packet
> headers are valid or not.  If it doesn't then when used with the
> kernel module the flows will be rejected.
>
ok

>> +static void
>>  dp_netdev_execute_actions(struct dp_netdev *dp,
>>                           struct ofpbuf *packet, struct flow *key,
>>                           const struct nlattr *actions,
> [...]
>> -        case OVS_ACTION_ATTR_SET_TUNNEL:
>
> You need to still include this in execute_set_action() as a no-op
> because it's still possible for the set_tunnel OpenFlow command to be
> used in this case.  When used without a tunnel (as is always the case
> here), there's no effect but after this patch it will cause an
> assertion failure.

right, i missed it.
>

>> diff --git a/lib/netlink.c b/lib/netlink.c
>> index b4de3ed..f90a0de 100644
>> --- a/lib/netlink.c
>> +++ b/lib/netlink.c
>> @@ -333,6 +333,18 @@ nl_msg_put_nested(struct ofpbuf *msg,
>>     nl_msg_end_nested(msg, offset);
>>  }
>>
>> +void
>> +nl_msg_put_nested_attr(struct ofpbuf *odp_actions, uint16_t type,
>> +                       uint16_t n_type, const void *n_data, size_t n_size)
>
> Where does the n_ prefix come from?  It's somewhat confusing because
> it's usually used for number of X.  I think this function doesn't
> really belong in netlink.c since it's not generic code but is actually
> specific to inserting actions.
>
ok, I will move it to ofproo-dpif.

>> +    void *attr;
>> +
>> +    attr = nl_msg_put_unspec_uninit(odp_actions, n_type, n_size);
>> +    memcpy(attr, n_data, n_size);
>
> I think you can use nl_msg_put_unspec() in place of these three lines.
>
ok

>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index a471099..042a1d5 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> +static void
>>  format_odp_action(struct ds *ds, const struct nlattr *a)
>>  {
> [...]
>> +    case OVS_ACTION_ATTR_POP:
>> +        if (nl_attr_get_u16(a) == OVS_KEY_ATTR_8021Q)
>> +            ds_put_cstr(ds, "pop_vlan");
>
> For consistency can we use "pop(vlan)"?
>
ok
>> +        else
>> +            ds_put_format(ds, "pop(%"PRIu16")", nl_attr_get_u16(a));
>
> When we format unknown actions in other places we use the prefix "key"
> as in "pop(key%"PRIu16")".
>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 8e5a863..0ada375 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> +static void
>> +commit_set_nw_action(const struct flow *flow, struct flow *base,
>> +                     struct ofpbuf *odp_actions)
>> +{
>> +    struct ovs_key_ipv4 ipv4_key;
>>
>> -    if (base->nw_dst != flow->nw_dst) {
>> -        nl_msg_put_be32(odp_actions, OVS_ACTION_ATTR_SET_NW_DST, flow->nw_dst);
>> -        base->nw_dst = flow->nw_dst;
>> +    if (flow->dl_type != htons(ETH_TYPE_IP) ||
>> +        !flow->nw_src || !flow->nw_src ) {
>
> I think that second comparison should be nw_dst.  There's also an
> extra space before the last parenthesis.
>
>> +static void
>> +commit_set_port_action(const struct flow *flow, struct flow *base,
>> +                       struct ofpbuf *odp_actions)
>> +{
>> +    if (!flow->tp_src || !flow->tp_src) {
>
> Another src || src.
>
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index b5ca08c..13c1435 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -75,7 +75,7 @@ in_port=5 actions=set_tunnel:5
>>  AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>>  AT_CHECK([ovs-appctl -t test-openflowd ofproto/trace br0 'tun_id(0x1),in_port(90),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0),icmp(type=8,code=0)'], [0], [stdout])
>>  AT_CHECK([tail -1 stdout], [0],
>> -  [Datapath actions: set_tunnel(0x1),1,2,set_tunnel(0x3),3,4
>> +  [Datapath actions: set(tun_id(0x1)),1,2,set(tun_id(0x3)),3,4
>
> You also need to update test 387: ofproto-dpif.at:83 ofproto-dpif -
> VLAN handling to account for the changed output.
>
right, vlan test was skipped in my make check.