[ovs-dev] [PATCH] netdev-dpdk: Use single struct/union for flow offload items.

Ilya Maximets i.maximets at samsung.com
Wed Feb 6 15:33:15 UTC 2019


On 06.02.2019 18:16, Ophir Munk wrote:
> Hi,
> Please find comments inline.
> 
>> -----Original Message-----
>> From: ovs-dev-bounces at openvswitch.org <ovs-dev-
>> bounces at openvswitch.org> On Behalf Of Ilya Maximets
>> Sent: Wednesday, February 6, 2019 3:19 PM
>> To: ovs-dev at openvswitch.org; Ian Stokes <ian.stokes at intel.com>
>> Cc: Ilya Maximets <i.maximets at samsung.com>; Roni Bar Yanai
>> <roniba at mellanox.com>
>> Subject: [ovs-dev] [PATCH] netdev-dpdk: Use single struct/union for flow
>> offload items.
>>
>> Having a single structure allows to simplify the code path and clear all the
>> items at once (probably faster). This does not increase stack memory usage
>> because all the L4 related items grouped in a union.
>>
>> Changes:
>>   - Memsets combined.
>>   - 'ipv4_next_proto_mask' calculated at the top as we already know
>>     the address. We also do not need to check it before clearing.
>>   - Group of 'if' statements for L4 protocols turned to a 'switch'.
>>     We can do that, because we don't have semi-local variables anymore.
>>   - Eliminated 'end_proto_check' label. Not needed with 'switch'.
>>
>> Additionally 'rte_memcpy' replaced with simple 'memcpy' as it makes no
>> sense to use 'rte_memcpy' for 6 bytes.
>>
>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>> ---
>>  lib/netdev-dpdk.c | 190 +++++++++++++++++++---------------------------
>>  1 file changed, 79 insertions(+), 111 deletions(-)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
>> 26022a59c..79219f6ef 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -4564,28 +4564,37 @@ netdev_dpdk_add_rte_flow_offload(struct
>> netdev *netdev,
>>      struct flow_actions actions = { .actions = NULL, .cnt = 0 };
>>      struct rte_flow *flow;
>>      struct rte_flow_error error;
>> -    uint8_t *ipv4_next_proto_mask = NULL;
>> +    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;
>> +    uint8_t *ipv4_next_proto_mask = &mask.ipv4.hdr.next_proto_id;
>> +
> 
> Please consider removing ipv4_next_porto_mask pointer assignment and later use directly mask.ipv4.hdr.next_proto_id.

Good point. Thanks.

> 
>> +    memset(&spec, 0, sizeof spec);
>> +    memset(&mask, 0, sizeof mask);
> 
> For your consideration: we can save some memset() calls if we moved them inside the Eth case. 
> I like their current position for better readability.

Single place for clearing the whole structure seems better for me because
it protects from possible issues with uninitialized data.
Also clearing big place at once could allow compiler to choose vectorized
implementations. Most of structures are too small.

> 
>>
>>      /* Eth */
>> -    struct rte_flow_item_eth eth_spec;
>> -    struct rte_flow_item_eth eth_mask;
>>      if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>          !eth_addr_is_zero(match->wc.masks.dl_dst)) {
>> -        memset(&eth_spec, 0, sizeof eth_spec);
>> -        memset(&eth_mask, 0, sizeof eth_mask);
>> -        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;
>> +        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;
>>
>>          add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
>> -                         &eth_spec, &eth_mask);
>> +                         &spec.eth, &mask.eth);
>>      } else {
>>          /*
>>           * If user specifies a flow (like UDP flow) without L2 patterns, @@ -
>> 4598,50 +4607,37 @@ netdev_dpdk_add_rte_flow_offload(struct netdev
>> *netdev,
>>      }
>>
>>      /* VLAN */
>> -    struct rte_flow_item_vlan vlan_spec;
>> -    struct rte_flow_item_vlan vlan_mask;
>>      if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
>> -        memset(&vlan_spec, 0, sizeof vlan_spec);
>> -        memset(&vlan_mask, 0, sizeof vlan_mask);
>> -        vlan_spec.tci  = match->flow.vlans[0].tci & ~htons(VLAN_CFI);
>> -        vlan_mask.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 */
>> -        vlan_mask.inner_type = 0;
>> +        mask.vlan.inner_type = 0;
>>
>>          add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_VLAN,
>> -                         &vlan_spec, &vlan_mask);
>> +                         &spec.vlan, &mask.vlan);
>>      }
>>
>>      /* IP v4 */
>> -    uint8_t proto = 0;
>> -    struct rte_flow_item_ipv4 ipv4_spec;
>> -    struct rte_flow_item_ipv4 ipv4_mask;
>>      if (match->flow.dl_type == htons(ETH_TYPE_IP)) {
>> -        memset(&ipv4_spec, 0, sizeof ipv4_spec);
>> -        memset(&ipv4_mask, 0, sizeof ipv4_mask);
>> -
>> -        ipv4_spec.hdr.type_of_service = match->flow.nw_tos;
>> -        ipv4_spec.hdr.time_to_live    = match->flow.nw_ttl;
>> -        ipv4_spec.hdr.next_proto_id   = match->flow.nw_proto;
>> -        ipv4_spec.hdr.src_addr        = match->flow.nw_src;
>> -        ipv4_spec.hdr.dst_addr        = match->flow.nw_dst;
>> -
>> -        ipv4_mask.hdr.type_of_service = match->wc.masks.nw_tos;
>> -        ipv4_mask.hdr.time_to_live    = match->wc.masks.nw_ttl;
>> -        ipv4_mask.hdr.next_proto_id   = match->wc.masks.nw_proto;
>> -        ipv4_mask.hdr.src_addr        = match->wc.masks.nw_src;
>> -        ipv4_mask.hdr.dst_addr        = match->wc.masks.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;
>>
>>          add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_IPV4,
>> -                         &ipv4_spec, &ipv4_mask);
>> +                         &spec.ipv4, &mask.ipv4);
>>
>>          /* Save proto for L4 protocol setup */
>> -        proto = ipv4_spec.hdr.next_proto_id &
>> -                ipv4_mask.hdr.next_proto_id;
>> -
>> -        /* Remember proto mask address for later modification */
>> -        ipv4_next_proto_mask = &ipv4_mask.hdr.next_proto_id;
>> +        proto = spec.ipv4.hdr.next_proto_id &
>> +                mask.ipv4.hdr.next_proto_id;
>>      }
>>
>>      if (proto != IPPROTO_ICMP && proto != IPPROTO_UDP  && @@ -4660,96
>> +4656,68 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>>          goto out;
>>      }
>>
>> -    struct rte_flow_item_tcp tcp_spec;
>> -    struct rte_flow_item_tcp tcp_mask;
>> -    if (proto == IPPROTO_TCP) {
>> -        memset(&tcp_spec, 0, sizeof tcp_spec);
>> -        memset(&tcp_mask, 0, sizeof tcp_mask);
>> -        tcp_spec.hdr.src_port  = match->flow.tp_src;
>> -        tcp_spec.hdr.dst_port  = match->flow.tp_dst;
>> -        tcp_spec.hdr.data_off  = ntohs(match->flow.tcp_flags) >> 8;
>> -        tcp_spec.hdr.tcp_flags = ntohs(match->flow.tcp_flags) & 0xff;
>> +    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;
>>
>> -        tcp_mask.hdr.src_port  = match->wc.masks.tp_src;
>> -        tcp_mask.hdr.dst_port  = match->wc.masks.tp_dst;
>> -        tcp_mask.hdr.data_off  = ntohs(match->wc.masks.tcp_flags) >> 8;
>> -        tcp_mask.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,
>> -                         &tcp_spec, &tcp_mask);
>> +                         &spec.tcp, &mask.tcp);
>>
>>          /* proto == TCP and ITEM_TYPE_TCP, thus no need for proto match */
>> -        if (ipv4_next_proto_mask) {
>> -            *ipv4_next_proto_mask = 0;
>> -        }
>> -        goto end_proto_check;
>> -    }
>> +        *ipv4_next_proto_mask = 0;
> Please consider:
> mask.ipv4.hdr.next_proto_id = 0;
> 
>> +        break;
>>
>> -    struct rte_flow_item_udp udp_spec;
>> -    struct rte_flow_item_udp udp_mask;
>> -    if (proto == IPPROTO_UDP) {
>> -        memset(&udp_spec, 0, sizeof udp_spec);
>> -        memset(&udp_mask, 0, sizeof udp_mask);
>> -        udp_spec.hdr.src_port = match->flow.tp_src;
>> -        udp_spec.hdr.dst_port = match->flow.tp_dst;
>> +    case IPPROTO_UDP:
>> +        spec.udp.hdr.src_port = match->flow.tp_src;
>> +        spec.udp.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;
>> +        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,
>> -                         &udp_spec, &udp_mask);
>> +                         &spec.udp, &mask.udp);
>>
>>          /* proto == UDP and ITEM_TYPE_UDP, thus no need for proto match */
>> -        if (ipv4_next_proto_mask) {
>> -            *ipv4_next_proto_mask = 0;
>> -        }
>> -        goto end_proto_check;
>> -    }
>> +        *ipv4_next_proto_mask = 0;
> 
> Dito
> 
>> +        break;
>>
>> -    struct rte_flow_item_sctp sctp_spec;
>> -    struct rte_flow_item_sctp sctp_mask;
>> -    if (proto == IPPROTO_SCTP) {
>> -        memset(&sctp_spec, 0, sizeof sctp_spec);
>> -        memset(&sctp_mask, 0, sizeof sctp_mask);
>> -        sctp_spec.hdr.src_port = match->flow.tp_src;
>> -        sctp_spec.hdr.dst_port = match->flow.tp_dst;
>> +    case IPPROTO_SCTP:
>> +        spec.sctp.hdr.src_port = match->flow.tp_src;
>> +        spec.sctp.hdr.dst_port = match->flow.tp_dst;
>>
>> -        sctp_mask.hdr.src_port = match->wc.masks.tp_src;
>> -        sctp_mask.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,
>> -                         &sctp_spec, &sctp_mask);
>> +                         &spec.sctp, &mask.sctp);
>>
>>          /* proto == SCTP and ITEM_TYPE_SCTP, thus no need for proto match */
>> -        if (ipv4_next_proto_mask) {
>> -            *ipv4_next_proto_mask = 0;
>> -        }
>> -        goto end_proto_check;
>> -    }
>> +        *ipv4_next_proto_mask = 0;
> 
> Dito
> 
>> +        break;
>>
>> -    struct rte_flow_item_icmp icmp_spec;
>> -    struct rte_flow_item_icmp icmp_mask;
>> -    if (proto == IPPROTO_ICMP) {
>> -        memset(&icmp_spec, 0, sizeof icmp_spec);
>> -        memset(&icmp_mask, 0, sizeof icmp_mask);
>> -        icmp_spec.hdr.icmp_type = (uint8_t)ntohs(match->flow.tp_src);
>> -        icmp_spec.hdr.icmp_code = (uint8_t)ntohs(match->flow.tp_dst);
>> +    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);
>>
>> -        icmp_mask.hdr.icmp_type = (uint8_t)ntohs(match->wc.masks.tp_src);
>> -        icmp_mask.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,
>> -                         &icmp_spec, &icmp_mask);
>> +                         &spec.icmp, &mask.icmp);
>>
>>          /* proto == ICMP and ITEM_TYPE_ICMP, thus no need for proto match
>> */
>> -        if (ipv4_next_proto_mask) {
>> -            *ipv4_next_proto_mask = 0;
>> -        }
>> -        goto end_proto_check;
>> +        *ipv4_next_proto_mask = 0;
> 
> Dito
> 
>> +        break;
>>      }
>>
>> -end_proto_check:
>> -
>>      add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_END, NULL, NULL);
>>
>>      struct rte_flow_action_mark mark;
>> --
>> 2.17.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
>> il.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-
>> dev&amp;data=02%7C01%7Cophirmu%40mellanox.com%7C6c046deb6e3c4c
>> 2de10f08d68c35a8f5%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C
>> 636850559431205667&amp;sdata=opRCXKToPo9cAAgjf2P5Rld9ep1s849LTeml
>> ReJQpmU%3D&amp;reserved=0
> 
> 


More information about the dev mailing list