On Tue, Mar 25, 2014 at 09:18:19PM -0700, Andy Zhou wrote:
> On Tue, Mar 25, 2014 at 7:19 PM, Simon Horman <ho...@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 <ho...@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 <ho...@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
> >> >
> >>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
