Thanks, I will send out V4 based on the comments.
On Wed, Jan 15, 2014 at 2:27 PM, Jesse Gross <je...@nicira.com> wrote:
> On Wed, Jan 15, 2014 at 1:06 PM, Andy Zhou <az...@nicira.com> wrote:
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index b42fd8b..6ae4ecf 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -1305,10 +1306,13 @@ static void __dp_destroy(struct datapath *dp)
> > list_del_rcu(&dp->list_node);
> >
> > /* OVSP_LOCAL is datapath internal port. We need to make sure
> that
> > - * all port in datapath are destroyed first before freeing
> datapath.
> > - */
> > + * all ports in datapath are destroyed first before freeing
> datapath.
> > + */
> > ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
> >
> > + /* Remove all flows while holding ovs_mutex */
> > + ovs_flow_tbl_flush(&dp->table);
> > +
> > call_rcu(&dp->rcu, destroy_dp_rcu);
>
> It might be simpler to just destroy the table at this point -
> otherwise we flush now and then destroy later in the RCU callback
> (even though these operations are essentially the same thing).
>
> > diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> > index 4232b82..3ebea78 100644
> > --- a/datapath/flow_table.c
> > +++ b/datapath/flow_table.c
> > -static void __table_instance_destroy(struct table_instance *ti)
> > +static void table_instance_free_flows(struct table_instance *ti, bool
> deferred)
>
> I believe this is only called from table_instance_destroy() - maybe we
> should just fold it in? It is somewhat of a discrete block of code but
> in this case I think consolidating functions helps readability since
> there are so many paths.
>
> > -skip_flows:
> > +static void __table_instance_destroy(struct table_instance *ti)
> > +{
> > free_buckets(ti->buckets);
> > kfree(ti);
> > }
> > @@ -262,7 +262,8 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head
> *rcu)
> > {
> > struct table_instance *ti = container_of(rcu, struct
> table_instance, rcu);
> >
> > - __table_instance_destroy(ti);
> > + free_buckets(ti->buckets);
> > + kfree(ti);
> > }
>
> Should we use the helper function here?
>
> This is definitely very intricate code. I think the basic principle
> here is that the only operation that should be performed in a deferred
> block is freeing memory - nothing more complex should take advantage
> of an existing RCU callback, which means that we have a series of
> callbacks in parallel instead of a chain. I think this principle is
> now respected everywhere.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
