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.
<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?
> +{
> + 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.
> @@ -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.
> +/* 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?
> +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?
> + }
> +
> +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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
