On Thu, Jan 16, 2014 at 03:50:31PM -0800, Ben Pfaff wrote:
> On Wed, Jan 15, 2014 at 04:13:21PM +0900, Simon Horman wrote:
> > This is an proposed enhancement to
> > "Implement OpenFlow support for MPLS, for up to 3 labels."
> >
> > It is in preparation for not adding flows to the datapath
> > which have more MPLS LSEs than the datapath supports.
> >
> > Signed-off-by: Simon Horman <[email protected]>
>
> Thanks a lot!
>
> I think that every iteration of the array probed the same depth, so I
> folded this in:
>
> - flow_set_mpls_bos(&flow, ARRAY_SIZE(flow.mpls_lse) - 1, 1);
> + flow_set_mpls_bos(&flow, i, 1);
Thanks, I thought that I had fixed that, but I guess not.
> I decided that probing from 0 up, instead of from the max down, was a
> slight improvement, because if you run into a funny error along the
> way you can always return the best you've seen so far instead of
> failing all the way back to 0.
>
> I decided that DPIF_FP_CREATE | DPIF_FP_MODIFY made more sense than
> just DPIF_FP_CREATE, because I wouldn't want some leftover flow from a
> previous run to cause a problem.
>
> I decided that EEXIST was actually a form of success: it means we hit
> some overlapping flow, which in turn means that the flow we're trying
> to add is OK.
>
> I decided that 'n' was a better name for the variable than 'i' ;-)
Thanks, all that sounds reasonable to me.
> Here's what I'm folding in:
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 1fe5957..b9811b5 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -981,47 +981,41 @@ check_variable_length_userdata(struct dpif_backer
> *backer)
> static size_t
> check_max_mpls_depth(struct dpif_backer *backer)
> {
> - size_t i;
> struct flow flow;
> - int error;
> + size_t n;
>
> - /* If ARRAY_SIZE(flow.mpls_lse) becomes large then it might
> - * make more sense to do a binary search here. */
> - for (i = ARRAY_SIZE(flow.mpls_lse) - 1; i > 0; i--) {
> + for (n = 0; n < ARRAY_SIZE(flow.mpls_lse); n++) {
> struct odputil_keybuf keybuf;
> struct ofpbuf key;
> + int error;
>
> memset(&flow, 0, sizeof flow);
> flow.dl_type = htons(ETH_TYPE_MPLS);
> - flow_set_mpls_bos(&flow, ARRAY_SIZE(flow.mpls_lse) - 1, 1);
> + flow_set_mpls_bos(&flow, n, 1);
>
> ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> odp_flow_key_from_flow(&key, &flow, 0);
>
> - error = dpif_flow_put(backer->dpif, DPIF_FP_CREATE, key.data,
> key.size,
> - NULL, 0, NULL, 0, NULL);
> - if (!error) {
> - error = dpif_flow_del(backer->dpif, key.data, key.size, NULL);
> - if (error) {
> - goto err;
> + error = dpif_flow_put(backer->dpif, DPIF_FP_CREATE | DPIF_FP_MODIFY,
> + key.data, key.size, NULL, 0, NULL, 0, NULL);
> + if (error && error != EEXIST) {
> + if (error != EINVAL) {
> + VLOG_WARN("%s: MPLS stack length feature probe failed (%s)",
> + dpif_name(backer->dpif), ovs_strerror(error));
> }
> - i++;
> break;
> - } else if (error != EINVAL) {
> - goto err;
> + }
> +
> + error = dpif_flow_del(backer->dpif, key.data, key.size, NULL);
> + if (error) {
> + VLOG_WARN("%s: failed to delete MPLS feature probe flow",
> + dpif_name(backer->dpif));
> }
> }
>
> VLOG_INFO("%s: MPLS label stack length probed as %"PRIuSIZE,
> - dpif_name(backer->dpif), i);
> - return i;
> -
> -err:
> - /* Something odd happened. We're not sure how large an
> - * MPLS label stack the datapath supports. Default to 0. */
> - VLOG_WARN("%s: mpls stack length feature probe failed (%s)",
> - dpif_name(backer->dpif), ovs_strerror(error));
> - return 0;
> + dpif_name(backer->dpif), n);
> + return n;
> }
>
> static int
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
