Thx for the review, Jarno,
I find it conceivable that post recirculation flows would be able to cross
> a later patch port in the pipeline. Therefore, rather than stating that
> “post recirculation flows are not allowed to cross a patch port”, I think
> we should say that recirculated flows must be processed by the ofproto that
> originated the recirculation, and that as bridges may only see their own
> ports, the in_port of another bridge is not passed to the recirc_ofproto.
>
> Should also comment here that the OFPP_NONE limits the recirc_ofproto from
> meaningfully matching on in_port post recirculation and that making the
> original patch port visible as an in_port is work-in-progress.
Thx for the suggested comments, I'll put them in.
> > +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) {
> > + VLOG_ERR("recirc_id %"PRIu32", not freed when ofproto (%s) "
> > + "is destructed", node->recirc_id, ofproto->up.name
> );
> > + cmap_remove(&backer->recirc_map, &node->cmap_node,
> > + node->recirc_id);
>
> Should the nodes also be freed? See also comment below.
Thx a lot for pointing it out, really bad to introduce them...
Add 'free(node)' in both places
> > ofproto_dpif_alloc_recirc_id(struct ofproto_dpif *ofproto)
> > {
> > struct dpif_backer *backer = ofproto->backer;
> > + uint32_t recirc_id = recirc_id_alloc(backer->rid_pool);
> >
> > - return recirc_id_alloc(backer->rid_pool);
> > + if (recirc_id) {
> > + struct dpif_backer_recirc_node *node = xmalloc(sizeof *node);
> > +
> > + node->recirc_id = recirc_id;
> > + node->ofproto = ofproto;
> > +
> > + ovs_mutex_lock(&backer->recirc_mutex);
> > + cmap_insert(&backer->recirc_map, &node->cmap_node,
> node->recirc_id);
> > + ovs_mutex_unlock(&backer->recirc_mutex);
> > + }
>
> It might be worthwhile sometime later to add an ‘aux’ pointer to the
> id_pool’s id_node so that we would not need two maps here (id_pool should
> be moved from a hmap to cmap to get lockless lookups, though.)
>
Makes sense!
Ready to apply/backport the patch~!
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
