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 <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.
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;
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
