[ovs-dev] [PATCH v2 2/8] dpif-netdev: retrieve flow directly from the flow mark

[Darrell Ball]

On 9/13/17, 7:50 PM, "Yuanhan Liu" <yliu at fridaylinux.org> wrote:

    On Wed, Sep 13, 2017 at 05:20:58AM +0000, Darrell Ball wrote:
    > 
    > 
    > On 9/5/17, 2:23 AM, "Yuanhan Liu" <yliu at fridaylinux.org> wrote:
    > 
    >     So that we could skip the heavy emc processing, notably, the
    >     miniflow_extract function. A simple PHY-PHY forwarding testing
    >     shows 53% performance improvement.
    >     
    >     Note that though the heavy miniflow_extract is skipped
    > 
    > [Darrell] How much of the increased performance is due to skipping 
    >                miniflow_extract ?
    
    For the PHY-PHY case, following are the performance gains for each case.
    
    with_miniflow_extract 22%                       
    with_parse_tcp_falgs  54%
    without both:         70%
    
    [...]
    >     +        if (dp_packet_has_flow_mark(packet, &flow_mark)) {
    >     +            flow = dp_netdev_pmd_find_flow_by_mark(pmd, flow_mark);
    >     +            if (flow) {
    >     +                tcp_flags = parse_tcp_flags(packet);
    >     +                dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
    >     +                                        n_batches);
    >     +                continue;
    >     +            }
    >     +        }
    >     +
    > 
    > [Darrell] As mentioned, you would move dp_netdev_pmd_find_flow_by_mark()  to following code; 
    >               also, maybe, you can get rid of parse_tcp_flags() as a result.
    
    I doubt it, as you see from above data, the performance impact due to
    miniflow_extract is huge.
    
    > 
    >              if (i != size - 1) {
    >                  struct dp_packet **packets = packets_->packets;
    >                  /* Prefetch next packet data and metadata. */
    >     @@ -4989,7 +5001,8 @@ emc_processing(struct dp_netdev_pmd_thread *pmd,
    >              /* If EMC is disabled skip emc_lookup */
    >              flow = (cur_min == 0) ? NULL: emc_lookup(flow_cache, key);
    >              if (OVS_LIKELY(flow)) {
    >     -            dp_netdev_queue_batches(packet, flow, &key->mf, batches,
    >     +            tcp_flags = miniflow_get_tcp_flags(&key->mf);
    >     +            dp_netdev_queue_batches(packet, flow, tcp_flags, batches,
    >                                          n_batches);
    >              } else {
    >                  /* Exact match cache missed. Group missed packets together at
    >     @@ -5166,7 +5179,9 @@ fast_path_processing(struct dp_netdev_pmd_thread *pmd,
    >              flow = dp_netdev_flow_cast(rules[i]);
    >      
    >              emc_probabilistic_insert(pmd, &keys[i], flow);
    >     -        dp_netdev_queue_batches(packet, flow, &keys[i].mf, batches, n_batches);
    >     +        dp_netdev_queue_batches(packet, flow,
    >     +                                miniflow_get_tcp_flags(&keys[i].mf),
    >     +                                batches, n_batches);
    >          }
    >      
    >          dp_netdev_count_packet(pmd, DP_STAT_MASKED_HIT, cnt - miss_cnt);
    >     diff --git a/lib/flow.c b/lib/flow.c
    >     index b2b10aa..912c538 100644
    >     --- a/lib/flow.c
    >     +++ b/lib/flow.c
    >     @@ -991,6 +991,84 @@ parse_dl_type(const struct eth_header *data_, size_t size)
    >          return parse_ethertype(&data, &size);
    >      }
    >      
    > 
    > [Darrell] Hopefully we can get rid of the following function which removes the changes to flow.c/.h
    >                 If not, we would have to splice out common code.
    
    To let parse_tcp_flags() and miniflow_extract() share more common code?

[Darrell]
It looks to me parse_tcp_flags() is distilled from miniflow_extract(), so, yes, that is what I meant.

    
    	--yliu