[ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation

Ben Pfaff blp at ovn.org
Mon Nov 13 19:02:22 UTC 2017


Here is an idea that just occurred to me.  What if we used a kind of
copy-on-write approach?  That is, whenever the flow is modified, we
first check whether we need to do a "backup" of it?  This should reduce
the number of copies greatly, because flow modifications are relatively
rare compared to, say, "resubmit" actions.  It will require a little
work to make the code in ofproto-dpif-xlate call the right function to
do the copying, but I think that it would be easier to implement and
maintain than the approaches you suggest (assuming that I understand
them correctly).

On Mon, Nov 13, 2017 at 05:28:24PM +0000, Zoltán Balogh wrote:
> Hello Ben,
> 
> Thank you for the review. I have been thinking about possible solutions.
> 
> 1) We could use non-temporary (non-cached) copy when backing up data. That would avoid using write cache and make writing memory faster. In turn reading data back right after the write would be slower but that should not be the regular case. Furthermore it should not be available for all hardware platforms.
> 
> 2) Sugesh came up with the idea to do conditional backup and restore of data. We should keep track if xlate flow, wildcard and base_flow are modified in translation and only copy needed fields. I think this would be the best choice but has large code impact and would make code maintenance more complicated.
> 
> 3) Another option would be to spilt up the flow structure into partitions. Then instead of copying the whole structure, partitions could be checked for change by using memcmp(), then do the copy in case of change. We could use something like this:
> 
> #define FLOW_PART_LEN(start, end) \
>     ((char *)&((struct flow *)0)->end) - ((char *)&((struct flow *)0)->start)
> 
> #define COMPARE_FLOW_PART(dst, src, start, end) \
>     memcmp(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> 
> #define COPY_FLOW_PART(dst, src, start, end) \
>     memcpy(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> 
> static void
> copy_altered_flow_data(struct flow *dst, const struct flow *src)
> {
>     BUILD_ASSERT_DECL(FLOW_WC_SEQ == 40);
> 
>     /* Metadata 1 */
>     if (!COMPARE_FLOW_PART(dst, src, tunnel, metadata)) {
>         COPY_FLOW_PART(dst, src, tunnel, metadata);
>     }
> 
>     /* Metadata 2 */
>     if (!COMPARE_FLOW_PART(dst, src, metadata, dl_dst)) {
>         COPY_FLOW_PART(dst, src, metadata, dl_dst);
>     }
> 
>     /* L2 */
>     if (!COMPARE_FLOW_PART(dst, src, dl_dst, nw_src)) {
>         COPY_FLOW_PART(dst, src, dl_dst, nw_src);
>     }
> 
>     /* L3 */
>     if (!COMPARE_FLOW_PART(dst, src, nw_src, tp_src)) {
>         COPY_FLOW_PART(dst, src, nw_src, tp_src);
>     }
> 
>     /* L4 */
>     if (!COMPARE_FLOW_PART(dst, src, tp_src, pad3)) {
>         COPY_FLOW_PART(dst, src, tp_src, pad3);
>     }
> }
> 
> static void
> store_ctx_xlate_data(struct xlate_backup_data *data,
>                      const struct xlate_ctx *ctx, bool force)
> {
>     data->odp_actions_size = ctx->odp_actions->size;
>     if (force) {
>         data->wc_masks = ctx->wc->masks;
>         data->flow = ctx->xin->flow;
>         data->base_flow = ctx->base_flow;
>     } else {
>         copy_altered_flow_data(&data->wc_masks, &ctx->wc->masks);
>         copy_altered_flow_data(&data->flow, &ctx->xin->flow);
>         copy_altered_flow_data(&data->base_flow, &ctx->base_flow);
>     }
> }
> 
> What do you think?
> 
> Best regards,
> Zoltan
> 
> 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp at ovn.org]
> > Sent: Tuesday, October 31, 2017 9:30 PM
> > To: Zoltán Balogh <zoltan.balogh at ericsson.com>
> > Cc: dev at openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation
> > 
> > On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> > > When all OF actions have been translated, there could be actions at
> > > the end of list of odp actions which are not needed to be executed.
> > > So, the list can be normalized at the end of xlate_actions().
> > >
> > > Signed-off-by: Zoltan Balogh <zoltan.balogh at ericsson.com>
> > > Signed-off-by: Sugesh Chandran <sugesh.chandran at intel.com>
> > > Co-authored-by: Sugesh Chandran <sugesh.chandran at intel.com>
> > > Tested-by: Sugesh Chandran <sugesh.chandran at intel.com>
> > 
> > Thanks for working on this.  I wasn't previously aware that there was a
> > problem, but now I see what you mean.
> > 
> > The description in 0/2 is necessary to properly understand the problem,
> > but that description will get lost when the patches are committed.  I
> > recommend adding all or most of it to patch 1.
> > 
> > This approach to saving and restoring is going to be super-expensive
> > during translation.  On i386, struct xlate_backup_data is 1996 bytes,
> > and as I read the patch, that's getting copied every time we visit a new
> > OpenFlow table.  Do you have any idea about how to make it cheaper, or
> > how to make fewer backups?
> > 
> > Thanks,
> > 
> > Ben.


More information about the dev mailing list