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:[email protected]]
> > Sent: Tuesday, October 31, 2017 9:30 PM
> > To: Zoltán Balogh <[email protected]>
> > Cc: [email protected]
> > 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 <[email protected]>
> > > Signed-off-by: Sugesh Chandran <[email protected]>
> > > Co-authored-by: Sugesh Chandran <[email protected]>
> > > Tested-by: Sugesh Chandran <[email protected]>
> >
> > 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
