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 -- \
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
