On Tue, Dec 22, 2020 at 9:11 PM Dumitru Ceara <[email protected]> wrote:
>
> When a user flushes the IGMP Groups ("ovn-sbctl ip-multicast-flush")
> there's no guarantee that the ovn-controller main thread runs soon after
> the pinctrl thread has flushed the in-memory IGMP groups, in
> ip_mcast_snoop_flush().
>
> To avoid unnecessarily waking the main thread just clear the local
> IGMP_Group records in the main thread.
>
> This scenario is quite hard to encounter in real deployments but every
> now and then we hit it in CI.
>
> Fixes: 70ff8243040f ("OVN: Add IGMP SB definitions and ovn-controller
> support")
> Signed-off-by: Dumitru Ceara <[email protected]>
Thanks for the fix. I applied this patch to master and back ported to
all stable branches - 20.12 to 20.03
Thanks
Numan
> ---
> controller/pinctrl.c | 37 +++++++++++++++++++++++++++++++++++--
> 1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index 7e3abf0..e957b75 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -4507,9 +4507,16 @@ ip_mcast_snoop_state_find(int64_t dp_key)
> return NULL;
> }
>
> +/* Updates the ip_mcast_snoop_cfg for a logical datapath specified by
> + * 'dp_key'. Also sets 'needs_flush' to 'true' if the config change should
> + * to trigger flushing of the existing IGMP_Groups.
> + *
> + * Returns 'true' if any changes happened to the configuration.
> + */
> static bool
> ip_mcast_snoop_state_update(int64_t dp_key,
> - const struct ip_mcast_snoop_cfg *cfg)
> + const struct ip_mcast_snoop_cfg *cfg,
> + bool *needs_flush)
> OVS_REQUIRES(pinctrl_mutex)
> {
> bool notify = false;
> @@ -4519,6 +4526,9 @@ ip_mcast_snoop_state_update(int64_t dp_key,
> ms_state = ip_mcast_snoop_state_add(dp_key);
> notify = true;
> } else if (memcmp(cfg, &ms_state->cfg, sizeof *cfg)) {
> + if (ms_state->cfg.seq_no != cfg->seq_no) {
> + *needs_flush = true;
> + }
> notify = true;
> }
>
> @@ -4738,6 +4748,25 @@ ip_mcast_snoop_run(void)
> }
> }
>
> +/* Flushes all IGMP_Groups installed by the local chassis for the logical
> + * datapath specified by 'dp_key'.
> + */
> +static void
> +ip_mcast_flush_groups(int64_t dp_key, const struct sbrec_chassis *chassis,
> + struct ovsdb_idl_index *sbrec_igmp_groups)
> +{
> + const struct sbrec_igmp_group *sbrec_igmp;
> +
> + SBREC_IGMP_GROUP_FOR_EACH_BYINDEX (sbrec_igmp, sbrec_igmp_groups) {
> + if (!sbrec_igmp->datapath ||
> + sbrec_igmp->datapath->tunnel_key != dp_key ||
> + sbrec_igmp->chassis != chassis) {
> + continue;
> + }
> + igmp_group_delete(sbrec_igmp);
> + }
> +}
> +
> /*
> * This runs in the pinctrl main thread, so it has access to the southbound
> * database. It reads the IP_Multicast table and updates the local multicast
> @@ -4770,11 +4799,15 @@ ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
>
> int64_t dp_key = ip_mcast->datapath->tunnel_key;
> struct ip_mcast_snoop_cfg cfg;
> + bool flush_groups = false;
>
> ip_mcast_snoop_cfg_load(&cfg, ip_mcast);
> - if (ip_mcast_snoop_state_update(dp_key, &cfg)) {
> + if (ip_mcast_snoop_state_update(dp_key, &cfg, &flush_groups)) {
> notify = true;
> }
> + if (flush_groups) {
> + ip_mcast_flush_groups(dp_key, chassis, sbrec_igmp_groups);
> + }
> }
>
> /* Then delete the old entries. */
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
