On Tue, Jun 20, 2023 at 09:55:31AM +0300, Nikolay Aleksandrov wrote:
> On 6/19/23 10:14, Johannes Nixdorf wrote:
> > +/* Set a FDB flag that implies the entry was not learned, and account
> > + * for changes in the learned status.
> > + */
> > +static void __fdb_set_flag_not_learned(struct net_bridge *br,
> > + struct net_bridge_fdb_entry *fdb,
> > + long nr)
> > +{
> > + WARN_ON_ONCE(!(BIT(nr) & BR_FDB_NOT_LEARNED_MASK));
>
> Please use *_bit
Can you tell me which *_bit helper you had in mind? The shortest option I could
come up with the ones I found seemed needlessly verbose and wasteful:
static const unsigned long br_fdb_not_learned_mask = BR_FDB_NOT_LEARNED_MASK;
...
WARN_ON_ONCE(test_bit(nr, &br_fdb_not_learned_mask));
> > +
> > + /* learned before, but we set a flag that implies it's manually added */
> > + if (!(fdb->flags & BR_FDB_NOT_LEARNED_MASK))
>
> Please use *_bit
This will be fixed by the redesign to get rid of my use of hash_lock
(proposed later in this mail), as I'll only have to test one bit and can
use test_and_clear_bit then.
> > + br->fdb_cur_learned_entries--;
> > + set_bit(nr, &fdb->flags);
> > +}
>
> Having a helper that conditionally decrements only is counterintuitive and
> people can get confused. Either add 2 helpers for inc/dec and use
> them where appropriate or don't use helpers at all.
The *_set_bit helper can only cause the count to drop, as there
is currently no flag that could turn a manually added entry back into
a dynamically learned one.
The analogous helper that increments the value would be *_clear_bit,
which I did not add because it has no users.
> > + spin_unlock_bh(&br->hash_lock);
> > +}
> > +
> > /* When a static FDB entry is deleted, the HW address from that entry is
> > * also removed from the bridge private HW address list and updates all
> > * the ports with needed information.
> > @@ -321,6 +353,8 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr)
> > static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
> > bool swdev_notify)
> > {
> > + bool learned = !(f->flags & BR_FDB_NOT_LEARNED_MASK);
>
> *_bit
I do not know a *_bit helper that would help me test the intersection
of multiple bits on both sides. Do you have any in mind?
> > +
> > return fdb;
> > }
> > @@ -894,7 +940,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
> > }
> > if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
> > - set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
> > + fdb_set_flag_not_learned(br, fdb, BR_FDB_ADDED_BY_USER);
>
> Unacceptable to take hash_lock and block all learning here, eventual
> consistency is ok or some other method that is much lighter and doesn't
> block all learning or requires a lock.
At the time of writing v2, this seemed difficult because we want to test
multiple bits and increment a counter, but remembering that clear_bit
is never called for the bits I care about I came up with the following
approach:
a) Add a new flag BR_FDB_DYNAMIC_LEARNED, which is set to 1 iff
BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL are set in br_create.
Every time BR_FDB_ADDED_BY_USER or BR_FDB_LOCAL is set, also clear
BR_FDB_DYNAMIC_LEARNED, and decrement the count if it was 1 before.
This solves the problem of testing two bits at once, and would not
have been possible if we had a code path that could clear both bits,
as it is not as easy to decide when to set BR_FDB_DYNAMIC_LEARNED
again in that case.
b) Replace the current count with an atomic_t.
I'll change it this way for v3.
> > return -EMSGSIZE;
> > #ifdef CONFIG_BRIDGE_VLAN_FILTERING
> > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> > index 2119729ded2b..df079191479e 100644
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -275,6 +275,8 @@ enum {
> > BR_FDB_LOCKED,
> > };
> > +#define BR_FDB_NOT_LEARNED_MASK (BIT(BR_FDB_LOCAL) | BIT(BR_FDB_ADDED_BY_USER))
>
> Not learned sounds confusing and doesn't accurately describe the entry.
> BR_FDB_DYNAMIC_LEARNED perhaps or some other name, that doesn't cause
> double negatives (not not learned).
Your proposal would not have captured the mask, as it describes all the
opposite cases, which were _not_ dynamically learned.
But with the proposed new flag from the hash_lock comment we can trivially
flip the meaning, so I went with your proposed name there.
