[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
describe;
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.
>

yep


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