On 7/6/25 2:46 PM, Roi Dayan wrote:
> From: Eli Britstein <el...@nvidia.com>
>
> Upon modifying a group, the following steps occur:
> 1. ofproto_group_mod_start():
> Find an old group object, create a new one.
> 2. ofproto_bump_tables_version()
> 3. ofproto_group_mod_finish():
> Modify the new group object with buckets etc.
>
> At step #3, the new group object is already in use by revalidators,
> that may read incorrect data while being modified.
>
> Instead, move the group modification of the new object to step #1.
>
> Fixes: 0a8f6beb54ab ("ofproto-dpif: Fix dp_hash mapping after select group
> modification.")
> Signed-off-by: Eli Britstein <el...@nvidia.com>
> Acked-by: Gaetan Rivet <gaet...@nvidia.com>
> Acked-by: Roi Dayan <r...@nvidia.com>
> ---
> ofproto/ofproto.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ef615e59c354..323e38a53d6c 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -7939,6 +7939,16 @@ ofproto_group_mod_start(struct ofproto *ofproto,
> struct ofproto_group_mod *ogm)
> error = OFPERR_OFPGMFC_BAD_COMMAND;
> break;
> }
> +
> + if (!error && ogm->new_group && group_collection_n(&ogm->old_groups) &&
> + ofproto->ofproto_class->group_modify) {
> + /* Modify a group. */
> + ovs_assert(group_collection_n(&ogm->old_groups) == 1);
> +
> + /* XXX: OK to lose old group's stats? */
> + ofproto->ofproto_class->group_modify(ogm->new_group);
> + }
> +
I'd suggest to move this closer to the code that modifies the group, so it's
more clear why this is needed. i.e. move this into the modify_group_start()
after we finish changing the buckets. This will also allow dropping most of
the checks from the if condition. The assertion can also be dropped or moved
right after the group_collection_add().
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
