[ovs-dev] [recirc datapath V4 2/5] odp-util: Always generate key/mask pair in netlink for recirc_id
Jarno Rajahalme
jrajahalme at nicira.com
Fri Apr 18 22:19:24 UTC 2014
LGTM,
Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
> On Apr 18, 2014, at 2:32 PM, Andy Zhou <azhou at nicira.com> wrote:
>
> Good point. I agree it looks odd. How about the following incremental:
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 9d49198..8e95c9e 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2481,21 +2481,17 @@ ovs_to_odp_frag_mask(uint8_t nw_frag_mask)
> }
>
> static void
> -odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *data,
> - const struct flow *flow, const struct flow *mask,
> - odp_port_t odp_in_port, size_t max_mpls_depth)
> +odp_flow_key_from_flow__(struct ofpbuf *buf, const struct flow *flow,
> + const struct flow *mask, odp_port_t odp_in_port,
> + size_t max_mpls_depth, bool export_mask)
> {
> - bool is_mask;
> struct ovs_key_ethernet *eth_key;
> size_t encap;
> -
> - /* We assume that if 'data' and 'flow' are not the same, we should
> - * treat 'data' as a mask. */
> - is_mask = (data != flow);
> + const struct flow *data = export_mask ? mask : flow;
>
> nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
>
> - if (flow->tunnel.ip_dst || is_mask) {
> + if (flow->tunnel.ip_dst || export_mask) {
> tun_key_to_attr(buf, &data->tunnel);
> }
>
> @@ -2511,7 +2507,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
>
> /* Add an ingress port attribute if this is a mask or 'odp_in_port'
> * is not the magical value "ODPP_NONE". */
> - if (is_mask || odp_in_port != ODPP_NONE) {
> + if (export_mask || odp_in_port != ODPP_NONE) {
> nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, odp_in_port);
> }
>
> @@ -2521,7 +2517,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
> memcpy(eth_key->eth_dst, data->dl_dst, ETH_ADDR_LEN);
>
> if (flow->vlan_tci != htons(0) || flow->dl_type == htons(ETH_TYPE_VLAN)) {
> - if (is_mask) {
> + if (export_mask) {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
> } else {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, htons(ETH_TYPE_VLAN));
> @@ -2547,7 +2543,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
> * <none> 0xffff Any non-Ethernet II frame (except valid
> * 802.3 SNAP packet with valid eth_type).
> */
> - if (is_mask) {
> + if (export_mask) {
> nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, OVS_BE16_MAX);
> }
> goto unencap;
> @@ -2565,7 +2561,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
> ipv4_key->ipv4_proto = data->nw_proto;
> ipv4_key->ipv4_tos = data->nw_tos;
> ipv4_key->ipv4_ttl = data->nw_ttl;
> - ipv4_key->ipv4_frag = is_mask ? ovs_to_odp_frag_mask(data->nw_frag)
> + ipv4_key->ipv4_frag = export_mask ? ovs_to_odp_frag_mask(data->nw_frag)
> : ovs_to_odp_frag(data->nw_frag);
> } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> struct ovs_key_ipv6 *ipv6_key;
> @@ -2578,7 +2574,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
> ipv6_key->ipv6_proto = data->nw_proto;
> ipv6_key->ipv6_tclass = data->nw_tos;
> ipv6_key->ipv6_hlimit = data->nw_ttl;
> - ipv6_key->ipv6_frag = is_mask ? ovs_to_odp_frag_mask(data->nw_frag)
> + ipv6_key->ipv6_frag = export_mask ? ovs_to_odp_frag_mask(data->nw_frag)
> : ovs_to_odp_frag(data->nw_frag);
> } else if (flow->dl_type == htons(ETH_TYPE_ARP) ||
> flow->dl_type == htons(ETH_TYPE_RARP)) {
> @@ -2650,7 +2646,7 @@ odp_flow_key_from_flow__(struct ofpbuf *buf,
> const struct flow *data,
> if (flow->tp_dst == htons(0) &&
> (flow->tp_src == htons(ND_NEIGHBOR_SOLICIT) ||
> flow->tp_src == htons(ND_NEIGHBOR_ADVERT)) &&
> - (!is_mask || (data->tp_src == htons(0xffff) &&
> + (!export_mask || (data->tp_src == htons(0xffff) &&
> data->tp_dst == htons(0xffff)))) {
>
> struct ovs_key_nd *nd_key;
> @@ -2683,7 +2679,7 @@ void
> odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow,
> const struct flow *mask, odp_port_t odp_in_port)
> {
> - odp_flow_key_from_flow__(buf, flow, flow, mask, odp_in_port, SIZE_MAX);
> + odp_flow_key_from_flow__(buf, flow, mask, odp_in_port, SIZE_MAX, false);
> }
>
> /* Appends a representation of 'mask' as OVS_KEY_ATTR_* attributes to
> @@ -2699,8 +2695,8 @@ odp_flow_key_from_mask(struct ofpbuf *buf, const
> struct flow *mask,
> const struct flow *flow, uint32_t odp_in_port_mask,
> size_t max_mpls_depth)
> {
> - odp_flow_key_from_flow__(buf, mask, flow, mask,
> - u32_to_odp(odp_in_port_mask), max_mpls_depth);
> + odp_flow_key_from_flow__(buf, flow, mask,
> + u32_to_odp(odp_in_port_mask),
> max_mpls_depth, true);
> }
>
> /* Generate ODP flow key from the given packet metadata */
>
>> On Fri, Apr 18, 2014 at 1:19 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>>
>>> On Apr 18, 2014, at 2:50 AM, Andy Zhou <azhou at nicira.com> wrote:
>>> ...
>>> @@ -2681,9 +2681,9 @@ unencap:
>>> * capable of being expanded to allow for that much space. */
>>> void
>>> odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow,
>>> - odp_port_t odp_in_port)
>>> + const struct flow *mask, odp_port_t odp_in_port)
>>> {
>>> - odp_flow_key_from_flow__(buf, flow, flow, odp_in_port, SIZE_MAX);
>>> + odp_flow_key_from_flow__(buf, flow, flow, mask, odp_in_port, SIZE_MAX);
>>> }
>>
>> This looks odd, how about passing in both flow and mask, and a boolean “is_mask” instead of “flow, flow, mask” or “mask, flow, mask” below?
>>
>>> /* Appends a representation of 'mask' as OVS_KEY_ATTR_* attributes to
>>> @@ -2699,8 +2699,8 @@ odp_flow_key_from_mask(struct ofpbuf *buf, const struct flow *mask,
>>> const struct flow *flow, uint32_t odp_in_port_mask,
>>> size_t max_mpls_depth)
>>> {
>>> - odp_flow_key_from_flow__(buf, mask, flow, u32_to_odp(odp_in_port_mask),
>>> - max_mpls_depth);
>>> + odp_flow_key_from_flow__(buf, mask, flow, mask,
>>> + u32_to_odp(odp_in_port_mask), max_mpls_depth);
>>> }
>>
>> Otherwise,
>>
>> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>>
>> Jarno
>>
More information about the dev
mailing list