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

Simon Horman horms at verge.net.au
Wed Mar 26 11:00:37 UTC 2014


On Tue, Mar 25, 2014 at 09:18:19PM -0700, Andy Zhou wrote:
> On Tue, Mar 25, 2014 at 7:19 PM, Simon Horman <horms at verge.net.au> wrote:
> > Hi Andy,
> >
> > to date, the implementation of MPLS has not provided any entity other
> > than the rules themselves.
> 
> I see. I need to step back to understand the bigger picture more. I
> have not been closely following the MPLS patches in the past.
> >
> > 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.
> 
> I understand and agree.  What would be the best way to describe how
> recirc_id is used in implementing mpls?  My best understanding of
> reading this patch series is each recirc_id represent a MPLS (or vlan)
> label stack change. Recirculation helps to break up actions that
> applies at different label stack level.  Is this correct?
> >
> > 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.
> >
> Point taken.  I am still not sure I understand why there should be only one
> recirc_id per rule.

I'm not sure that there should be only one recirc_id per rule.
But rather one every time a rule makes a transition that
requires recirculation.

I'm not sure that my implementation meets that need but that
was the intention.

> > 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.
> 
> The design intent is that the recirculation infrastructure should be
> general enough for multiple features.  I'd like to study the patch
> series in more detail to make sure it works for mpls.  Having
> higher-level entity or not is a detailed  issue that we should be able
> flush out soon.
> 
> >
> > 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
> >> >
> >>
> 



More information about the dev mailing list