On Wed, Jun 11, 2014 at 11:20:40AM -0700, Ben Pfaff wrote:
> On Sat, May 31, 2014 at 01:53:15AM -0300, Flavio Leitner wrote:
> > This patch adds generic IGMP snooping library code
> > that is used in follow-up patches.
> >
> > Signed-off-by: Cong Wang <amw...@redhat.com>
> > Signed-off-by: Daniel Borkmann <dbork...@redhat.com>
> > Acked-by: Thomas Graf <tg...@redhat.com>
> > Signed-off-by: Flavio Leitner <f...@redhat.com>
>
> Thanks for the revision!
>
> In mcast_snooping_prune_expired(), I think that we can "break" out of
> the loop after we reach a bundle that has not expired (the list is
> sorted on expiration time, right?).
That's right.
> MCAST_MROUTER_PORT_DEFAULT_IDLE_TIME is actually the fixed idle time
> that is always used, so I would consider removing "default" from its
> name.
One would like to change that in future, but since we don't have
that facility now, I agree with you.
> My remaining comments are stylistic. I think it's easiest to just
> suggest folding in the following incremental patch; let me know if
> some of them don't make sense.
I didn't like much the OVS_FORCE change, but indeed it's cheaper
than calling ntohl() and I don't have a better suggestion.
Ok, I will respin the series with your fixes included and post v4.
fbl
>
> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> index 0bf5195..8416753 100644
> --- a/lib/mcast-snooping.c
> +++ b/lib/mcast-snooping.c
> @@ -40,7 +40,8 @@ COVERAGE_DEFINE(mcast_snooping_expired);
>
> static struct mcast_mrouter_bundle *
> mcast_snooping_mrouter_lookup(struct mcast_snooping *ms, uint16_t vlan,
> - void *port);
> + void *port)
> + OVS_REQ_RDLOCK(ms->rwlock);
>
> bool
> mcast_snooping_enabled(const struct mcast_snooping *ms)
> @@ -57,10 +58,7 @@ mcast_snooping_flood_unreg(const struct mcast_snooping *ms)
> bool
> mcast_snooping_is_query(ovs_be16 igmp_type)
> {
> - if (igmp_type == htons(IGMP_HOST_MEMBERSHIP_QUERY)) {
> - return true;
> - }
> - return false;
> + return igmp_type == htons(IGMP_HOST_MEMBERSHIP_QUERY);
> }
>
> bool
> @@ -75,8 +73,8 @@ mcast_snooping_is_membership(ovs_be16 igmp_type)
> return false;
> }
>
> -/* Returns the number of seconds since the multicast group
> - * was learned in a port */
> +/* Returns the number of seconds since multicast group 'b' was learned in a
> + * port on 'ms'. */
> int
> mcast_bundle_age(const struct mcast_snooping *ms,
> const struct mcast_group_bundle *b)
> @@ -89,7 +87,7 @@ static uint32_t
> mcast_table_hash(const struct mcast_snooping *ms, ovs_be32 grp_ip4,
> uint16_t vlan)
> {
> - return hash_3words(ntohl(grp_ip4), vlan, ms->secret);
> + return hash_3words((OVS_FORCE uint32_t) grp_ip4, vlan, ms->secret);
> }
>
> static struct mcast_group_bundle *
> @@ -105,7 +103,7 @@ mcast_group_from_lru_node(struct list *list)
> }
>
> /* Searches 'ms' for and returns an mcast group for destination address
> - * 'dip' in 'vlan' */
> + * 'dip' in 'vlan'. */
> struct mcast_group *
> mcast_snooping_lookup(const struct mcast_snooping *ms, ovs_be32 dip,
> uint16_t vlan)
> @@ -148,7 +146,7 @@ normalize_idle_time(unsigned int idle_time)
> }
>
> /* Creates and returns a new mcast table with an initial mcast aging
> - * timeout of 'idle_time' seconds and an initial maximum of
> + * timeout of MCAST_ENTRY_DEFAULT_IDLE_TIME seconds and an initial maximum of
> * MCAST_DEFAULT_MAX entries. */
> struct mcast_snooping *
> mcast_snooping_create(void)
> @@ -198,8 +196,7 @@ mcast_snooping_unref(struct mcast_snooping *ms)
>
> /* Changes the mcast aging timeout of 'ms' to 'idle_time' seconds. */
> void
> -mcast_snooping_set_idle_time(struct mcast_snooping *ms,
> - unsigned int idle_time)
> +mcast_snooping_set_idle_time(struct mcast_snooping *ms, unsigned int
> idle_time)
> OVS_REQ_WRLOCK(ms->rwlock)
> {
> struct mcast_group *grp;
> @@ -233,8 +230,8 @@ mcast_snooping_set_max_entries(struct mcast_snooping *ms,
> /* Sets if unregistered multicast packets should be flooded to
> * all ports or only to ports connected to multicast routers
> *
> - * return true if previous state differs from current state
> - * return false otherwise. */
> + * Returns true if previous state differs from current state,
> + * false otherwise. */
> bool
> mcast_snooping_set_flood_unreg(struct mcast_snooping *ms, bool enable)
> OVS_REQ_WRLOCK(ms->rwlock)
> @@ -282,8 +279,8 @@ mcast_group_insert_bundle(struct mcast_snooping *ms
> OVS_UNUSED,
> return b;
> }
>
> -/* Return true if multicast still has bundles associated
> - * Return false if there is no bundles */
> +/* Return true if multicast still has bundles associated.
> + * Return false if there is no bundles. */
> static bool
> mcast_group_has_bundles(struct mcast_group *grp)
> {
> @@ -291,7 +288,7 @@ mcast_group_has_bundles(struct mcast_group *grp)
> }
>
> /* Delete 'grp' from the 'ms' hash table.
> - * Caller is responsible to clean bundle lru first */
> + * Caller is responsible to clean bundle lru first. */
> static void
> mcast_snooping_flush_group__(struct mcast_snooping *ms,
> struct mcast_group *grp)
> @@ -318,8 +315,8 @@ mcast_snooping_flush_group(struct mcast_snooping *ms,
> struct mcast_group *grp)
> }
>
>
> -/* Delete bundle returning true if it succeed
> - * false if it didn't find the group */
> +/* Delete bundle returning true if it succeeds,
> + * false if it didn't find the group. */
> static bool
> mcast_group_delete_bundle(struct mcast_snooping *ms OVS_UNUSED,
> struct mcast_group *grp, void *port)
> @@ -337,7 +334,8 @@ mcast_group_delete_bundle(struct mcast_snooping *ms
> OVS_UNUSED,
> return false;
> }
>
> -/* check if any bundle has expired and delete it */
> +/* If any bundle has expired, delete it. Returns the number of deleted
> + * bundles. */
> static int
> mcast_snooping_prune_expired(struct mcast_snooping *ms,
> struct mcast_group *grp)
> @@ -372,14 +370,14 @@ mcast_snooping_prune_expired(struct mcast_snooping *ms,
> * move to the last position in the LRU list.
> */
> bool
> -mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4, uint16_t
> vlan,
> - void *port)
> +mcast_snooping_add_group(struct mcast_snooping *ms, ovs_be32 ip4,
> + uint16_t vlan, void *port)
> OVS_REQ_WRLOCK(ms->rwlock)
> {
> bool learned;
> struct mcast_group *grp;
>
> - /* avoid duplicate packets */
> + /* Avoid duplicate packets. */
> if (mcast_snooping_mrouter_lookup(ms, vlan, port)
> || mcast_snooping_fport_lookup(ms, vlan, port)) {
> return false;
> @@ -407,6 +405,7 @@ mcast_snooping_add_group(struct mcast_snooping *ms,
> ovs_be32 ip4, uint16_t vlan,
> list_remove(&grp->group_node);
> }
> mcast_group_insert_bundle(ms, grp, port, ms->idle_time);
> +
> /* Mark 'grp' as recently used. */
> list_push_back(&ms->group_lru, &grp->group_node);
> return learned;
> @@ -428,10 +427,10 @@ mcast_snooping_leave_group(struct mcast_snooping *ms,
> ovs_be32 ip4,
> }
>
>
> -/* Router ports */
> +/* Router ports. */
>
> /* Returns the number of seconds since the multicast router
> - * was learned in a port */
> + * was learned in a port. */
> int
> mcast_mrouter_age(const struct mcast_snooping *ms OVS_UNUSED,
> const struct mcast_mrouter_bundle *mrouter)
> @@ -485,7 +484,7 @@ mcast_snooping_add_mrouter(struct mcast_snooping *ms,
> uint16_t vlan,
> {
> struct mcast_mrouter_bundle *mrouter;
>
> - /* avoid duplicate packets */
> + /* Avoid duplicate packets. */
> if (mcast_snooping_fport_lookup(ms, vlan, port)) {
> return false;
> }
> @@ -512,9 +511,8 @@ mcast_snooping_flush_mrouter(struct mcast_mrouter_bundle
> *mrouter)
> list_remove(&mrouter->mrouter_node);
> free(mrouter);
> }
> -
>
> -/* Flood ports */
> +/* Flood ports. */
>
> static struct mcast_fport_bundle *
> mcast_fport_from_list_node(struct list *list)
> @@ -582,16 +580,14 @@ mcast_snooping_set_port_flood(struct mcast_snooping
> *ms, uint16_t vlan,
> if (flood && !fport) {
> mcast_snooping_add_fport(ms, vlan, port);
> ms->need_revalidate = true;
> - }
> -
> - if (!flood && fport) {
> + } else if (!flood && fport) {
> mcast_snooping_flush_fport(fport);
> ms->need_revalidate = true;
> }
> }
> -
>
> -/* Run and Flush */
> +/* Run and flush. */
> +
> static void
> mcast_snooping_mdb_flush__(struct mcast_snooping *ms)
> OVS_REQ_WRLOCK(ms->rwlock)
> @@ -599,13 +595,13 @@ mcast_snooping_mdb_flush__(struct mcast_snooping *ms)
> struct mcast_group *grp;
> struct mcast_mrouter_bundle *mrouter;
>
> - while (group_get_lru(ms, &grp)){
> + while (group_get_lru(ms, &grp)) {
> mcast_snooping_flush_group(ms, grp);
> }
>
> hmap_shrink(&ms->table);
>
> - while (mrouter_get_lru(ms, &mrouter)){
> + while (mrouter_get_lru(ms, &mrouter)) {
> mcast_snooping_flush_mrouter(mrouter);
> }
> }
> @@ -622,7 +618,7 @@ mcast_snooping_mdb_flush(struct mcast_snooping *ms)
> ovs_rwlock_unlock(&ms->rwlock);
> }
>
> -/* flushes mdb and flood ports */
> +/* Flushes mdb and flood ports. */
> static void
> mcast_snooping_flush__(struct mcast_snooping *ms)
> OVS_REQ_WRLOCK(ms->rwlock)
> @@ -631,17 +627,17 @@ mcast_snooping_flush__(struct mcast_snooping *ms)
> struct mcast_mrouter_bundle *mrouter;
> struct mcast_fport_bundle *fport;
>
> - while (group_get_lru(ms, &grp)){
> + while (group_get_lru(ms, &grp)) {
> mcast_snooping_flush_group(ms, grp);
> }
>
> hmap_shrink(&ms->table);
>
> - while (mrouter_get_lru(ms, &mrouter)){
> + while (mrouter_get_lru(ms, &mrouter)) {
> mcast_snooping_flush_mrouter(mrouter);
> }
>
> - while (fport_get(ms, &fport)){
> + while (fport_get(ms, &fport)) {
> mcast_snooping_flush_fport(fport);
> }
> }
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
