On 08.12.2019 14:22, Eli Britstein wrote:
> Refactor the flow patterns code to a helper function for better
> readability and towards supporting more matches.
>
> Signed-off-by: Eli Britstein <el...@mellanox.com>
> Reviewed-by: Oz Shlomo <o...@mellanox.com>
> ---
Although I'm not quite sure that we need this change, a few comments
for the current patch inline.
> lib/netdev-offload-dpdk.c | 208
> +++++++++++++++++++++++++---------------------
> 1 file changed, 112 insertions(+), 96 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 96794dc4d..a008e642f 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -410,54 +410,42 @@ add_flow_rss_action(struct flow_actions *actions,
> return rss_data;
> }
>
> +struct flow_items {
> + struct rte_flow_item_eth eth;
> + struct rte_flow_item_vlan vlan;
> + struct rte_flow_item_ipv4 ipv4;
> + union {
> + struct rte_flow_item_tcp tcp;
> + struct rte_flow_item_udp udp;
> + struct rte_flow_item_sctp sctp;
> + struct rte_flow_item_icmp icmp;
> + };
> +};
> +
> static int
> -netdev_offload_dpdk_add_flow(struct netdev *netdev,
> - const struct match *match,
> - struct nlattr *nl_actions OVS_UNUSED,
> - size_t actions_len OVS_UNUSED,
> - const ovs_u128 *ufid,
> - struct offload_info *info)
> +parse_flow_match(struct flow_patterns *patterns,
> + struct flow_items *spec,
> + struct flow_items *mask,
> + const struct match *match)
> {
> - 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 };
> - struct rte_flow *flow;
> - struct rte_flow_error error;
> uint8_t proto = 0;
> - int ret = 0;
> - struct flow_items {
> - struct rte_flow_item_eth eth;
> - struct rte_flow_item_vlan vlan;
> - struct rte_flow_item_ipv4 ipv4;
> - union {
> - struct rte_flow_item_tcp tcp;
> - struct rte_flow_item_udp udp;
> - struct rte_flow_item_sctp sctp;
> - struct rte_flow_item_icmp icmp;
> - };
> - } spec, mask;
> -
> - memset(&spec, 0, sizeof spec);
> - memset(&mask, 0, sizeof mask);
> +
> + memset(spec, 0, sizeof *spec);
> + memset(mask, 0, sizeof *mask);
I think that we need to clear those before calling parse_flow_match().
Will look cleaner.
>
> /* Eth */
> if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
> !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> - memcpy(&spec.eth.dst, &match->flow.dl_dst, sizeof spec.eth.dst);
> - memcpy(&spec.eth.src, &match->flow.dl_src, sizeof spec.eth.src);
> - spec.eth.type = match->flow.dl_type;
> + memcpy(&spec->eth.dst, &match->flow.dl_dst, sizeof spec->eth.dst);
> + memcpy(&spec->eth.src, &match->flow.dl_src, sizeof spec->eth.src);
> + spec->eth.type = match->flow.dl_type;
>
> - memcpy(&mask.eth.dst, &match->wc.masks.dl_dst, sizeof mask.eth.dst);
> - memcpy(&mask.eth.src, &match->wc.masks.dl_src, sizeof mask.eth.src);
> - mask.eth.type = match->wc.masks.dl_type;
> + memcpy(&mask->eth.dst, &match->wc.masks.dl_dst, sizeof
> mask->eth.dst);
> + memcpy(&mask->eth.src, &match->wc.masks.dl_src, sizeof
> mask->eth.src);
> + mask->eth.type = match->wc.masks.dl_type;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> - &spec.eth, &mask.eth);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH,
> + &spec->eth, &mask->eth);
> } else {
> /*
> * If user specifies a flow (like UDP flow) without L2 patterns,
> @@ -466,41 +454,41 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
> * 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);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
> }
>
> /* VLAN */
> if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
> - spec.vlan.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> - mask.vlan.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
> + spec->vlan.tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> + mask->vlan.tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
>
> /* Match any protocols. */
> - mask.vlan.inner_type = 0;
> + mask->vlan.inner_type = 0;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_VLAN,
> - &spec.vlan, &mask.vlan);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_VLAN,
> + &spec->vlan, &mask->vlan);
> }
>
> /* IP v4 */
> if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
> - spec.ipv4.hdr.type_of_service = match->flow.nw_tos;
> - spec.ipv4.hdr.time_to_live = match->flow.nw_ttl;
> - spec.ipv4.hdr.next_proto_id = match->flow.nw_proto;
> - spec.ipv4.hdr.src_addr = match->flow.nw_src;
> - spec.ipv4.hdr.dst_addr = match->flow.nw_dst;
> + spec->ipv4.hdr.type_of_service = match->flow.nw_tos;
> + spec->ipv4.hdr.time_to_live = match->flow.nw_ttl;
> + spec->ipv4.hdr.next_proto_id = match->flow.nw_proto;
> + spec->ipv4.hdr.src_addr = match->flow.nw_src;
> + spec->ipv4.hdr.dst_addr = match->flow.nw_dst;
>
> - mask.ipv4.hdr.type_of_service = match->wc.masks.nw_tos;
> - mask.ipv4.hdr.time_to_live = match->wc.masks.nw_ttl;
> - mask.ipv4.hdr.next_proto_id = match->wc.masks.nw_proto;
> - mask.ipv4.hdr.src_addr = match->wc.masks.nw_src;
> - mask.ipv4.hdr.dst_addr = match->wc.masks.nw_dst;
> + mask->ipv4.hdr.type_of_service = match->wc.masks.nw_tos;
> + mask->ipv4.hdr.time_to_live = match->wc.masks.nw_ttl;
> + mask->ipv4.hdr.next_proto_id = match->wc.masks.nw_proto;
> + mask->ipv4.hdr.src_addr = match->wc.masks.nw_src;
> + mask->ipv4.hdr.dst_addr = match->wc.masks.nw_dst;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_IPV4,
> - &spec.ipv4, &mask.ipv4);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4,
> + &spec->ipv4, &mask->ipv4);
>
> /* Save proto for L4 protocol setup. */
> - proto = spec.ipv4.hdr.next_proto_id &
> - mask.ipv4.hdr.next_proto_id;
> + proto = spec->ipv4.hdr.next_proto_id &
> + mask->ipv4.hdr.next_proto_id;
> }
>
> if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP &&
> @@ -509,79 +497,107 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
> match->wc.masks.tp_dst ||
> match->wc.masks.tcp_flags)) {
> VLOG_DBG("L4 Protocol (%u) not supported", proto);
> - ret = -1;
> - goto out;
> + return -1;
> }
>
> if ((match->wc.masks.tp_src && match->wc.masks.tp_src != OVS_BE16_MAX) ||
> (match->wc.masks.tp_dst && match->wc.masks.tp_dst != OVS_BE16_MAX)) {
> - ret = -1;
> - goto out;
> + return -1;
> }
>
> switch (proto) {
> case IPPROTO_TCP:
> - spec.tcp.hdr.src_port = match->flow.tp_src;
> - spec.tcp.hdr.dst_port = match->flow.tp_dst;
> - spec.tcp.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> - spec.tcp.hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
> + spec->tcp.hdr.src_port = match->flow.tp_src;
> + spec->tcp.hdr.dst_port = match->flow.tp_dst;
> + spec->tcp.hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> + spec->tcp.hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
>
> - mask.tcp.hdr.src_port = match->wc.masks.tp_src;
> - mask.tcp.hdr.dst_port = match->wc.masks.tp_dst;
> - mask.tcp.hdr.data_off = ntohs(match->wc.masks.tcp_flags) >> 8;
> - mask.tcp.hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;
> + mask->tcp.hdr.src_port = match->wc.masks.tp_src;
> + mask->tcp.hdr.dst_port = match->wc.masks.tp_dst;
> + mask->tcp.hdr.data_off = ntohs(match->wc.masks.tcp_flags) >> 8;
> + mask->tcp.hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_TCP,
> - &spec.tcp, &mask.tcp);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_TCP,
> + &spec->tcp, &mask->tcp);
>
> /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match. */
> - mask.ipv4.hdr.next_proto_id = 0;
> + mask->ipv4.hdr.next_proto_id = 0;
> break;
>
> case IPPROTO_UDP:
> - spec.udp.hdr.src_port = match->flow.tp_src;
> - spec.udp.hdr.dst_port = match->flow.tp_dst;
> + spec->udp.hdr.src_port = match->flow.tp_src;
> + spec->udp.hdr.dst_port = match->flow.tp_dst;
>
> - mask.udp.hdr.src_port = match->wc.masks.tp_src;
> - mask.udp.hdr.dst_port = match->wc.masks.tp_dst;
> + mask->udp.hdr.src_port = match->wc.masks.tp_src;
> + mask->udp.hdr.dst_port = match->wc.masks.tp_dst;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_UDP,
> - &spec.udp, &mask.udp);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP,
> + &spec->udp, &mask->udp);
>
> /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match. */
> - mask.ipv4.hdr.next_proto_id = 0;
> + mask->ipv4.hdr.next_proto_id = 0;
> break;
>
> case IPPROTO_SCTP:
> - spec.sctp.hdr.src_port = match->flow.tp_src;
> - spec.sctp.hdr.dst_port = match->flow.tp_dst;
> + spec->sctp.hdr.src_port = match->flow.tp_src;
> + spec->sctp.hdr.dst_port = match->flow.tp_dst;
>
> - mask.sctp.hdr.src_port = match->wc.masks.tp_src;
> - mask.sctp.hdr.dst_port = match->wc.masks.tp_dst;
> + mask->sctp.hdr.src_port = match->wc.masks.tp_src;
> + mask->sctp.hdr.dst_port = match->wc.masks.tp_dst;
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_SCTP,
> - &spec.sctp, &mask.sctp);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_SCTP,
> + &spec->sctp, &mask->sctp);
>
> /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto match. */
> - mask.ipv4.hdr.next_proto_id = 0;
> + mask->ipv4.hdr.next_proto_id = 0;
> break;
>
> case IPPROTO_ICMP:
> - spec.icmp.hdr.icmp_type = (uint8_t) ntohs(match->flow.tp_src);
> - spec.icmp.hdr.icmp_code = (uint8_t) ntohs(match->flow.tp_dst);
> + spec->icmp.hdr.icmp_type = (uint8_t) ntohs(match->flow.tp_src);
> + spec->icmp.hdr.icmp_code = (uint8_t) ntohs(match->flow.tp_dst);
>
> - mask.icmp.hdr.icmp_type = (uint8_t) ntohs(match->wc.masks.tp_src);
> - mask.icmp.hdr.icmp_code = (uint8_t) ntohs(match->wc.masks.tp_dst);
> + mask->icmp.hdr.icmp_type = (uint8_t) ntohs(match->wc.masks.tp_src);
> + mask->icmp.hdr.icmp_code = (uint8_t) ntohs(match->wc.masks.tp_dst);
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ICMP,
> - &spec.icmp, &mask.icmp);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ICMP,
> + &spec->icmp, &mask->icmp);
>
> /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto match. */
> - mask.ipv4.hdr.next_proto_id = 0;
> + mask->ipv4.hdr.next_proto_id = 0;
> break;
> }
>
> - add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
> +
> + return 0;
> +}
> +
> +static int
> +netdev_offload_dpdk_add_flow(struct netdev *netdev,
> + const struct match *match,
> + struct nlattr *nl_actions OVS_UNUSED,
> + size_t actions_len OVS_UNUSED,
> + const ovs_u128 *ufid,
> + struct offload_info *info)
> +{
> + const struct rte_flow_attr flow_attr = {
> + .group = 0,
> + .priority = 0,
> + .ingress = 1,
> + .egress = 0
It's not really connected to this patch, but we might want to initialize
'transfer' explicitely here too.
> + };
> + struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> + struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> + struct rte_flow *flow;
> + struct rte_flow_error error;
> + int ret = 0;
> + struct flow_items spec, mask;
> +
> + ret = parse_flow_match(&patterns, &spec, &mask, match);
> + if (ret) {
> + ret = -1;
Why re-setting?
> + goto out;
> + }
>
> struct rte_flow_action_mark mark;
> struct action_rss_data *rss;
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
