> It seems a little odd to make the mirror code keep a map from > ofproto_dpif to mbridge, instead of making ofproto_dpif keep an > mbridge pointer. I think you might have mentioned the reason to me in > person, but I don't remember what it was.
Yeah turns out that wasn't necessary. I ditched this approach for a reference counted struct mbridge handle. > It seems like the direct cost/benefit ratio for this change is low. > There's a lot of various kinds of extra overhead (memory, time, code) > for relatively little benefit. That's too bad. I hope that the later > modularization makes it worth it. The main issue here is keeping mirroring up to date between ofproto-dpif and ofproto-dpif-xlate in the next patch. It could be done without this patch, and I did have a prototype which did it, but it was such a clunky pain that I think this patch is worth it. That said, perhaps there's a simpler way to go about it, we can always refactor it later. > The interface for mirror_get_ffs() is really strange. I would have > the caller pass in a mirror index, not a bitmask from which the > function extracts the lowest 1-bit. > > Please don't use ovs_assert() on an expression that must be evaluated. > It's true that ovs_assert() cannot be disabled currently, but I'd like > to leave open that possibility later. Both of these points are fixed in the version I'll be sending out shortly. > add_mirror_actions() leaves me a little unsatisfied. It seems to me > that one could write a function of the form > > mirror_mask_t > mirror_get_outputs(const struct ofproto_dpif *, > mirror_mask mirrors, > struct of_bundle *out_bundles[MAX_MIRRORS + 1], > int out_vlans[MAX_MIRRORS + 1], > struct flow_wildcards *wc); Given the refactoring I've done in the new version of the previous patch. I decided to shelve this suggestion for now as add_mirror_actions() is pretty much the same as it's always been. Let me know if you feel strongly about it and I can revisit. > In run(), it's a little odd to bother making mac_learning_flush() > report the invalidated tags, since we're doing a full need_revalidate > anyway. I see that the original code did the same; I didn't notice > that it was odd before. Changed in the new version. Ethan X-CudaMail-Whitelist-To: dev@openvswitch.org _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
