Re: [PATCH net-next v4 1/4] net: openvswitch: make flow_table an rcu pointer

[Eelco Chaudron <echaudro@redhat.com>]

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 <amorenoz@redhat.com>

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);
>  }

[...]