[ovs-dev] [PATCH v2 4/8] netdev-dpdk: implement flow put with rte flow

Simon Horman simon.horman at netronome.com
Fri Sep 8 16:47:55 UTC 2017


On Tue, Sep 05, 2017 at 05:22:57PM +0800, Yuanhan Liu wrote:
> From: Finn Christensen <fc at napatech.com>
> 
> The basic yet the major part of this patch is to translate the "match"
> to rte flow patterns. And then, we create a rte flow with a MARK action.
> Afterwards, all pkts matches the flow will have the mark id in the mbuf.
> 
> For any unsupported flows, such as MPLS, -1 is returned, meaning the
> flow offload is failed and then skipped.

...

> +static int
> +netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
> +                                 const struct match *match,
> +                                 struct nlattr *nl_actions OVS_UNUSED,
> +                                 size_t actions_len,
> +                                 const ovs_u128 *ufid,
> +                                 struct offload_info *info)
> +{
> +    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> +    const struct rte_flow_attr flow_attr = {
> +        .group = 0,
> +        .priority = 0,
> +        .ingress = 1,
> +        .egress = 0
> +    };
> +    struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> +    struct flow_actions actions = { .actions = NULL, .cnt = 0 };

I believe the following will give the same result as the above,
less verbosely.

+    const struct rte_flow_attr flow_attr = { .ingress = 1 };
+    struct flow_patterns patterns = { .cnt = 0 };
+    struct flow_actions actions = { .cnt = 0 };

> +    struct rte_flow *flow;
> +    struct rte_flow_error error;
> +    uint8_t *ipv4_next_proto_mask = NULL;
> +    int ret = 0;
> +
> +    /* Eth */
> +    struct rte_flow_item_eth eth_spec;
> +    struct rte_flow_item_eth eth_mask;

Empty line here please.

> +    memset(&eth_mask, 0, sizeof(eth_mask));
> +    if (match->wc.masks.dl_src.be16[0] ||
> +        match->wc.masks.dl_src.be16[1] ||
> +        match->wc.masks.dl_src.be16[2] ||
> +        match->wc.masks.dl_dst.be16[0] ||
> +        match->wc.masks.dl_dst.be16[1] ||
> +        match->wc.masks.dl_dst.be16[2]) {

It looks like eth_addr_is_zero() can be used in the above.

> +        rte_memcpy(&eth_spec.dst, &match->flow.dl_dst, sizeof(eth_spec.dst));
> +        rte_memcpy(&eth_spec.src, &match->flow.dl_src, sizeof(eth_spec.src));
> +        eth_spec.type = match->flow.dl_type;
> +
> +        rte_memcpy(&eth_mask.dst, &match->wc.masks.dl_dst,
> +                   sizeof(eth_mask.dst));
> +        rte_memcpy(&eth_mask.src, &match->wc.masks.dl_src,
> +                   sizeof(eth_mask.src));
> +        eth_mask.type = match->wc.masks.dl_type;
> +
> +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> +                         &eth_spec, &eth_mask);
> +    } else {
> +        /*
> +         * If user specifies a flow (like UDP flow) without L2 patterns,
> +         * OVS will at least set the dl_type. Normally, it's enough to
> +         * create an eth pattern just with it. Unluckily, some Intel's
> +         * NIC (such as XL710) doesn't support that. Below is a workaround,
> +         * which simply matches any L2 pkts.
> +         */
> +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> +    }

This feels a lot like a layer violation - making the core aware
of specific implementation details of lower layers.

>From a functional point of view, is the idea that
a eth_type+5-tuple match is converted to a 5-tuple match?

> +
> +    /* VLAN */
> +    struct rte_flow_item_vlan vlan_spec;
> +    struct rte_flow_item_vlan vlan_mask;

Please declare all local variables at the top of the context
(in this case function).

...

> +    struct rte_flow_item_udp udp_spec;
> +    struct rte_flow_item_udp udp_mask;
> +    memset(&udp_mask, 0, sizeof(udp_mask));
> +    if ((proto == IPPROTO_UDP) &&
> +        (match->wc.masks.tp_src || match->wc.masks.tp_dst)) {
> +        udp_spec.hdr.src_port = match->flow.tp_src;
> +        udp_spec.hdr.dst_port = match->flow.tp_dst;
> +
> +        udp_mask.hdr.src_port = match->wc.masks.tp_src;
> +        udp_mask.hdr.dst_port = match->wc.masks.tp_dst;
> +
> +        add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
> +                         &udp_spec, &udp_mask);
> +
> +        /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match */
> +        if (ipv4_next_proto_mask) {
> +            *ipv4_next_proto_mask = 0;

I think this should be:

+            *ipv4_next_proto_mask = NULL;

> +        }
> +    }
> +
> +    struct rte_flow_item_sctp sctp_spec;
> +    struct rte_flow_item_sctp sctp_mask;
> +    memset(&sctp_mask, 0, sizeof(sctp_mask));
> +    if ((proto == IPPROTO_SCTP) &&

It seems there are unnecessary () in the line above.

...

> +/*
> + * Validate for later rte flow offload creation. If any unsupported
> + * flow are specified, return -1.
> + */
> +static int
> +netdev_dpdk_validate_flow(const struct match *match)
> +{
> +    struct match match_zero_wc;
> +
> +    /* Create a wc-zeroed version of flow */
> +    match_init(&match_zero_wc, &match->flow, &match->wc);
> +
> +#define CHECK_NONZERO_BYTES(addr, size) do {    \

I think do should appear on a new line.

> +    uint8_t *padr = (uint8_t *)(addr);          \
> +    int i;                                      \
> +    for (i = 0; i < (size); i++) {              \
> +        if (padr[i] != 0) {                     \
> +            goto err;                           \
> +        }                                       \
> +    }                                           \
> +} while (0)
> +
> +#define CHECK_NONZERO(var)              do {    \

Here too.

> +    if ((var) != 0) {                           \
> +        goto err;                               \
> +    }                                           \
> +} while (0)

I think its better if macros (and defines) aren't declared
inside of functions. The top of the file seems like
the best place to me. If not above the first function
that uses the macros, presumably this function.

...


More information about the dev mailing list