On Fri, Dec 2, 2011 at 11:00 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index acbd3bf..ae6b39a 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -63,6 +63,10 @@
> #error Kernels before 2.6.18 or after 3.2 are not supported by this version of Open vSwitch.
> #endif
>
> +#define REBUILD_INTERVAL (HZ * 10 * 60)
I think we should give a little more context for this and call it
something like FLOW_REBUILD_INTERVAL.
> +static struct timer_list ovs_table_rebuild_timer;
I won't bother commenting on the timer/locking stuff since we decided
to change it.
When doing the switch over between the old table and new table we
can't destroy the old table before assigning the new one since if
there is no current read lock holder it's possible for the table to
get destroyed immediately and then a new reader picks up the old value
before the new one is assigned.
> static void dp_cleanup(void)
> {
> rcu_barrier();
> + del_timer(&ovs_table_rebuild_timer);
This needs to be del_timer_sync (or the workqueue variant) since the
timer could currently be running on a different CPU when we unload the
module.
> diff --git a/datapath/flow.c b/datapath/flow.c
> index a357077..9e64458 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -338,6 +337,9 @@ struct flow_table *ovs_flow_tbl_alloc(int new_size)
> }
> table->n_buckets = new_size;
> table->count = 0;
> + table->node_ver = 0;
> + table->keep_flows = 0;
Since keep_flows is actually a bool, it's nicer to use true and false.
> -struct flow_table *ovs_flow_tbl_expand(struct flow_table *table)
> +static void flow_table_copy_flows(struct flow_table *old, struct flow_table *new)
> {
[...]
> + hlist_for_each_entry(flow, n, head, hash_node[old_ver]) {
> + ovs_flow_tbl_insert(new, flow);
> }
We don't need the braces here.
> +struct flow_table *ovs_flow_tbl_expand(struct flow_table *table)
> +{
> + return __flow_tbl_rehash(table, (table->n_buckets * 2));
The parentheses aren't needed here either.
> void ovs_flow_tbl_remove(struct flow_table *table, struct sw_flow *flow)
> {
> - if (!hlist_unhashed(&flow->hash_node)) {
> - hlist_del_init_rcu(&flow->hash_node);
> + if (!hlist_unhashed(&flow->hash_node[table->node_ver])) {
> + hlist_del_init_rcu(&flow->hash_node[table->node_ver]);
I think both of these lines to check whether a flow is currently
inserted in a table and then to reinitialize it aren't really
necessary because the concept of initialized depends on which table
version you are looking at (and we never try to reinsert anyways).
> diff --git a/datapath/flow.h b/datapath/flow.h
> index 36e738d..16e3310 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> +struct flow_table *ovs_flow_tbl_rehash(struct flow_table *table);
I would probably group this with ovs_flow_tbl_expand().
> +void flow_table_clear_flows(struct flow_table *old);
Global symbol need to be prefixed with "ovs_" but it looks like this
function doesn't actually exist anyways.