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

Darrell Ball dlu998 at gmail.com
Tue Jul 9 16:19:52 UTC 2019

Thanks for the review !

On Mon, Jul 8, 2019 at 2:58 PM Ben Pfaff <blp at ovn.org> wrote:

> 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?

it is about 7% pps increase for just 2 passes thru conntrack for UDP
So, yes.
There are 14 tests that exercise this new code path (in addition to the
negative test
added with this patch); these help mitigate the 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?

One of my early changes was to save the hash table lookup only, as you
it did not move the needle enough to warrant any changes.

Extraction savings has the most benefit.

> 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``.)

When I write new static functions I write them with the inline specifier to
build test them.
Then after I write the code to call them, I remove the inline specifier
(well, most of the time, apparently).

> 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);
>         }

yep :-)

> Thanks,
> Ben.

More information about the dev mailing list