[ovs-dev] [PATCH/RFC 4/8] ofproto-dpif: Add recirc_id field to struct rule_dpif

[Simon Horman]

Hi Andy,

to date, the implementation of MPLS has not provided any entity other
than the rules themselves.

This being the case it is not the label that defines the behaviour after
recirculation but rather what is done to a packet after the label stack is
modified. It is quite conceivable (though possibly not useful) to construct
different rules which use the same label in different ways.  Or to have
rules which require recirculation for MPLS but are not tied to a specific
label. An example of the latter is match=mpls actions=pop_mpls:0x800,dec_ttl.

To turn the tables around. I was somewhat surprised when I initially read
your implementation to see that you were using a higher-level entity than
rules in conjunction with recirculation for bonding. But I see that it
makes quite a lot of sense in the case of bonding.

Assuming I have understood it correctly, I think that the infrastructure
you have created should work quite nicely both with and without a
higher-level entity. As I hope my patchset demonstrates.

On Tue, Mar 25, 2014 at 02:54:46PM -0700, Andy Zhou wrote:
> Simon, I am not sure If I understand how recirc_id should be managed for MPLS.
> 
> Why should rule allocate and free recirc_id by itself, rather that
> some other entity managing them?  In the bond case, bond is one
> managing recirc_ids. one recirc_id per bond.  I'd imaging some entity
> in the user space should keep track of the mpls labels and their
> mapping to recirc_id?
> 
> 
> On Tue, Mar 25, 2014 at 2:24 PM, Simon Horman <horms at verge.net.au> wrote:
> > This is to allow a recirculation id to be associated with a rule
> > in the case that its actions cause recirculation.
> >
> > In such a case if the recirc_id field is non-zero then that value should be
> > used, otherwise a value should be obtained using
> > ofproto_dpif_alloc_recirc_id and saved in recirc_id field.
> >
> > When destructing the rule if the recirc_id field is non-zero then
> > the associated internal flow should be deleted.
> >
> > This is in preparation for using the same helper as part of support
> > for using recirculation in conjunction series of actions including
> > with MPLS actions that are currently not able to be translated.
> >
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> > ---
> >  ofproto/ofproto-dpif.c | 27 +++++++++++++++++++++++++++
> >  ofproto/ofproto-dpif.h |  1 +
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 05f7bca..f239735 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -89,6 +89,12 @@ struct rule_dpif {
> >       *   recently been processed by a revalidator. */
> >      struct ovs_mutex stats_mutex;
> >      struct dpif_flow_stats stats OVS_GUARDED;
> > +
> > +    /* If non-zero then the recirculation id that has
> > +     * been allocated for use with this rule.
> > +     * The recirculation id and associated internal flow should
> > +     * be freed when the rule is freed */
> > +    uint32_t recirc_id;
> >  };
> >
> >  static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes,
> > @@ -3154,6 +3160,19 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
> >      return rule_get_actions(&rule->up);
> >  }
> >
> > +/* Returns 'rule''s recirculation id. */
> > +uint32_t
> > +rule_dpif_get_recirc_id(struct rule_dpif *rule)
> > +    OVS_REQUIRES(rule->up.mutex)
> > +{
> > +    if (!rule->recirc_id) {
> > +        struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> > +
> > +        rule->recirc_id = ofproto_dpif_alloc_recirc_id(ofproto);
> > +    }
> > +    return rule->recirc_id;
> > +}
> > +
> >  static uint8_t
> >  rule_dpif_lookup__ (struct ofproto_dpif *ofproto, const struct flow *flow,
> >                      struct flow_wildcards *wc, struct rule_dpif **rule)
> > @@ -3376,6 +3395,8 @@ rule_construct(struct rule *rule_)
> >      rule->stats.n_packets = 0;
> >      rule->stats.n_bytes = 0;
> >      rule->stats.used = rule->up.modified;
> > +    rule->recirc_id = 0;
> > +
> >      return 0;
> >  }
> >
> > @@ -3399,7 +3420,13 @@ static void
> >  rule_destruct(struct rule *rule_)
> >  {
> >      struct rule_dpif *rule = rule_dpif_cast(rule_);
> > +
> >      ovs_mutex_destroy(&rule->stats_mutex);
> > +    if (rule->recirc_id) {
> > +        struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> > +
> > +        ofproto_dpif_free_recirc_id(ofproto, rule->recirc_id);
> > +    }
> >  }
> >
> >  static void
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> > index 6d21bac..9a07d34 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -101,6 +101,7 @@ bool rule_dpif_is_internal(const struct rule_dpif *);
> >  uint8_t rule_dpif_get_table(const struct rule_dpif *);
> >
> >  struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *);
> > +uint32_t rule_dpif_get_recirc_id(struct rule_dpif *rule);
> >
> >  ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
> >
> > --
> > 1.8.4
> >
>