[ovs-dev] [bridge 10/15] mac-learning: Refactor to increase generality.

[Ben Pfaff]

On Mon, Mar 21, 2011 at 03:42:15PM -0700, Ethan Jackson wrote:
> Thanks for writing this up.  Looks like it will be nifty in the future.
> 
> > + * If the returned MAC entry is new (as may be determined by calling
> > + * mac_entry_is_new()), then the caller must pass the new entry to
> > + * mac_learning_touch() and mac_learning_change(), in that order. ?The caller
> > + * must also initialize the new entry's 'port' member. ?Otherwise calling those
> > + * functions is at the caller's discretion. */
> > +struct mac_entry *
> > +mac_learning_insert(struct mac_learning *ml,
> > + ? ? ? ? ? ? ? ? ? ?const uint8_t src_mac[ETH_ADDR_LEN], uint16_t vlan)
> > ?{
> 
> It may make sense to automatically call mac_learning_touch and
> mac_learning_change for the caller if the returned mac_entry is new.
> This may reduce the risk of errors.  I think there is a slight risk of
> either of these functions being called twice on new mac entries, but
> that doesn't seem like a huge problem to me.

I agree that that would be a better interface.

Looking more carefully, I don't see a problem with always
unconditionally doing mac_learning_touch().  Before, I was concerned
about behaving exactly the same way as the previous version, which did
not do the mac_learning_touch() equivalent for locked ARP entries, but
I can't see how it would make a real different in practice.  So, I've
integrated that into mac_learning_insert().

That's the one that's really important.  If mac_learning_change()
doesn't get called, it's really not a big deal for the mac_learning
module itself.

> If we can't do this, it may make sense alternatively to have
> mac_learning_lookup assert each returned mac_entries tag and expires
> members to ensure that they were properly initialized.

It's a good idea anyway, so I added that too.

> > +mac_learning_change(struct mac_learning *ml, struct mac_entry *e)
> > +{
> > + ? ?tag_type old_tag = e->tag;
> > +
> > + ? ?COVERAGE_INC(mac_learning_learned);
> 
> It may make sense to rename this mac_learning_changed.

I was on the fence about that already, so I made the change.

> 
> > + ? ?mac = mac_learning_insert(br->ml, flow->dl_src, vlan);
> > + ? ?if (is_gratuitous_arp(flow)) {
> > + ? ? ? ?/* We don't want to learn from gratuitous ARP packets that are
> > + ? ? ? ? * reflected back over bond slaves so we lock the learning table. */
> > + ? ? ? ?if (in_port->n_ifaces == 1) {
> > + ? ? ? ? ? ?mac_entry_set_grat_arp_lock(mac);
> > + ? ? ? ?} else if (mac_entry_is_grat_arp_locked(mac)) {
> > + ? ? ? ? ? ?return;
> 
> If mac is new here it will never be touched or changed.  I'm not sure
> if that's a problem as it may get touched next time
> update_learning_table is called, but it seems like we may end up with
> entries with bogus lru status.

A new MAC won't be locked so the return here will never be reached.