On Mon, Feb 03, 2014 at 05:12:12PM -0800, Ben Pfaff wrote:
> On Wed, Jan 29, 2014 at 12:53:03PM +0900, Simon Horman wrote:
> > This reworks lookup of rules for both table 0 and table action translation.
> > The result is that Table Mod settings, which can alter the miss-behaviour
> > of tables, including table 0, on a per-table basis may be honoured.
> >
> > Previous patches proposed by myself which build on earlier merged patches
> > by Andy Zhou implement the ofproto side of Table Mod. So with this patch
> > the feature should be complete.
> >
> > Neither this patch, nor any other patches it builds on, alter the default
> > behaviour of Open vSwitch. And in particular the OpenFlow1.1 behaviour is
> > the default regardless of which OpenFlow version is negotiated between the
> > switch and the controller.
> >
> > An implementation detail, which lends itself to future work, is the
> > handling of OFPTC_TABLE_MISS_CONTINUE. If a table has this behaviour set by
> > Table Mod and a miss occurs then a loop is created, skipping to the next
> > table. It is quite easy to create a situation where this loop covers ~255
> > tables which is very expensive as the lookup for each table involves taking
> > locks, amongst other things.
> >
> > Cc: Andy Zhou <az...@nicira.com>
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
Sorry for missing this and posting v6.
> This commit reads more naturally to me when we fold in the following.
> (I haven't tested it yet to make sure the tests still pass, but I
> don't intend this to change behavior.)
Thanks, I'll look at squashing it (or some variant of it that works)
into my next post.
>
> Reading through, what really bothers me here is the significant
> apparent code duplication between xlate_table_action() and
> rule_dpif_lookup(). Do you see a way to avoid that?
I agree that its unfortunate but it didn't seem obvious to me
how it could cleanly be avoided. I'll look over it again.
> I don't think that "resubmit" should honor OFPTC11_TABLE_MISS_*. It
> would be pretty unexpected behavior, I think.
Thanks, I will fix that.
> I think that we need to work on stats. OpenFlow specifies per-table
> lookup and match counts, but it looks like we squash all of them into
> a single pair of stats and attribute those to table 0. I guess we
> need one miss_rule and one packet_in_rule per table, not just one of
> each globally. I don't think that has to happen in this patch,
> though.
Agreed, I think there is work to be done there.
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 8ecf4ca..fe0ca79 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2972,7 +2972,7 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
> }
>
> /* Lookup 'flow' in table 0 of 'ofproto''s classifier.
> - * If 'wc' is non-null, sets * the fields that were relevant as part of
> + * If 'wc' is non-null, sets the fields that were relevant as part of
> * the lookup. Returns the table_id where a match or miss occurred.
> *
> * The return value will be zero unless there was a miss and
> @@ -3080,16 +3080,13 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
> const struct flow *flow, struct flow_wildcards
> *wc,
> uint8_t *table_id, struct rule_dpif **rule)
> {
> - while (1) {
> + while (*table_id < ofproto->up.n_tables) {
> enum ofp_table_config config;
>
> if (rule_dpif_lookup_in_table(ofproto, flow, wc, *table_id, rule)) {
> return RULE_DPIF_LOOKUP_VERDICT_MATCH;
> }
>
> - config = table_get_config(&ofproto->up, *table_id);
> - config &= OFPTC11_TABLE_MISS_MASK;
> -
> /* XXX
> * This does not take into account different
> * behaviour for different OpenFlow versions
> @@ -3100,36 +3097,25 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
> *
> * Instead the global default is OFPTC_TABLE_MISS_CONTROLLER
> * which may be configured globally using Table Mod. */
> + config = table_get_config(&ofproto->up, *table_id);
> switch (config & OFPTC11_TABLE_MISS_MASK) {
> - case OFPTC11_TABLE_MISS_CONTINUE: {
> - uint8_t next_id = *table_id + 1;
> -
> - if (next_id == TBL_INTERNAL) {
> - next_id++;
> - }
> -
> - if (next_id < ofproto->up.n_tables) {
> - /* XXX
> - * This may get very expensive as each call to
> - * rule_dpif_lookup_in_table() and table_get_config() is
> - * expensive and this may occur up to approximately
> - * ofproto->up.n_tables times. */
> - *table_id = next_id;
> - continue;
> - }
> - /* Fall through to OFPTC_TABLE_MISS_CONTROLLER
> - * as this is the last table in the pipeline */
> - }
> + case OFPTC11_TABLE_MISS_CONTINUE:
> + break;
> case OFPTC11_TABLE_MISS_CONTROLLER:
> return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
> case OFPTC11_TABLE_MISS_DROP:
> return RULE_DPIF_LOOKUP_VERDICT_DROP;
> - default:
> - OVS_NOT_REACHED();
> + }
> +
> + /* Go on to next table. */
> + ++*table_id;
> + if (*table_id == TBL_INTERNAL) {
> + ++*table_id;
> }
> }
>
> - OVS_NOT_REACHED();
> + /* We fell off the */
> + return RULE_DPIF_LOOKUP_VERDICT_CONTROLLER;
> }
>
> /* Given a port configuration (specified as zero if there's no port), chooses
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
