Thanks for the review ! On Mon, Jul 8, 2019 at 2:58 PM Ben Pfaff <[email protected]> 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 <[email protected]> > > --- > > > > 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. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
