[ovs-dev] [patch v5] conntrack: Optimize recirculations.

Ben Pfaff blp at ovn.org
Mon Jul 8 21:57:56 UTC 2019


On Sun, Jun 09, 2019 at 07:32:03AM -0700, Darrell Ball wrote:
> Cache the 'conn' context and use it when it is valid.  The cached 'conn'
> context will get reset if it is not expected to be valid; a negative test
> is added to check this.
> 
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
> ---
> 
> v5: Check for alg ctl on recirculation to handle potential corner case.
>     Remove unnecessary 'commit' filter.
> 
> v4: Reset 'conn' cache context automatically when tuple changes, rather than
>     needing an explicit 'ct_clear' action.  Need to check if all cases are
>     handled.
> v3: Remove unneeded 'NAT' field added to 'struct conn_lookup_ctx'.

It looks like this got missed for review.  Oops.

This is nice!  I guess RCU makes it safe to keep a "struct conn *"
pointer around without worrying about it too much?

How much time does this save?  Do you have an idea where the savings
come from?  I have two motivations for asking:

   1. Do the savings warrant the (slight) complexity and (some) risk?

   2. I think that the cost that this saves can be divided into
      extraction and hash table lookup.  If the hash table lookup is the
      expensive part, then we could skip the work here on invalidating
      when the packet changes, instead always doing extraction and then
      comparing against the cache entry.

Your thoughts?

The 'conn' member can be declared as type "struct conn *" rather than
"void *", for slightly better safety.

Please don't mark functions in .c files as 'inline' without some
exceptional reason.  coding-style.rst says why:

    Functions in ``.c`` files should not normally be marked ``inline``,
    because it does not usually help code generation and it does
    suppress compiler warnings about unused functions. (Functions
    defined in ``.h`` usually should be marked ``inline``.)

All the "continue;"s in conntrack_execute() start to look odd.  Maybe
like this instead?

        if (OVS_UNLIKELY(packet->md.ct_state == CS_INVALID)) {
            write_ct_md(packet, zone, NULL, NULL, NULL);
        } else if (conn && conn->key.zone == zone && !force
                   && !get_alg_ctl_type(packet, tp_src, tp_dst, helper)) {
            process_one_fast(zone, setmark, setlabel, nat_action_info,
                             conn, packet);
        } else if (OVS_UNLIKELY(!conn_key_extract(ct, packet, dl_type, &ctx,
                                zone))) {
            packet->md.ct_state = CS_INVALID;
            write_ct_md(packet, zone, NULL, NULL, NULL);
        } else {
            process_one(ct, packet, &ctx, zone, force, commit, now, setmark,
                        setlabel, nat_action_info, tp_src, tp_dst, helper);
        }

Thanks,

Ben.


More information about the dev mailing list