On 11 Jun 2026, at 6:58, Adrian Moreno wrote:
> This patch turns "flow_table" from being embedded into "datapath" to
> being an rcu protected pointer. No functional change intended.
>
> Signed-off-by: Adrian Moreno <[email protected]>
Hi Adrian,
Thanks for the series. I need some time to take a look, as this is
the first time I reviewed this series. I'll probably reply to the
other patches early next week.
Some small comments below.
Cheers,
Eelco
[...]
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
[...]
> @@ -752,12 +759,16 @@ static struct genl_family dp_packet_genl_family
> __ro_after_init = {
> static void get_dp_stats(const struct datapath *dp, struct ovs_dp_stats
> *stats,
> struct ovs_dp_megaflow_stats *mega_stats)
> {
> + struct flow_table *table = ovsl_dereference(dp->table);
> int i;
>
> memset(mega_stats, 0, sizeof(*mega_stats));
> + memset(stats, 0, sizeof(*stats));
>
> - stats->n_flows = ovs_flow_tbl_count(&dp->table);
> - mega_stats->n_masks = ovs_flow_tbl_num_masks(&dp->table);
> + if (table) {
> + stats->n_flows = ovs_flow_tbl_count(table);
> + mega_stats->n_masks = ovs_flow_tbl_num_masks(table);
> + }
The memset above seems like an overkill, why not do this:
if (table) {
stats->n_flows = ovs_flow_tbl_count(table);
mega_stats->n_masks = ovs_flow_tbl_num_masks(table);
} else {
stats->n_flows = 0;
}
>
> stats->n_hit = stats->n_missed = stats->n_lost = 0;
[...]
> @@ -1598,8 +1639,13 @@ static int ovs_dp_cmd_fill_info(struct datapath *dp,
> struct sk_buff *skb,
> struct ovs_dp_stats dp_stats;
> struct ovs_dp_megaflow_stats dp_megaflow_stats;
> struct dp_nlsk_pids *pids = ovsl_dereference(dp->upcall_portids);
> + struct flow_table *table;
> int err, pids_len;
>
> + table = ovsl_dereference(dp->table);
> + if (!table)
> + return -ENODEV;
ovs_dp_cmd_fill_info() can now return -ENODEV when table is
NULL, but every caller immediately follows with:
BUG_ON(err < 0);
so this would panic the kernel. I guess you fix this in the 3rd patch,
but just mentioning this as patch 1 is unsafe to apply in isolation.
> ovs_header = genlmsg_put(skb, portid, seq, &dp_datapath_genl_family,
> flags, cmd);
> if (!ovs_header)
[...]
> @@ -1917,7 +1972,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct
> genl_info *info)
> /* Called with ovs_mutex. */
> static void __dp_destroy(struct datapath *dp)
> {
> - struct flow_table *table = &dp->table;
> + struct flow_table *table = ovsl_dereference(dp->table);
> int i;
>
> if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
> @@ -1948,6 +2003,7 @@ static void __dp_destroy(struct datapath *dp)
>
> /* RCU destroy the ports, meters and flow tables. */
> call_rcu(&dp->rcu, destroy_dp_rcu);
In theory the table pointer cannot be NULL here, but would
it make sense to add a WARN_ON(!table) to catch any future
mistakes?
> + call_rcu(&table->rcu, ovs_flow_tbl_destroy_rcu);
> }
>
[...]
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index 67d5b8c0fe79..3b7518e3394d 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -406,15 +406,19 @@ int ovs_flow_tbl_masks_cache_resize(struct flow_table
> *table, u32 size)
> return 0;
> }
>
> -int ovs_flow_tbl_init(struct flow_table *table)
> +struct flow_table *ovs_flow_tbl_alloc(void)
> {
> struct table_instance *ti, *ufid_ti;
> + struct flow_table *table;
> struct mask_cache *mc;
> struct mask_array *ma;
>
> + table = kzalloc_obj(*table, GFP_KERNEL);
> + if (!table)
> + return ERR_PTR(-ENOMEM);
Add an extra blank line?
> mc = tbl_mask_cache_alloc(MC_DEFAULT_HASH_ENTRIES);
> if (!mc)
> - return -ENOMEM;
> + goto free_table;
>
> ma = tbl_mask_array_alloc(MASK_ARRAY_SIZE_MIN);
> if (!ma)
> @@ -435,7 +439,7 @@ int ovs_flow_tbl_init(struct flow_table *table)
> table->last_rehash = jiffies;
> table->count = 0;
> table->ufid_count = 0;
> - return 0;
> + return table;
>
> free_ti:
> __table_instance_destroy(ti);
> @@ -443,7 +447,9 @@ int ovs_flow_tbl_init(struct flow_table *table)
> __mask_array_destroy(ma);
> free_mask_cache:
> __mask_cache_destroy(mc);
> - return -ENOMEM;
> +free_table:
> + kfree(table);
> + return ERR_PTR(-ENOMEM);
> }
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
