[ovs-dev] [ovs-dev, v2] netdev-dpdk: Memset rte_flow_item on a need basis.

Ilya Maximets i.maximets at samsung.com
Tue Feb 5 11:45:34 UTC 2019


On 04.02.2019 19:14, Asaf Penso wrote:
> In netdev_dpdk_add_rte_flow_offload function different rte_flow_item are
> created as part of the pattern matching.
> 
> For most of them, there is a check whether the wildcards are not zero.
> In case of zero, nothing is being done with the rte_flow_item.
> 
> Befor the wildcard check, and regardless of the result, the
> rte_flow_item is being memset.
> 
> The patch moves the memset function only if the condition of the
> wildcard is true, thus saving cycles of memset if not needed.
> 
> Signed-off-by: Asaf Penso <asafp at mellanox.com>
> ---

I thought about this part of code a bit. IMHO, we could create
a union from the L4 items and clear them all at once. Like this:

    uint8_t proto = 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;

    memset(&spec, 0, sizeof spec);
    memset(&mask, 0, sizeof mask);


Ethernet is a common case, userspace datapath always has exact match on vlan,
IPv4 is also the common case, I think. So current code in most cases we'll
not call memset only for few of L4 protocols which are in the union here and
does not eat extra memory.
Anyway we're not on a very hot path.

With that you'll be able to easily convert L4 proto checking 'if's to a single
'switch (proto)' statement and also clear the *ipv4_next_proto_mask unconditionally.

What do you think ?

You could incorporate these changes to this patch or I could prepare the separate
patch on top of it.

Anyway, for the current version:

Acked-by: Ilya Maximets <i.maximets at samsung.com>

> v2 update coding-style compliant for sizeof operator
> ---
>  lib/netdev-dpdk.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 4bf0ca9..f07b10c 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4570,18 +4570,18 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>      /* Eth */
>      struct rte_flow_item_eth eth_spec;
>      struct rte_flow_item_eth eth_mask;
> -    memset(&eth_spec, 0, sizeof(eth_spec));
> -    memset(&eth_mask, 0, sizeof(eth_mask));
>      if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>          !eth_addr_is_zero(match->wc.masks.dl_dst)) {
> -        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));
> +        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));
> +                   sizeof eth_mask.dst);
>          rte_memcpy(&eth_mask.src, &match->wc.masks.dl_src,
> -                   sizeof(eth_mask.src));
> +                   sizeof eth_mask.src);
>          eth_mask.type = match->wc.masks.dl_type;
>  
>          add_flow_pattern(&patterns, RTE_FLOW_ITEM_TYPE_ETH,
> @@ -4600,9 +4600,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>      /* VLAN */
>      struct rte_flow_item_vlan vlan_spec;
>      struct rte_flow_item_vlan vlan_mask;
> -    memset(&vlan_spec, 0, sizeof(vlan_spec));
> -    memset(&vlan_mask, 0, sizeof(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);
>  
> @@ -4617,9 +4617,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>      uint8_t proto = 0;
>      struct rte_flow_item_ipv4 ipv4_spec;
>      struct rte_flow_item_ipv4 ipv4_mask;
> -    memset(&ipv4_spec, 0, sizeof(ipv4_spec));
> -    memset(&ipv4_mask, 0, sizeof(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;
> @@ -4662,9 +4662,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>  
>      struct rte_flow_item_tcp tcp_spec;
>      struct rte_flow_item_tcp tcp_mask;
> -    memset(&tcp_spec, 0, sizeof(tcp_spec));
> -    memset(&tcp_mask, 0, sizeof(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;
> @@ -4687,9 +4687,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>  
>      struct rte_flow_item_udp udp_spec;
>      struct rte_flow_item_udp udp_mask;
> -    memset(&udp_spec, 0, sizeof(udp_spec));
> -    memset(&udp_mask, 0, sizeof(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;
>  
> @@ -4708,9 +4708,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>  
>      struct rte_flow_item_sctp sctp_spec;
>      struct rte_flow_item_sctp sctp_mask;
> -    memset(&sctp_spec, 0, sizeof(sctp_spec));
> -    memset(&sctp_mask, 0, sizeof(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;
>  
> @@ -4729,9 +4729,9 @@ netdev_dpdk_add_rte_flow_offload(struct netdev *netdev,
>  
>      struct rte_flow_item_icmp icmp_spec;
>      struct rte_flow_item_icmp icmp_mask;
> -    memset(&icmp_spec, 0, sizeof(icmp_spec));
> -    memset(&icmp_mask, 0, sizeof(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);
>  
> 


More information about the dev mailing list