[ovs-dev] [PATCH 03/20] netdev-offload-dpdk-flow: Dynamically allocate pattern items
Sriharsha Basavapatna
sriharsha.basavapatna at broadcom.com
Tue Dec 3 15:00:14 UTC 2019
On Wed, Nov 20, 2019 at 9:07 PM Eli Britstein <elibr at mellanox.com> wrote:
>
> Instead of statically allocated pattern items on the stack,dynamically
> allocate only the required items while parsing the required matches.
What are the benefits of dynamic allocation scheme? It is not clear
from the patch whether this additional complexity over the static
allocation is worthwhile.
Thanks,
-Harsha
>
> Signed-off-by: Eli Britstein <elibr at mellanox.com>
> Reviewed-by: Oz Shlomo <ozsh at mellanox.com>
> ---
> lib/netdev-offload-dpdk-flow.c | 163 +++++++++++++++++++++-----------------
> lib/netdev-offload-dpdk-private.h | 14 ----
> lib/netdev-offload-dpdk.c | 6 +-
> 3 files changed, 90 insertions(+), 93 deletions(-)
>
> diff --git a/lib/netdev-offload-dpdk-flow.c b/lib/netdev-offload-dpdk-flow.c
> index d1d5ce2c6..c8c3e28ea 100644
> --- a/lib/netdev-offload-dpdk-flow.c
> +++ b/lib/netdev-offload-dpdk-flow.c
> @@ -29,6 +29,16 @@ void
> netdev_dpdk_flow_patterns_free(struct flow_patterns *patterns)
> {
> /* When calling this function 'patterns' must be valid */
> + int i;
> +
> + for (i = 0; i < patterns->cnt; i++) {
> + if (patterns->items[i].spec) {
> + free((void *)patterns->items[i].spec);
> + }
> + if (patterns->items[i].mask) {
> + free((void *)patterns->items[i].mask);
> + }
> + }
> free(patterns->items);
> patterns->items = NULL;
> patterns->cnt = 0;
> @@ -333,8 +343,6 @@ netdev_dpdk_flow_actions_add_mark_rss(struct flow_actions *actions,
>
> int
> netdev_dpdk_flow_patterns_add(struct flow_patterns *patterns,
> - struct flow_pattern_items *spec,
> - struct flow_pattern_items *mask,
> const struct match *match)
> {
> uint8_t proto = 0;
> @@ -342,16 +350,20 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns *patterns,
> /* 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;
> + struct rte_flow_item_eth *spec, *mask;
> +
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - 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(&spec->dst, &match->flow.dl_dst, sizeof spec->dst);
> + memcpy(&spec->src, &match->flow.dl_src, sizeof spec->src);
> + spec->type = match->flow.dl_type;
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH,
> - &spec->eth, &mask->eth);
> + memcpy(&mask->dst, &match->wc.masks.dl_dst, sizeof mask->dst);
> + memcpy(&mask->src, &match->wc.masks.dl_src, sizeof mask->src);
> + mask->type = match->wc.masks.dl_type;
> +
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask);
> } else {
> /*
> * If user specifies a flow (like UDP flow) without L2 patterns,
> @@ -365,36 +377,43 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns *patterns,
>
> /* 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);
> + struct rte_flow_item_vlan *spec, *mask;
> +
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
> +
> + spec->tci = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
> + mask->tci = match->wc.masks.vlans[0].tci & ~htons(VLAN_CFI);
>
> /* Match any protocols. */
> - mask->vlan.inner_type = 0;
> + mask->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, mask);
> }
>
> /* 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;
> + struct rte_flow_item_ipv4 *spec, *mask;
>
> - 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;
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4,
> - &spec->ipv4, &mask->ipv4);
> + spec->hdr.type_of_service = match->flow.nw_tos;
> + spec->hdr.time_to_live = match->flow.nw_ttl;
> + spec->hdr.next_proto_id = match->flow.nw_proto;
> + spec->hdr.src_addr = match->flow.nw_src;
> + spec->hdr.dst_addr = match->flow.nw_dst;
> +
> + mask->hdr.type_of_service = match->wc.masks.nw_tos;
> + mask->hdr.time_to_live = match->wc.masks.nw_ttl;
> + mask->hdr.next_proto_id = match->wc.masks.nw_proto;
> + mask->hdr.src_addr = match->wc.masks.nw_src;
> + mask->hdr.dst_addr = match->wc.masks.nw_dst;
> +
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_IPV4, spec, mask);
>
> /* Save proto for L4 protocol setup. */
> - proto = spec->ipv4.hdr.next_proto_id &
> - mask->ipv4.hdr.next_proto_id;
> + proto = spec->hdr.next_proto_id & mask->hdr.next_proto_id;
> }
>
> if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP &&
> @@ -411,66 +430,62 @@ netdev_dpdk_flow_patterns_add(struct flow_patterns *patterns,
> 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;
> + if (proto == IPPROTO_TCP) {
> + struct rte_flow_item_tcp *spec, *mask;
> +
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - 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;
> + spec->hdr.src_port = match->flow.tp_src;
> + spec->hdr.dst_port = match->flow.tp_dst;
> + spec->hdr.data_off = ntohs(match->flow.tcp_flags) >> 8;
> + spec->hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_TCP,
> - &spec->tcp, &mask->tcp);
> + mask->hdr.src_port = match->wc.masks.tp_src;
> + mask->hdr.dst_port = match->wc.masks.tp_dst;
> + mask->hdr.data_off = ntohs(match->wc.masks.tcp_flags) >> 8;
> + mask->hdr.tcp_flags = ntohs(match->wc.masks.tcp_flags) & 0xff;
>
> - /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match. */
> - mask->ipv4.hdr.next_proto_id = 0;
> - break;
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_TCP, spec, mask);
> + } else if (proto == IPPROTO_UDP) {
> + struct rte_flow_item_udp *spec, *mask;
>
> - case IPPROTO_UDP:
> - spec->udp.hdr.src_port = match->flow.tp_src;
> - spec->udp.hdr.dst_port = match->flow.tp_dst;
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - mask->udp.hdr.src_port = match->wc.masks.tp_src;
> - mask->udp.hdr.dst_port = match->wc.masks.tp_dst;
> + spec->hdr.src_port = match->flow.tp_src;
> + spec->hdr.dst_port = match->flow.tp_dst;
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP,
> - &spec->udp, &mask->udp);
> + mask->hdr.src_port = match->wc.masks.tp_src;
> + mask->hdr.dst_port = match->wc.masks.tp_dst;
>
> - /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match. */
> - mask->ipv4.hdr.next_proto_id = 0;
> - break;
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_UDP, spec, mask);
> + } else if (proto == IPPROTO_SCTP) {
> + struct rte_flow_item_sctp *spec, *mask;
>
> - case IPPROTO_SCTP:
> - spec->sctp.hdr.src_port = match->flow.tp_src;
> - spec->sctp.hdr.dst_port = match->flow.tp_dst;
> + spec = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - mask->sctp.hdr.src_port = match->wc.masks.tp_src;
> - mask->sctp.hdr.dst_port = match->wc.masks.tp_dst;
> + spec->hdr.src_port = match->flow.tp_src;
> + spec->hdr.dst_port = match->flow.tp_dst;
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_SCTP,
> - &spec->sctp, &mask->sctp);
> + mask->hdr.src_port = match->wc.masks.tp_src;
> + mask->hdr.dst_port = match->wc.masks.tp_dst;
>
> - /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto match. */
> - mask->ipv4.hdr.next_proto_id = 0;
> - break;
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_SCTP, spec, mask);
> + } else if (proto == IPPROTO_ICMP) {
> + struct rte_flow_item_icmp *spec, *mask;
>
> - 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 = xzalloc(sizeof *spec);
> + mask = xzalloc(sizeof *mask);
>
> - 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);
> + spec->hdr.icmp_type = (uint8_t) ntohs(match->flow.tp_src);
> + spec->hdr.icmp_code = (uint8_t) ntohs(match->flow.tp_dst);
>
> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ICMP,
> - &spec->icmp, &mask->icmp);
> + mask->hdr.icmp_type = (uint8_t) ntohs(match->wc.masks.tp_src);
> + mask->hdr.icmp_code = (uint8_t) ntohs(match->wc.masks.tp_dst);
>
> - /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto match. */
> - mask->ipv4.hdr.next_proto_id = 0;
> - break;
> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ICMP, spec, mask);
> }
>
> add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
> diff --git a/lib/netdev-offload-dpdk-private.h b/lib/netdev-offload-dpdk-private.h
> index 397384cb0..e9c9e602a 100644
> --- a/lib/netdev-offload-dpdk-private.h
> +++ b/lib/netdev-offload-dpdk-private.h
> @@ -40,24 +40,10 @@ struct flow_actions {
> int current_max;
> };
>
> -struct flow_pattern_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;
> - };
> -};
> -
> void
> netdev_dpdk_flow_patterns_free(struct flow_patterns *patterns);
> int
> netdev_dpdk_flow_patterns_add(struct flow_patterns *patterns,
> - struct flow_pattern_items *spec,
> - struct flow_pattern_items *mask,
> const struct match *match);
> void
> netdev_dpdk_flow_actions_free(struct flow_actions *actions);
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index d6bbb7166..9882e1d23 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -131,15 +131,11 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
> };
> struct flow_patterns patterns = { .items = NULL, .cnt = 0 };
> struct flow_actions actions = { .actions = NULL, .cnt = 0 };
> - struct flow_pattern_items spec, mask;
> struct rte_flow_error error;
> struct rte_flow *flow;
> int ret = 0;
>
> - memset(&spec, 0, sizeof spec);
> - memset(&mask, 0, sizeof mask);
> -
> - ret = netdev_dpdk_flow_patterns_add(&patterns, &spec, &mask, match);
> + ret = netdev_dpdk_flow_patterns_add(&patterns, match);
> if (ret) {
> VLOG_WARN("Adding rte match patterns for flow ufid"UUID_FMT" failed",
> UUID_ARGS((struct uuid *)ufid));
> --
> 2.14.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list