Hi Joe, Thank you for providing the comments on this series. Please see my answers below.
Regards _Sugesh > -----Original Message----- > From: Joe Stringer [mailto:[email protected]] > Sent: Wednesday, July 19, 2017 1:40 AM > To: Chandran, Sugesh <[email protected]> > Cc: ovs dev <[email protected]>; Andy Zhou <[email protected]>; Zoltán > Balogh <[email protected]> > Subject: Re: [PATCH v3 3/3] tunneling: Avoid datapath-recirc by combining > recirc actions at xlate. > > On 18 July 2017 at 10:49, Sugesh Chandran <[email protected]> > wrote: > > This patch set removes the recirculation of encapsulated tunnel > > packets if possible. It is done by computing the post tunnel actions > > at the time of translation. The combined nested action set are > > programmed in the datapath using CLONE action. > > > > The following test results shows the performance improvement offered > > by this optimization for tunnel encap. > > > > +-------------+ > > dpdk0 | | > > -->o br-in | > > | o--> gre0 > > +-------------+ > > > > --> LOCAL > > +-----------o-+ > > | | dpdk1 > > | br-p1 o--> > > | | > > +-------------+ > > > > Test result on OVS master with DPDK 16.11.2 (Without optimization): > > > > # dpdk0 > > > > RX packets : 7037641.60 / sec > > RX packet errors : 0 / sec > > RX packets dropped : 7730632.90 / sec > > RX rate : 402.69 MB/sec > > > > # dpdk1 > > > > TX packets : 7037641.60 / sec > > TX packet errors : 0 / sec > > TX packets dropped : 0 / sec > > TX rate : 657.73 MB/sec > > TX processing cost per TX packets in nsec : 142.09 > > > > Test result on OVS master + DPDK 16.11.2 (With optimization): > > > > # dpdk0 > > > > RX packets : 9386809.60 / sec > > RX packet errors : 0 / sec > > RX packets dropped : 5381496.40 / sec > > RX rate : 537.11 MB/sec > > > > # dpdk1 > > > > TX packets : 9386809.60 / sec > > TX packet errors : 0 / sec > > TX packets dropped : 0 / sec > > TX rate : 877.29 MB/sec > > TX processing cost per TX packets in nsec : 106.53 > > > > The offered performance gain is approx 30%. > > > > Signed-off-by: Sugesh Chandran <[email protected]> > > Signed-off-by: Zoltán Balogh <[email protected]> > > Co-authored-by: Zoltán Balogh <[email protected]> > > --- > > Hi Sugesh, > > I have some brief feedback below. Looks like this is getting close. [Sugesh] Happy to see that and thank you for quick feedback :) > > <snip> > > > @@ -279,3 +289,21 @@ xlate_cache_delete(struct xlate_cache *xcache) > > xlate_cache_uninit(xcache); > > free(xcache); > > } > > + > > +/* Append all the entries in src into dst and remove them from src. > > + * The caller must own both xc-caches to use this function. > > + * The 'src' entries are not freed in this function as its owned by caller. > > + */ > > +void xlate_cache_steal_entries(struct xlate_cache *dst, struct > > + xlate_cache *src) > > Usually, the type definition and the variable are kept on the same line. > Would you mind updating this to follow the surrounding code style? [Sugesh] Sure, Will change in the next series. > > > +{ > > + if (!dst || !src) { > > + return; > > + } > > + void *p; > > + struct ofpbuf *src_entries = &src->entries; > > + struct ofpbuf *dst_entries = &dst->entries; > > + p = ofpbuf_put_zeros(dst_entries, src_entries->size); > > + nullable_memcpy(p, src_entries->data, src_entries->size); > > + ofpbuf_clear(src_entries); > > I don't think we need to zero the buffer if we're going to write into it, so > we > could use ofpbuf_put_uninit() instead. That function will never return a NULL > pointer, so a regular memcpy should be sufficient. [Sugesh] Ok, Will change these lines to, + p = ofpbuf_put_uninit(dst_entries, src_entries->size); + memcpy(p, src_entries->data, src_entries->size); > > > @@ -134,11 +142,13 @@ struct xlate_cache { void > > xlate_cache_init(struct xlate_cache *); struct xlate_cache > > *xlate_cache_new(void); struct xc_entry *xlate_cache_add_entry(struct > > xlate_cache *, enum xc_type); -void xlate_push_stats_entry(struct > > xc_entry *, const struct dpif_flow_stats *); -void > > xlate_push_stats(struct xlate_cache *, const struct dpif_flow_stats > > *); > > +void xlate_push_stats_entry(struct xc_entry *, struct dpif_flow_stats > > +*); void xlate_push_stats(struct xlate_cache *, struct > > +dpif_flow_stats *); > > void xlate_cache_clear_entry(struct xc_entry *); void > > xlate_cache_clear(struct xlate_cache *); void > > xlate_cache_uninit(struct xlate_cache *); void > > xlate_cache_delete(struct xlate_cache *); > > +void xlate_cache_steal_entries(struct xlate_cache *, struct > > +xlate_cache *); > > + > > There's some extra unnecessary whitespace here. [Sugesh] Will remove it. > > > +/* Validate if the transalated combined actions are OK to proceed. > > + * If actions consist of TRUNC action, it is not allowed to do the > > + * tunnel_push combine as it cannot update stats correctly. > > + */ > > +static bool > > +is_tunnel_actions_clone_ready(struct xlate_ctx *ctx) { > > + struct nlattr *tnl_actions; > > + const struct nlattr *a; > > + unsigned int left; > > + size_t actions_len; > > + struct ofpbuf *actions = ctx->odp_actions; > > + > > + if (!actions) { > > + /* No actions, no harm in doing combine. */ > > + return true; > > + } > > + > > + /* Cannot perform tunnel push on slow path action > CONTROLLER_OUTPUT. */ > > + if (!ctx->xout->avoid_caching && > > + (ctx->xout->slow & SLOW_CONTROLLER)) { > > Even if we are avoiding caching the flow, I still think that the controller > action > cannot be correctly handled through this new path so we should return false. > Do you have a particular condition in mind for why we need to care about the > 'avoid_caching' flag in this case? [Sugesh] I don’t have any specific case as such. Will remove it and change the condition to + if (ctx->xout->slow & SLOW_CONTROLLER)) { > > > +static bool > > +validate_and_combine_post_tnl_actions(struct xlate_ctx *ctx, > > + const struct xport *xport, > > + struct xport *out_dev, > > + struct ovs_action_push_tnl > > +tnl_push_data) { > ... > > + if (ctx->odp_actions->size > push_action_size) { > > + /* Update the CLONE action only when combined. */ > > + nl_msg_end_nested(ctx->odp_actions, clone_ofs); > > + } else { > > + /* No actions after the tunnel, no need of clone. */ > > + nl_msg_cancel_nested(ctx->odp_actions, clone_ofs); > > + odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data); > > I looked at this line again for this version since I didn't understand it > last time > around, and I'm still a bit confused. If there's no actions to run on the > second > bridge, then the copy of the packet which is tunneled is effectively dropped. > If that copy of the packet is dropped, then why do we need the tunnel action > at all? If I follow correctly, then this means that if you have two bridges, > where the first bridge has output(tunnel),output(other_device) then the > second bridge where the tunneling occurs has no flows, then the datapath > flow will end up as something like: > > push_tnl(...),output(other_device). > > I realise that the testsuite breaks if you remove this line, but maybe the > testsuite needs fixing for these cases? [Sugesh] Actually as I mentioned earlier, this case was not really needed for any usecase. (As far I know). I totally agree that its not necessary to push tunnel when there are no actions afterwards. But some of the tunnel and OVN test cases are failing because of this. Do you want to fix the test suits as part of this series.? > > > + } > > + > > +out: > > + /* Restore context status. */ > > + ctx->xin->resubmit_stats = backup_resubmit_stats; > > + xlate_cache_delete(ctx->xin->xcache); > > + ctx->xin->xcache = backup_xcache; > > + ctx->xin->allow_side_effects = backup_side_effects; > > + ctx->xin->packet = backup_pkt; > > + ctx->wc = backup_flow_wc_ptr; > > + return nested_act_flag; > > + > > There's some extra unnecessary whitespace here. [Sugesh] Will remove it in the next series. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
