| Message ID | 1608651697-25226-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pinctrl: Fix race condition when explicitly clearing IGMP groups. | expand |
On Tue, Dec 22, 2020 at 9:11 PM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 1/7/21 9:55 AM, Numan Siddique wrote: > On Tue, Dec 22, 2020 at 9:11 PM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> > > Thanks for the fix. I applied this patch to master and back ported to > all stable branches - 20.12 to 20.03 > > Thanks > Numan > Thanks!
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. */
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 <dceara@redhat.com> --- controller/pinctrl.c | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)