| Message ID | 20191208132304.15521-2-elibr@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | netdev datapath actions offload | expand |
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 <elibr@mellanox.com> > Reviewed-by: Oz Shlomo <ozsh@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; >
On 12/10/2019 3:27 PM, Ilya Maximets wrote: > 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 <elibr@mellanox.com> >> Reviewed-by: Oz Shlomo <ozsh@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. OK > >> >> /* 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. Not related to this patch, and also fields that are not explicitly initialized are implicitly initialized to 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; >> + int ret = 0; >> + struct flow_items spec, mask; >> + >> + ret = parse_flow_match(&patterns, &spec, &mask, match); >> + if (ret) { >> + ret = -1; > Why re-setting? OK > >> + goto out; >> + } >> >> struct rte_flow_action_mark mark; >> struct action_rss_data *rss; >>
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); /* 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 + }; + 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; + goto out; + } struct rte_flow_action_mark mark; struct action_rss_data *rss;