[ovs-dev] [RFC recirc fix] recirculation: Map recirc_id to ofproto_dpif.

[Alex Wang]

Thx Andy for the review,

Please see my reply below,


> + * When recirc_id is set in 'flow', checks whether the ofproto_dpif that
> > + * corresponds to the recirc_id is same as the receiving bridge.  If
> they are
> > + * the same, uses the 'ofproto' and keeps the 'ofp_in_port' as assigned.
> > + * Otherwise, uses the 'recirc_ofproto' that owns recirc_id and assigns
> > + * OFPP_NONE to 'ofp_in_port'.  Doing this is in that, for the latter
> case,
> > + * the rules that match the recirculated packets are installed only to
> the
> > + * 'recirc_ofproto', and using OFPP_NONE avoids the case where the
> in_port is
> > + * taken or is non-exist.
> It is probably worth noting that post recirculation flow (with a non
> zero recirc_id) should not be allowed to cross a patch port.
>


Sure, I'll mention this,



> > +        if (recirc_ofproto != ofproto) {
> > +            *ofp_in_port = OFPP_NONE;
> > +            ofproto = recirc_ofproto;
> > +        }
> > +    }
> Would it make sense to move the logic above into xlate_lookup_ofproto_ ?
>


Yes, I'll put it inside xlate_lookup_ofproto_(), since both are for getting
the
ofproto,



> > +static void
> > +dpif_backer_recirc_clear_ofproto(struct dpif_backer *backer,
> > +                                 struct ofproto_dpif *ofproto)
> > +{
> > +    struct dpif_backer_recirc_node *node;
> > +
> > +    ovs_mutex_lock(&backer->recirc_mutex);
> > +    CMAP_FOR_EACH (node, cmap_node, &backer->recirc_map) {
> > +        if (node->ofproto == ofproto) {
> > +            cmap_remove(&backer->recirc_map, &node->cmap_node,
> > +                        node->recirc_id);
>
> This is really an error condition, right?  Before a ofproto is
> removed, all resources
> including recirc id should already be freed.  On the flip side, if
> this code actually
> found any mapping remaining in cmap, the recirc_id is then leaked.
>


Makes sense, I'll put a warning there,



> >  uint32_t
> >  ofproto_dpif_alloc_recirc_id(struct ofproto_dpif *ofproto)
> >  {
> >      struct dpif_backer *backer = ofproto->backer;
> > +    struct dpif_backer_recirc_node *node = xmalloc(sizeof *node);
> > +    uint32_t recirc_id = recirc_id_alloc(backer->rid_pool);
> >
> > -    return  recirc_id_alloc(backer->rid_pool);
> recirc_id == 0 indicate failure of allocating the id (i.e. out of id
> pool). We may not want to add it
> to cmap in this case.
>


Thx for pointing it out, I'll add a check,



> > +    node = CONTAINER_OF(cmap_find(&backer->recirc_map, recirc_id),
> > +                        struct dpif_backer_recirc_node, cmap_node);
> > +    if (node) {
> > +        ovs_mutex_lock(&backer->recirc_mutex);
> > +        cmap_remove(&backer->recirc_map, &node->cmap_node,
> node->recirc_id);
> > +        recirc_id_free(backer->rid_pool, node->recirc_id);
> The locking looks strange. recirc_id_alloc/free uses its own locking.
> There is really no need
> to call it within the recirc_mutex lock. The alloc() call above does
> it outside.  May be we can
> drop id_pool lock and only use recirc_mutex lock.
>


I'll move recirc_id_alloc/free() outside the critical section,

I think we still need to id_pool lock, since other user of the module
may need it,



> > +   set interface p1 type=dummy
> options:pstream=punix:$OVS_RUNDIR/p1.sock ofport_request=1 -- \
> > +   set interface p2 type=dummy
> options:pstream=punix:$OVS_RUNDIR/p2.sock ofport_request=2 -- \
> > +   add-br br1 -- \
> > +   set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> > +   set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> > +       fail-mode=standalone -- \
> > +   add-bond br1 bond1 p3 p4 bond_mode=balance-tcp lacp=active \
> > +       other-config:lacp-time=fast
> other-config:bond-rebalance-interval=0 -- \
> > +   set interface p3 type=dummy options:stream=unix:$OVS_RUNDIR/p1.sock
> ofport_request=3 -- \
> > +   set interface p4 type=dummy options:stream=unix:$OVS_RUNDIR/p2.sock
> ofport_request=4 -- \
> > +   add-port br1 br1- -- set interface br1- type=patch options:peer=br1+
> ofport_request=100 -- \
> > +   set port br1- tag=1 -- \
> > +   add-br br-int -- \
> > +   set bridge br-int other-config:hwaddr=aa:77:aa:77:00:00 -- \
> > +   set bridge br-int datapath-type=dummy other-config:datapath-id=1235 \
> > +       fail-mode=secure -- \
> > +   add-port br-int br1+ -- set interface br1+ type=patch
> options:peer=br1- ofport_request=101 -- \
> > +   add-port br-int p5 -- set interface p5 ofport_request=5 type=dummy
> > +])
> > +AT_CHECK([ovs-appctl netdev-dummy/set-admin-state up], 0, [OK
> > +])
> > +
> > +# The dl_vlan flow should not be ever matched,
> > +# since recirculation should not change the flow handling.
> > +AT_DATA([flows.txt], [dnl
> > +table=0 priority=1 in_port=5 actions=output(101)
> According to the commit comment, should there be a push vlan action as
> well ?
>

I tag the patch port, which is equivalent to setting the vlan.  I agree
this is
very unclear, I'll add a vlan action explicitly!

> +   set port br1- tag=1 -- \