* [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Hi,
We'd like to have a well-defined behaviour when changing fdb flags. The
problem is that we've added new fields which are changed from all
contexts without any locking. We are aware of the bit test/change races
and these are fine (we can remove them later), but it is considered
undefined behaviour to change bitfields from multiple threads and also
on some architectures that can result in unexpected results,
specifically when all fields between the changed ones are also
bitfields. The conversion to bitops shows the intent clearly and
makes them use functions with well-defined behaviour in such cases.
There is no overhead for the fast-path, the bit changing functions are
used only in special cases when learning and in the slow path.
In addition this conversion allows us to simplify fdb flag handling and
avoid bugs for future bits (e.g. a forgetting to clear the new bit when
allocating a new fdb). All bridge selftests passed, also tried all of the
converted bits manually in a VM.
Thanks,
Nik
Nikolay Aleksandrov (7):
net: bridge: fdb: convert is_local to bitops
net: bridge: fdb: convert is_static to bitops
net: bridge: fdb: convert is_sticky to bitops
net: bridge: fdb: convert added_by_user to bitops
net: bridge: fdb: convert added_by_external_learn to use bitops
net: bridge: fdb: convert offloaded to use bitops
net: bridge: fdb: set flags directly in fdb_create
net/bridge/br_fdb.c | 133 +++++++++++++++++++-------------------
net/bridge/br_input.c | 2 +-
net/bridge/br_private.h | 17 +++--
net/bridge/br_switchdev.c | 12 ++--
4 files changed, 85 insertions(+), 79 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Hi,
We'd like to have a well-defined behaviour when changing fdb flags. The
problem is that we've added new fields which are changed from all
contexts without any locking. We are aware of the bit test/change races
and these are fine (we can remove them later), but it is considered
undefined behaviour to change bitfields from multiple threads and also
on some architectures that can result in unexpected results,
specifically when all fields between the changed ones are also
bitfields. The conversion to bitops shows the intent clearly and
makes them use functions with well-defined behaviour in such cases.
There is no overhead for the fast-path, the bit changing functions are
used only in special cases when learning and in the slow path.
In addition this conversion allows us to simplify fdb flag handling and
avoid bugs for future bits (e.g. a forgetting to clear the new bit when
allocating a new fdb). All bridge selftests passed, also tried all of the
converted bits manually in a VM.
Thanks,
Nik
Nikolay Aleksandrov (7):
net: bridge: fdb: convert is_local to bitops
net: bridge: fdb: convert is_static to bitops
net: bridge: fdb: convert is_sticky to bitops
net: bridge: fdb: convert added_by_user to bitops
net: bridge: fdb: convert added_by_external_learn to use bitops
net: bridge: fdb: convert offloaded to use bitops
net: bridge: fdb: set flags directly in fdb_create
net/bridge/br_fdb.c | 133 +++++++++++++++++++-------------------
net/bridge/br_input.c | 2 +-
net/bridge/br_private.h | 17 +++--
net/bridge/br_switchdev.c | 12 ++--
4 files changed, 85 insertions(+), 79 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 1/7] net: bridge: fdb: convert is_local to bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
The patch adds a new fdb flags field in the hole between the two cache
lines and uses it to convert is_local to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 32 +++++++++++++++++++-------------
net/bridge/br_input.c | 2 +-
net/bridge/br_private.h | 9 +++++++--
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b1d3248c0252..e67d5eb8bc1d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -250,7 +250,8 @@ void br_fdb_find_delete_local(struct net_bridge *br,
spin_lock_bh(&br->hash_lock);
f = br_fdb_find(br, addr, vid);
- if (f && f->is_local && !f->added_by_user && f->dst == p)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->added_by_user && f->dst == p)
fdb_delete_local(br, p, f);
spin_unlock_bh(&br->hash_lock);
}
@@ -265,7 +266,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
spin_lock_bh(&br->hash_lock);
vg = nbp_vlan_group(p);
hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
- if (f->dst == p && f->is_local && !f->added_by_user) {
+ if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->added_by_user) {
/* delete old one */
fdb_delete_local(br, p, f);
@@ -306,7 +308,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
/* If old entry was unassociated with any port, then delete it. */
f = br_fdb_find(br, br->dev->dev_addr, 0);
- if (f && f->is_local && !f->dst && !f->added_by_user)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->dst && !f->added_by_user)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, 0);
@@ -321,7 +324,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
if (!br_vlan_should_use(v))
continue;
f = br_fdb_find(br, br->dev->dev_addr, v->vid);
- if (f && f->is_local && !f->dst && !f->added_by_user)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->dst && !f->added_by_user)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, v->vid);
}
@@ -400,7 +404,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
if (f->is_static || (vid && f->key.vlan_id != vid))
continue;
- if (f->is_local)
+ if (test_bit(BR_FDB_LOCAL, &f->flags))
fdb_delete_local(br, p, f);
else
fdb_delete(br, f, true);
@@ -469,7 +473,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
fe->port_no = f->dst->port_no;
fe->port_hi = f->dst->port_no >> 8;
- fe->is_local = f->is_local;
+ fe->is_local = test_bit(BR_FDB_LOCAL, &f->flags);
if (!f->is_static)
fe->ageing_timer_value = jiffies_delta_to_clock_t(jiffies - f->updated);
++fe;
@@ -494,7 +498,9 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
fdb->dst = source;
fdb->key.vlan_id = vid;
- fdb->is_local = is_local;
+ fdb->flags = 0;
+ if (is_local)
+ set_bit(BR_FDB_LOCAL, &fdb->flags);
fdb->is_static = is_static;
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
@@ -526,7 +532,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
/* it is okay to have multiple ports with same
* address, just use the first one.
*/
- if (fdb->is_local)
+ if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return 0;
br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
source ? source->dev->name : br->dev->name, addr, vid);
@@ -572,7 +578,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb = fdb_find_rcu(&br->fdb_hash_tbl, addr, vid);
if (likely(fdb)) {
/* attempt to update an entry for a local interface */
- if (unlikely(fdb->is_local)) {
+ if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) {
if (net_ratelimit())
br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n",
source->dev->name, addr, vid);
@@ -616,7 +622,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
static int fdb_to_nud(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
- if (fdb->is_local)
+ if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return NUD_PERMANENT;
else if (fdb->is_static)
return NUD_NOARP;
@@ -840,19 +846,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (fdb_to_nud(br, fdb) != state) {
if (state & NUD_PERMANENT) {
- fdb->is_local = 1;
+ set_bit(BR_FDB_LOCAL, &fdb->flags);
if (!fdb->is_static) {
fdb->is_static = 1;
fdb_add_hw_addr(br, addr);
}
} else if (state & NUD_NOARP) {
- fdb->is_local = 0;
+ clear_bit(BR_FDB_LOCAL, &fdb->flags);
if (!fdb->is_static) {
fdb->is_static = 1;
fdb_add_hw_addr(br, addr);
}
} else {
- fdb->is_local = 0;
+ clear_bit(BR_FDB_LOCAL, &fdb->flags);
if (fdb->is_static) {
fdb->is_static = 0;
fdb_del_hw_addr(br, addr);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 09b1dd8cd853..7f5f646dba6e 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -151,7 +151,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
if (dst) {
unsigned long now = jiffies;
- if (dst->is_local)
+ if (test_bit(BR_FDB_LOCAL, &dst->flags))
return br_pass_frame_up(skb);
if (now != dst->used)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ce2ab14ee605..888cbe9c639a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -172,6 +172,11 @@ struct net_bridge_vlan_group {
u16 pvid;
};
+/* bridge fdb flags */
+enum {
+ BR_FDB_LOCAL,
+};
+
struct net_bridge_fdb_key {
mac_addr addr;
u16 vlan_id;
@@ -183,8 +188,8 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
- unsigned char is_local:1,
- is_static:1,
+ unsigned long flags;
+ unsigned char is_static:1,
is_sticky:1,
added_by_user:1,
added_by_external_learn:1,
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 1/7] net: bridge: fdb: convert is_local to bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
The patch adds a new fdb flags field in the hole between the two cache
lines and uses it to convert is_local to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 32 +++++++++++++++++++-------------
net/bridge/br_input.c | 2 +-
net/bridge/br_private.h | 9 +++++++--
3 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b1d3248c0252..e67d5eb8bc1d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -250,7 +250,8 @@ void br_fdb_find_delete_local(struct net_bridge *br,
spin_lock_bh(&br->hash_lock);
f = br_fdb_find(br, addr, vid);
- if (f && f->is_local && !f->added_by_user && f->dst == p)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->added_by_user && f->dst == p)
fdb_delete_local(br, p, f);
spin_unlock_bh(&br->hash_lock);
}
@@ -265,7 +266,8 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
spin_lock_bh(&br->hash_lock);
vg = nbp_vlan_group(p);
hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
- if (f->dst == p && f->is_local && !f->added_by_user) {
+ if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->added_by_user) {
/* delete old one */
fdb_delete_local(br, p, f);
@@ -306,7 +308,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
/* If old entry was unassociated with any port, then delete it. */
f = br_fdb_find(br, br->dev->dev_addr, 0);
- if (f && f->is_local && !f->dst && !f->added_by_user)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->dst && !f->added_by_user)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, 0);
@@ -321,7 +324,8 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
if (!br_vlan_should_use(v))
continue;
f = br_fdb_find(br, br->dev->dev_addr, v->vid);
- if (f && f->is_local && !f->dst && !f->added_by_user)
+ if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
+ !f->dst && !f->added_by_user)
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, v->vid);
}
@@ -400,7 +404,7 @@ void br_fdb_delete_by_port(struct net_bridge *br,
if (f->is_static || (vid && f->key.vlan_id != vid))
continue;
- if (f->is_local)
+ if (test_bit(BR_FDB_LOCAL, &f->flags))
fdb_delete_local(br, p, f);
else
fdb_delete(br, f, true);
@@ -469,7 +473,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
fe->port_no = f->dst->port_no;
fe->port_hi = f->dst->port_no >> 8;
- fe->is_local = f->is_local;
+ fe->is_local = test_bit(BR_FDB_LOCAL, &f->flags);
if (!f->is_static)
fe->ageing_timer_value = jiffies_delta_to_clock_t(jiffies - f->updated);
++fe;
@@ -494,7 +498,9 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
fdb->dst = source;
fdb->key.vlan_id = vid;
- fdb->is_local = is_local;
+ fdb->flags = 0;
+ if (is_local)
+ set_bit(BR_FDB_LOCAL, &fdb->flags);
fdb->is_static = is_static;
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
@@ -526,7 +532,7 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
/* it is okay to have multiple ports with same
* address, just use the first one.
*/
- if (fdb->is_local)
+ if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return 0;
br_warn(br, "adding interface %s with same address as a received packet (addr:%pM, vlan:%u)\n",
source ? source->dev->name : br->dev->name, addr, vid);
@@ -572,7 +578,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb = fdb_find_rcu(&br->fdb_hash_tbl, addr, vid);
if (likely(fdb)) {
/* attempt to update an entry for a local interface */
- if (unlikely(fdb->is_local)) {
+ if (unlikely(test_bit(BR_FDB_LOCAL, &fdb->flags))) {
if (net_ratelimit())
br_warn(br, "received packet on %s with own address as source address (addr:%pM, vlan:%u)\n",
source->dev->name, addr, vid);
@@ -616,7 +622,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
static int fdb_to_nud(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
- if (fdb->is_local)
+ if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return NUD_PERMANENT;
else if (fdb->is_static)
return NUD_NOARP;
@@ -840,19 +846,19 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (fdb_to_nud(br, fdb) != state) {
if (state & NUD_PERMANENT) {
- fdb->is_local = 1;
+ set_bit(BR_FDB_LOCAL, &fdb->flags);
if (!fdb->is_static) {
fdb->is_static = 1;
fdb_add_hw_addr(br, addr);
}
} else if (state & NUD_NOARP) {
- fdb->is_local = 0;
+ clear_bit(BR_FDB_LOCAL, &fdb->flags);
if (!fdb->is_static) {
fdb->is_static = 1;
fdb_add_hw_addr(br, addr);
}
} else {
- fdb->is_local = 0;
+ clear_bit(BR_FDB_LOCAL, &fdb->flags);
if (fdb->is_static) {
fdb->is_static = 0;
fdb_del_hw_addr(br, addr);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 09b1dd8cd853..7f5f646dba6e 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -151,7 +151,7 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
if (dst) {
unsigned long now = jiffies;
- if (dst->is_local)
+ if (test_bit(BR_FDB_LOCAL, &dst->flags))
return br_pass_frame_up(skb);
if (now != dst->used)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ce2ab14ee605..888cbe9c639a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -172,6 +172,11 @@ struct net_bridge_vlan_group {
u16 pvid;
};
+/* bridge fdb flags */
+enum {
+ BR_FDB_LOCAL,
+};
+
struct net_bridge_fdb_key {
mac_addr addr;
u16 vlan_id;
@@ -183,8 +188,8 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
- unsigned char is_local:1,
- is_static:1,
+ unsigned long flags;
+ unsigned char is_static:1,
is_sticky:1,
added_by_user:1,
added_by_external_learn:1,
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 2/7] net: bridge: fdb: convert is_static to bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Convert the is_static to bitops, make use of the combined
test_and_set/clear_bit to simplify expressions in fdb_add_entry.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 40 +++++++++++++++++++---------------------
net/bridge/br_private.h | 4 ++--
2 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e67d5eb8bc1d..1c890e2d694b 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -75,8 +75,9 @@ static inline unsigned long hold_time(const struct net_bridge *br)
static inline int has_expired(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
- return !fdb->is_static && !fdb->added_by_external_learn &&
- time_before_eq(fdb->updated + hold_time(br), jiffies);
+ return !test_bit(BR_FDB_STATIC, &fdb->flags) &&
+ !fdb->added_by_external_learn &&
+ time_before_eq(fdb->updated + hold_time(br), jiffies);
}
static void fdb_rcu_free(struct rcu_head *head)
@@ -197,7 +198,7 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
{
trace_fdb_delete(br, f);
- if (f->is_static)
+ if (test_bit(BR_FDB_STATIC, &f->flags))
fdb_del_hw_addr(br, f->key.addr.addr);
hlist_del_init_rcu(&f->fdb_node);
@@ -350,7 +351,8 @@ void br_fdb_cleanup(struct work_struct *work)
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
unsigned long this_timer;
- if (f->is_static || f->added_by_external_learn)
+ if (test_bit(BR_FDB_STATIC, &f->flags) ||
+ f->added_by_external_learn)
continue;
this_timer = f->updated + delay;
if (time_after(this_timer, now)) {
@@ -377,7 +379,7 @@ void br_fdb_flush(struct net_bridge *br)
spin_lock_bh(&br->hash_lock);
hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
fdb_delete(br, f, true);
}
spin_unlock_bh(&br->hash_lock);
@@ -401,7 +403,8 @@ void br_fdb_delete_by_port(struct net_bridge *br,
continue;
if (!do_all)
- if (f->is_static || (vid && f->key.vlan_id != vid))
+ if (test_bit(BR_FDB_STATIC, &f->flags) ||
+ (vid && f->key.vlan_id != vid))
continue;
if (test_bit(BR_FDB_LOCAL, &f->flags))
@@ -474,7 +477,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
fe->port_hi = f->dst->port_no >> 8;
fe->is_local = test_bit(BR_FDB_LOCAL, &f->flags);
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
fe->ageing_timer_value = jiffies_delta_to_clock_t(jiffies - f->updated);
++fe;
++num;
@@ -501,7 +504,8 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
fdb->flags = 0;
if (is_local)
set_bit(BR_FDB_LOCAL, &fdb->flags);
- fdb->is_static = is_static;
+ if (is_static)
+ set_bit(BR_FDB_STATIC, &fdb->flags);
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
@@ -624,7 +628,7 @@ static int fdb_to_nud(const struct net_bridge *br,
{
if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return NUD_PERMANENT;
- else if (fdb->is_static)
+ else if (test_bit(BR_FDB_STATIC, &fdb->flags))
return NUD_NOARP;
else if (has_expired(br, fdb))
return NUD_STALE;
@@ -847,22 +851,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (fdb_to_nud(br, fdb) != state) {
if (state & NUD_PERMANENT) {
set_bit(BR_FDB_LOCAL, &fdb->flags);
- if (!fdb->is_static) {
- fdb->is_static = 1;
+ if (!test_and_set_bit(BR_FDB_STATIC, &fdb->flags))
fdb_add_hw_addr(br, addr);
- }
} else if (state & NUD_NOARP) {
clear_bit(BR_FDB_LOCAL, &fdb->flags);
- if (!fdb->is_static) {
- fdb->is_static = 1;
+ if (!test_and_set_bit(BR_FDB_STATIC, &fdb->flags))
fdb_add_hw_addr(br, addr);
- }
} else {
clear_bit(BR_FDB_LOCAL, &fdb->flags);
- if (fdb->is_static) {
- fdb->is_static = 0;
+ if (test_and_clear_bit(BR_FDB_STATIC, &fdb->flags))
fdb_del_hw_addr(br, addr);
- }
}
modified = true;
@@ -1070,7 +1068,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
rcu_read_lock();
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
continue;
err = dev_uc_add(p->dev, f->key.addr.addr);
if (err)
@@ -1084,7 +1082,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
rollback:
hlist_for_each_entry_rcu(tmp, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!tmp->is_static)
+ if (!test_bit(BR_FDB_STATIC, &tmp->flags))
continue;
if (tmp == f)
break;
@@ -1103,7 +1101,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
rcu_read_lock();
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
continue;
dev_uc_del(p->dev, f->key.addr.addr);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 888cbe9c639a..c5258fad76e5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -175,6 +175,7 @@ struct net_bridge_vlan_group {
/* bridge fdb flags */
enum {
BR_FDB_LOCAL,
+ BR_FDB_STATIC,
};
struct net_bridge_fdb_key {
@@ -189,8 +190,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char is_static:1,
- is_sticky:1,
+ unsigned char is_sticky:1,
added_by_user:1,
added_by_external_learn:1,
offloaded:1;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 2/7] net: bridge: fdb: convert is_static to bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Convert the is_static to bitops, make use of the combined
test_and_set/clear_bit to simplify expressions in fdb_add_entry.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 40 +++++++++++++++++++---------------------
net/bridge/br_private.h | 4 ++--
2 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index e67d5eb8bc1d..1c890e2d694b 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -75,8 +75,9 @@ static inline unsigned long hold_time(const struct net_bridge *br)
static inline int has_expired(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
- return !fdb->is_static && !fdb->added_by_external_learn &&
- time_before_eq(fdb->updated + hold_time(br), jiffies);
+ return !test_bit(BR_FDB_STATIC, &fdb->flags) &&
+ !fdb->added_by_external_learn &&
+ time_before_eq(fdb->updated + hold_time(br), jiffies);
}
static void fdb_rcu_free(struct rcu_head *head)
@@ -197,7 +198,7 @@ static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
{
trace_fdb_delete(br, f);
- if (f->is_static)
+ if (test_bit(BR_FDB_STATIC, &f->flags))
fdb_del_hw_addr(br, f->key.addr.addr);
hlist_del_init_rcu(&f->fdb_node);
@@ -350,7 +351,8 @@ void br_fdb_cleanup(struct work_struct *work)
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
unsigned long this_timer;
- if (f->is_static || f->added_by_external_learn)
+ if (test_bit(BR_FDB_STATIC, &f->flags) ||
+ f->added_by_external_learn)
continue;
this_timer = f->updated + delay;
if (time_after(this_timer, now)) {
@@ -377,7 +379,7 @@ void br_fdb_flush(struct net_bridge *br)
spin_lock_bh(&br->hash_lock);
hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
fdb_delete(br, f, true);
}
spin_unlock_bh(&br->hash_lock);
@@ -401,7 +403,8 @@ void br_fdb_delete_by_port(struct net_bridge *br,
continue;
if (!do_all)
- if (f->is_static || (vid && f->key.vlan_id != vid))
+ if (test_bit(BR_FDB_STATIC, &f->flags) ||
+ (vid && f->key.vlan_id != vid))
continue;
if (test_bit(BR_FDB_LOCAL, &f->flags))
@@ -474,7 +477,7 @@ int br_fdb_fillbuf(struct net_bridge *br, void *buf,
fe->port_hi = f->dst->port_no >> 8;
fe->is_local = test_bit(BR_FDB_LOCAL, &f->flags);
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
fe->ageing_timer_value = jiffies_delta_to_clock_t(jiffies - f->updated);
++fe;
++num;
@@ -501,7 +504,8 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
fdb->flags = 0;
if (is_local)
set_bit(BR_FDB_LOCAL, &fdb->flags);
- fdb->is_static = is_static;
+ if (is_static)
+ set_bit(BR_FDB_STATIC, &fdb->flags);
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
@@ -624,7 +628,7 @@ static int fdb_to_nud(const struct net_bridge *br,
{
if (test_bit(BR_FDB_LOCAL, &fdb->flags))
return NUD_PERMANENT;
- else if (fdb->is_static)
+ else if (test_bit(BR_FDB_STATIC, &fdb->flags))
return NUD_NOARP;
else if (has_expired(br, fdb))
return NUD_STALE;
@@ -847,22 +851,16 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (fdb_to_nud(br, fdb) != state) {
if (state & NUD_PERMANENT) {
set_bit(BR_FDB_LOCAL, &fdb->flags);
- if (!fdb->is_static) {
- fdb->is_static = 1;
+ if (!test_and_set_bit(BR_FDB_STATIC, &fdb->flags))
fdb_add_hw_addr(br, addr);
- }
} else if (state & NUD_NOARP) {
clear_bit(BR_FDB_LOCAL, &fdb->flags);
- if (!fdb->is_static) {
- fdb->is_static = 1;
+ if (!test_and_set_bit(BR_FDB_STATIC, &fdb->flags))
fdb_add_hw_addr(br, addr);
- }
} else {
clear_bit(BR_FDB_LOCAL, &fdb->flags);
- if (fdb->is_static) {
- fdb->is_static = 0;
+ if (test_and_clear_bit(BR_FDB_STATIC, &fdb->flags))
fdb_del_hw_addr(br, addr);
- }
}
modified = true;
@@ -1070,7 +1068,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
rcu_read_lock();
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
continue;
err = dev_uc_add(p->dev, f->key.addr.addr);
if (err)
@@ -1084,7 +1082,7 @@ int br_fdb_sync_static(struct net_bridge *br, struct net_bridge_port *p)
rollback:
hlist_for_each_entry_rcu(tmp, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!tmp->is_static)
+ if (!test_bit(BR_FDB_STATIC, &tmp->flags))
continue;
if (tmp == f)
break;
@@ -1103,7 +1101,7 @@ void br_fdb_unsync_static(struct net_bridge *br, struct net_bridge_port *p)
rcu_read_lock();
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
/* We only care for static entries */
- if (!f->is_static)
+ if (!test_bit(BR_FDB_STATIC, &f->flags))
continue;
dev_uc_del(p->dev, f->key.addr.addr);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 888cbe9c639a..c5258fad76e5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -175,6 +175,7 @@ struct net_bridge_vlan_group {
/* bridge fdb flags */
enum {
BR_FDB_LOCAL,
+ BR_FDB_STATIC,
};
struct net_bridge_fdb_key {
@@ -189,8 +190,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char is_static:1,
- is_sticky:1,
+ unsigned char is_sticky:1,
added_by_user:1,
added_by_external_learn:1,
offloaded:1;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 3/7] net: bridge: fdb: convert is_sticky to bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Straight-forward convert of the is_sticky field to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 12 ++++++------
net/bridge/br_private.h | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 1c890e2d694b..3645c1172b50 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -509,7 +509,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
- fdb->is_sticky = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -590,7 +589,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
unsigned long now = jiffies;
/* fastpath: update of existing entry */
- if (unlikely(source != fdb->dst && !fdb->is_sticky)) {
+ if (unlikely(source != fdb->dst &&
+ !test_bit(BR_FDB_STICKY, &fdb->flags))) {
fdb->dst = source;
fdb_modified = true;
/* Take over HW learned entry */
@@ -662,7 +662,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_flags |= NTF_OFFLOADED;
if (fdb->added_by_external_learn)
ndm->ndm_flags |= NTF_EXT_LEARNED;
- if (fdb->is_sticky)
+ if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
@@ -809,7 +809,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
const u8 *addr, u16 state, u16 flags, u16 vid,
u8 ndm_flags)
{
- u8 is_sticky = !!(ndm_flags & NTF_STICKY);
+ bool is_sticky = !!(ndm_flags & NTF_STICKY);
struct net_bridge_fdb_entry *fdb;
bool modified = false;
@@ -866,8 +866,8 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
- if (is_sticky != fdb->is_sticky) {
- fdb->is_sticky = is_sticky;
+ if (is_sticky != test_bit(BR_FDB_STICKY, &fdb->flags)) {
+ change_bit(BR_FDB_STICKY, &fdb->flags);
modified = true;
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index c5258fad76e5..296f2f12c232 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -176,6 +176,7 @@ struct net_bridge_vlan_group {
enum {
BR_FDB_LOCAL,
BR_FDB_STATIC,
+ BR_FDB_STICKY,
};
struct net_bridge_fdb_key {
@@ -190,8 +191,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char is_sticky:1,
- added_by_user:1,
+ unsigned char added_by_user:1,
added_by_external_learn:1,
offloaded:1;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 3/7] net: bridge: fdb: convert is_sticky to bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Straight-forward convert of the is_sticky field to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 12 ++++++------
net/bridge/br_private.h | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 1c890e2d694b..3645c1172b50 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -509,7 +509,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
- fdb->is_sticky = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -590,7 +589,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
unsigned long now = jiffies;
/* fastpath: update of existing entry */
- if (unlikely(source != fdb->dst && !fdb->is_sticky)) {
+ if (unlikely(source != fdb->dst &&
+ !test_bit(BR_FDB_STICKY, &fdb->flags))) {
fdb->dst = source;
fdb_modified = true;
/* Take over HW learned entry */
@@ -662,7 +662,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_flags |= NTF_OFFLOADED;
if (fdb->added_by_external_learn)
ndm->ndm_flags |= NTF_EXT_LEARNED;
- if (fdb->is_sticky)
+ if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr))
@@ -809,7 +809,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
const u8 *addr, u16 state, u16 flags, u16 vid,
u8 ndm_flags)
{
- u8 is_sticky = !!(ndm_flags & NTF_STICKY);
+ bool is_sticky = !!(ndm_flags & NTF_STICKY);
struct net_bridge_fdb_entry *fdb;
bool modified = false;
@@ -866,8 +866,8 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
- if (is_sticky != fdb->is_sticky) {
- fdb->is_sticky = is_sticky;
+ if (is_sticky != test_bit(BR_FDB_STICKY, &fdb->flags)) {
+ change_bit(BR_FDB_STICKY, &fdb->flags);
modified = true;
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index c5258fad76e5..296f2f12c232 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -176,6 +176,7 @@ struct net_bridge_vlan_group {
enum {
BR_FDB_LOCAL,
BR_FDB_STATIC,
+ BR_FDB_STICKY,
};
struct net_bridge_fdb_key {
@@ -190,8 +191,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char is_sticky:1,
- added_by_user:1,
+ unsigned char added_by_user:1,
added_by_external_learn:1,
offloaded:1;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 4/7] net: bridge: fdb: convert added_by_user to bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Straight-forward convert of the added_by_user field to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 25 ++++++++++++-------------
net/bridge/br_private.h | 4 ++--
net/bridge/br_switchdev.c | 6 ++++--
3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 3645c1172b50..6f00cca4afc8 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -225,7 +225,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
(!vid || br_vlan_find(vg, vid))) {
f->dst = op;
- f->added_by_user = 0;
+ clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
return;
}
}
@@ -236,7 +236,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
(!vid || (v && br_vlan_should_use(v)))) {
f->dst = NULL;
- f->added_by_user = 0;
+ clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
return;
}
@@ -252,7 +252,7 @@ void br_fdb_find_delete_local(struct net_bridge *br,
spin_lock_bh(&br->hash_lock);
f = br_fdb_find(br, addr, vid);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->added_by_user && f->dst == p)
+ !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) && f->dst == p)
fdb_delete_local(br, p, f);
spin_unlock_bh(&br->hash_lock);
}
@@ -268,7 +268,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
vg = nbp_vlan_group(p);
hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->added_by_user) {
+ !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) {
/* delete old one */
fdb_delete_local(br, p, f);
@@ -310,7 +310,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
/* If old entry was unassociated with any port, then delete it. */
f = br_fdb_find(br, br->dev->dev_addr, 0);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->dst && !f->added_by_user)
+ !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, 0);
@@ -326,7 +326,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
continue;
f = br_fdb_find(br, br->dev->dev_addr, v->vid);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->dst && !f->added_by_user)
+ !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, v->vid);
}
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
@@ -600,7 +599,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
if (now != fdb->updated)
fdb->updated = now;
if (unlikely(added_by_user))
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
if (unlikely(fdb_modified)) {
trace_br_fdb_update(br, source, addr, vid, added_by_user);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -611,7 +610,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb = fdb_create(br, source, addr, vid, 0, 0);
if (fdb) {
if (unlikely(added_by_user))
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
trace_br_fdb_update(br, source, addr, vid,
added_by_user);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -871,7 +870,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
fdb->used = jiffies;
if (modified) {
@@ -1129,7 +1128,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
goto err_unlock;
}
if (swdev_notify)
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
fdb->added_by_external_learn = 1;
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
} else {
@@ -1143,14 +1142,14 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
if (fdb->added_by_external_learn) {
/* Refresh entry */
fdb->used = jiffies;
- } else if (!fdb->added_by_user) {
+ } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
/* Take over SW learned entry */
fdb->added_by_external_learn = 1;
modified = true;
}
if (swdev_notify)
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
if (modified)
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 296f2f12c232..bf4a4d1cc3bb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -177,6 +177,7 @@ enum {
BR_FDB_LOCAL,
BR_FDB_STATIC,
BR_FDB_STICKY,
+ BR_FDB_ADDED_BY_USER,
};
struct net_bridge_fdb_key {
@@ -191,8 +192,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char added_by_user:1,
- added_by_external_learn:1,
+ unsigned char added_by_external_learn:1,
offloaded:1;
/* write-heavy members should not affect lookups */
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 921310d3cbae..5010fbf74778 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -129,14 +129,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
br_switchdev_fdb_call_notifiers(false, fdb->key.addr.addr,
fdb->key.vlan_id,
fdb->dst->dev,
- fdb->added_by_user,
+ test_bit(BR_FDB_ADDED_BY_USER,
+ &fdb->flags),
fdb->offloaded);
break;
case RTM_NEWNEIGH:
br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
fdb->key.vlan_id,
fdb->dst->dev,
- fdb->added_by_user,
+ test_bit(BR_FDB_ADDED_BY_USER,
+ &fdb->flags),
fdb->offloaded);
break;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 4/7] net: bridge: fdb: convert added_by_user to bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Straight-forward convert of the added_by_user field to bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 25 ++++++++++++-------------
net/bridge/br_private.h | 4 ++--
net/bridge/br_switchdev.c | 6 ++++--
3 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 3645c1172b50..6f00cca4afc8 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -225,7 +225,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (op != p && ether_addr_equal(op->dev->dev_addr, addr) &&
(!vid || br_vlan_find(vg, vid))) {
f->dst = op;
- f->added_by_user = 0;
+ clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
return;
}
}
@@ -236,7 +236,7 @@ static void fdb_delete_local(struct net_bridge *br,
if (p && ether_addr_equal(br->dev->dev_addr, addr) &&
(!vid || (v && br_vlan_should_use(v)))) {
f->dst = NULL;
- f->added_by_user = 0;
+ clear_bit(BR_FDB_ADDED_BY_USER, &f->flags);
return;
}
@@ -252,7 +252,7 @@ void br_fdb_find_delete_local(struct net_bridge *br,
spin_lock_bh(&br->hash_lock);
f = br_fdb_find(br, addr, vid);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->added_by_user && f->dst == p)
+ !test_bit(BR_FDB_ADDED_BY_USER, &f->flags) && f->dst == p)
fdb_delete_local(br, p, f);
spin_unlock_bh(&br->hash_lock);
}
@@ -268,7 +268,7 @@ void br_fdb_changeaddr(struct net_bridge_port *p, const unsigned char *newaddr)
vg = nbp_vlan_group(p);
hlist_for_each_entry(f, &br->fdb_list, fdb_node) {
if (f->dst == p && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->added_by_user) {
+ !test_bit(BR_FDB_ADDED_BY_USER, &f->flags)) {
/* delete old one */
fdb_delete_local(br, p, f);
@@ -310,7 +310,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
/* If old entry was unassociated with any port, then delete it. */
f = br_fdb_find(br, br->dev->dev_addr, 0);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->dst && !f->added_by_user)
+ !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, 0);
@@ -326,7 +326,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
continue;
f = br_fdb_find(br, br->dev->dev_addr, v->vid);
if (f && test_bit(BR_FDB_LOCAL, &f->flags) &&
- !f->dst && !f->added_by_user)
+ !f->dst && !test_bit(BR_FDB_ADDED_BY_USER, &f->flags))
fdb_delete_local(br, NULL, f);
fdb_insert(br, NULL, newaddr, v->vid);
}
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->added_by_user = 0;
fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
@@ -600,7 +599,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
if (now != fdb->updated)
fdb->updated = now;
if (unlikely(added_by_user))
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
if (unlikely(fdb_modified)) {
trace_br_fdb_update(br, source, addr, vid, added_by_user);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -611,7 +610,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb = fdb_create(br, source, addr, vid, 0, 0);
if (fdb) {
if (unlikely(added_by_user))
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
trace_br_fdb_update(br, source, addr, vid,
added_by_user);
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
@@ -871,7 +870,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
modified = true;
}
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
fdb->used = jiffies;
if (modified) {
@@ -1129,7 +1128,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
goto err_unlock;
}
if (swdev_notify)
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
fdb->added_by_external_learn = 1;
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
} else {
@@ -1143,14 +1142,14 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
if (fdb->added_by_external_learn) {
/* Refresh entry */
fdb->used = jiffies;
- } else if (!fdb->added_by_user) {
+ } else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
/* Take over SW learned entry */
fdb->added_by_external_learn = 1;
modified = true;
}
if (swdev_notify)
- fdb->added_by_user = 1;
+ set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
if (modified)
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 296f2f12c232..bf4a4d1cc3bb 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -177,6 +177,7 @@ enum {
BR_FDB_LOCAL,
BR_FDB_STATIC,
BR_FDB_STICKY,
+ BR_FDB_ADDED_BY_USER,
};
struct net_bridge_fdb_key {
@@ -191,8 +192,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char added_by_user:1,
- added_by_external_learn:1,
+ unsigned char added_by_external_learn:1,
offloaded:1;
/* write-heavy members should not affect lookups */
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 921310d3cbae..5010fbf74778 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -129,14 +129,16 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
br_switchdev_fdb_call_notifiers(false, fdb->key.addr.addr,
fdb->key.vlan_id,
fdb->dst->dev,
- fdb->added_by_user,
+ test_bit(BR_FDB_ADDED_BY_USER,
+ &fdb->flags),
fdb->offloaded);
break;
case RTM_NEWNEIGH:
br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
fdb->key.vlan_id,
fdb->dst->dev,
- fdb->added_by_user,
+ test_bit(BR_FDB_ADDED_BY_USER,
+ &fdb->flags),
fdb->offloaded);
break;
}
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 5/7] net: bridge: fdb: convert added_by_external_learn to use bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Convert the added_by_external_learn field to a flag and use bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 19 +++++++++----------
net/bridge/br_private.h | 4 ++--
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6f00cca4afc8..83d6be3f87f1 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -76,7 +76,7 @@ static inline int has_expired(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
return !test_bit(BR_FDB_STATIC, &fdb->flags) &&
- !fdb->added_by_external_learn &&
+ !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) &&
time_before_eq(fdb->updated + hold_time(br), jiffies);
}
@@ -352,7 +352,7 @@ void br_fdb_cleanup(struct work_struct *work)
unsigned long this_timer;
if (test_bit(BR_FDB_STATIC, &f->flags) ||
- f->added_by_external_learn)
+ test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags))
continue;
this_timer = f->updated + delay;
if (time_after(this_timer, now)) {
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
@@ -593,8 +592,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb->dst = source;
fdb_modified = true;
/* Take over HW learned entry */
- if (unlikely(fdb->added_by_external_learn))
- fdb->added_by_external_learn = 0;
+ test_and_clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+ &fdb->flags);
}
if (now != fdb->updated)
fdb->updated = now;
@@ -659,7 +658,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
if (fdb->offloaded)
ndm->ndm_flags |= NTF_OFFLOADED;
- if (fdb->added_by_external_learn)
+ if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
ndm->ndm_flags |= NTF_EXT_LEARNED;
if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
@@ -1129,7 +1128,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
}
if (swdev_notify)
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
- fdb->added_by_external_learn = 1;
+ set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
} else {
fdb->updated = jiffies;
@@ -1139,12 +1138,12 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
modified = true;
}
- if (fdb->added_by_external_learn) {
+ if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
/* Refresh entry */
fdb->used = jiffies;
} else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
/* Take over SW learned entry */
- fdb->added_by_external_learn = 1;
+ set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
modified = true;
}
@@ -1171,7 +1170,7 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);
fdb = br_fdb_find(br, addr, vid);
- if (fdb && fdb->added_by_external_learn)
+ if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
fdb_delete(br, fdb, swdev_notify);
else
err = -ENOENT;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index bf4a4d1cc3bb..cf325177a34e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -178,6 +178,7 @@ enum {
BR_FDB_STATIC,
BR_FDB_STICKY,
BR_FDB_ADDED_BY_USER,
+ BR_FDB_ADDED_BY_EXT_LEARN,
};
struct net_bridge_fdb_key {
@@ -192,8 +193,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char added_by_external_learn:1,
- offloaded:1;
+ unsigned char offloaded:1;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 5/7] net: bridge: fdb: convert added_by_external_learn to use bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Convert the added_by_external_learn field to a flag and use bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 19 +++++++++----------
net/bridge/br_private.h | 4 ++--
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 6f00cca4afc8..83d6be3f87f1 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -76,7 +76,7 @@ static inline int has_expired(const struct net_bridge *br,
const struct net_bridge_fdb_entry *fdb)
{
return !test_bit(BR_FDB_STATIC, &fdb->flags) &&
- !fdb->added_by_external_learn &&
+ !test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags) &&
time_before_eq(fdb->updated + hold_time(br), jiffies);
}
@@ -352,7 +352,7 @@ void br_fdb_cleanup(struct work_struct *work)
unsigned long this_timer;
if (test_bit(BR_FDB_STATIC, &f->flags) ||
- f->added_by_external_learn)
+ test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags))
continue;
this_timer = f->updated + delay;
if (time_after(this_timer, now)) {
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->added_by_external_learn = 0;
fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
@@ -593,8 +592,8 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
fdb->dst = source;
fdb_modified = true;
/* Take over HW learned entry */
- if (unlikely(fdb->added_by_external_learn))
- fdb->added_by_external_learn = 0;
+ test_and_clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
+ &fdb->flags);
}
if (now != fdb->updated)
fdb->updated = now;
@@ -659,7 +658,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
if (fdb->offloaded)
ndm->ndm_flags |= NTF_OFFLOADED;
- if (fdb->added_by_external_learn)
+ if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
ndm->ndm_flags |= NTF_EXT_LEARNED;
if (test_bit(BR_FDB_STICKY, &fdb->flags))
ndm->ndm_flags |= NTF_STICKY;
@@ -1129,7 +1128,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
}
if (swdev_notify)
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
- fdb->added_by_external_learn = 1;
+ set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
fdb_notify(br, fdb, RTM_NEWNEIGH, swdev_notify);
} else {
fdb->updated = jiffies;
@@ -1139,12 +1138,12 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
modified = true;
}
- if (fdb->added_by_external_learn) {
+ if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags)) {
/* Refresh entry */
fdb->used = jiffies;
} else if (!test_bit(BR_FDB_ADDED_BY_USER, &fdb->flags)) {
/* Take over SW learned entry */
- fdb->added_by_external_learn = 1;
+ set_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags);
modified = true;
}
@@ -1171,7 +1170,7 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);
fdb = br_fdb_find(br, addr, vid);
- if (fdb && fdb->added_by_external_learn)
+ if (fdb && test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
fdb_delete(br, fdb, swdev_notify);
else
err = -ENOENT;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index bf4a4d1cc3bb..cf325177a34e 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -178,6 +178,7 @@ enum {
BR_FDB_STATIC,
BR_FDB_STICKY,
BR_FDB_ADDED_BY_USER,
+ BR_FDB_ADDED_BY_EXT_LEARN,
};
struct net_bridge_fdb_key {
@@ -192,8 +193,7 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char added_by_external_learn:1,
- offloaded:1;
+ unsigned char offloaded:1;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 6/7] net: bridge: fdb: convert offloaded to use bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
Convert the offloaded field to a flag and use bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 9 ++++-----
net/bridge/br_private.h | 2 +-
net/bridge/br_switchdev.c | 6 ++++--
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 83d6be3f87f1..d4f6b398303d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -656,7 +655,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_ifindex = fdb->dst ? fdb->dst->dev->ifindex : br->dev->ifindex;
ndm->ndm_state = fdb_to_nud(br, fdb);
- if (fdb->offloaded)
+ if (test_bit(BR_FDB_OFFLOADED, &fdb->flags))
ndm->ndm_flags |= NTF_OFFLOADED;
if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
ndm->ndm_flags |= NTF_EXT_LEARNED;
@@ -1188,8 +1187,8 @@ void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);
fdb = br_fdb_find(br, addr, vid);
- if (fdb)
- fdb->offloaded = offloaded;
+ if (fdb && offloaded != test_bit(BR_FDB_OFFLOADED, &fdb->flags))
+ change_bit(BR_FDB_OFFLOADED, &fdb->flags);
spin_unlock_bh(&br->hash_lock);
}
@@ -1208,7 +1207,7 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
spin_lock_bh(&p->br->hash_lock);
hlist_for_each_entry(f, &p->br->fdb_list, fdb_node) {
if (f->dst == p && f->key.vlan_id == vid)
- f->offloaded = 0;
+ clear_bit(BR_FDB_OFFLOADED, &f->flags);
}
spin_unlock_bh(&p->br->hash_lock);
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index cf325177a34e..f4754bf7f4bd 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -179,6 +179,7 @@ enum {
BR_FDB_STICKY,
BR_FDB_ADDED_BY_USER,
BR_FDB_ADDED_BY_EXT_LEARN,
+ BR_FDB_OFFLOADED,
};
struct net_bridge_fdb_key {
@@ -193,7 +194,6 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char offloaded:1;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 5010fbf74778..015209bf44aa 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -131,7 +131,8 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
fdb->dst->dev,
test_bit(BR_FDB_ADDED_BY_USER,
&fdb->flags),
- fdb->offloaded);
+ test_bit(BR_FDB_OFFLOADED,
+ &fdb->flags));
break;
case RTM_NEWNEIGH:
br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
@@ -139,7 +140,8 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
fdb->dst->dev,
test_bit(BR_FDB_ADDED_BY_USER,
&fdb->flags),
- fdb->offloaded);
+ test_bit(BR_FDB_OFFLOADED,
+ &fdb->flags));
break;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 6/7] net: bridge: fdb: convert offloaded to use bitops
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
Convert the offloaded field to a flag and use bitops.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 9 ++++-----
net/bridge/br_private.h | 2 +-
net/bridge/br_switchdev.c | 6 ++++--
3 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 83d6be3f87f1..d4f6b398303d 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -506,7 +506,6 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
set_bit(BR_FDB_LOCAL, &fdb->flags);
if (is_static)
set_bit(BR_FDB_STATIC, &fdb->flags);
- fdb->offloaded = 0;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -656,7 +655,7 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br,
ndm->ndm_ifindex = fdb->dst ? fdb->dst->dev->ifindex : br->dev->ifindex;
ndm->ndm_state = fdb_to_nud(br, fdb);
- if (fdb->offloaded)
+ if (test_bit(BR_FDB_OFFLOADED, &fdb->flags))
ndm->ndm_flags |= NTF_OFFLOADED;
if (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &fdb->flags))
ndm->ndm_flags |= NTF_EXT_LEARNED;
@@ -1188,8 +1187,8 @@ void br_fdb_offloaded_set(struct net_bridge *br, struct net_bridge_port *p,
spin_lock_bh(&br->hash_lock);
fdb = br_fdb_find(br, addr, vid);
- if (fdb)
- fdb->offloaded = offloaded;
+ if (fdb && offloaded != test_bit(BR_FDB_OFFLOADED, &fdb->flags))
+ change_bit(BR_FDB_OFFLOADED, &fdb->flags);
spin_unlock_bh(&br->hash_lock);
}
@@ -1208,7 +1207,7 @@ void br_fdb_clear_offload(const struct net_device *dev, u16 vid)
spin_lock_bh(&p->br->hash_lock);
hlist_for_each_entry(f, &p->br->fdb_list, fdb_node) {
if (f->dst == p && f->key.vlan_id == vid)
- f->offloaded = 0;
+ clear_bit(BR_FDB_OFFLOADED, &f->flags);
}
spin_unlock_bh(&p->br->hash_lock);
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index cf325177a34e..f4754bf7f4bd 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -179,6 +179,7 @@ enum {
BR_FDB_STICKY,
BR_FDB_ADDED_BY_USER,
BR_FDB_ADDED_BY_EXT_LEARN,
+ BR_FDB_OFFLOADED,
};
struct net_bridge_fdb_key {
@@ -193,7 +194,6 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
unsigned long flags;
- unsigned char offloaded:1;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index 5010fbf74778..015209bf44aa 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -131,7 +131,8 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
fdb->dst->dev,
test_bit(BR_FDB_ADDED_BY_USER,
&fdb->flags),
- fdb->offloaded);
+ test_bit(BR_FDB_OFFLOADED,
+ &fdb->flags));
break;
case RTM_NEWNEIGH:
br_switchdev_fdb_call_notifiers(true, fdb->key.addr.addr,
@@ -139,7 +140,8 @@ br_switchdev_fdb_notify(const struct net_bridge_fdb_entry *fdb, int type)
fdb->dst->dev,
test_bit(BR_FDB_ADDED_BY_USER,
&fdb->flags),
- fdb->offloaded);
+ test_bit(BR_FDB_OFFLOADED,
+ &fdb->flags));
break;
}
}
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 7/7] net: bridge: fdb: set flags directly in fdb_create
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: davem, roopa, bridge, Nikolay Aleksandrov
No need to have separate arguments for each flag, just set the flags to
whatever was passed to fdb_create() before the fdb is published.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d4f6b398303d..f244f2ac7156 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -491,8 +491,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
struct net_bridge_port *source,
const unsigned char *addr,
__u16 vid,
- unsigned char is_local,
- unsigned char is_static)
+ unsigned long flags)
{
struct net_bridge_fdb_entry *fdb;
@@ -501,11 +500,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
fdb->dst = source;
fdb->key.vlan_id = vid;
- fdb->flags = 0;
- if (is_local)
- set_bit(BR_FDB_LOCAL, &fdb->flags);
- if (is_static)
- set_bit(BR_FDB_STATIC, &fdb->flags);
+ fdb->flags = flags;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -539,7 +534,8 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
fdb_delete(br, fdb, true);
}
- fdb = fdb_create(br, source, addr, vid, 1, 1);
+ fdb = fdb_create(br, source, addr, vid,
+ BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
if (!fdb)
return -ENOMEM;
@@ -605,7 +601,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
}
} else {
spin_lock(&br->hash_lock);
- fdb = fdb_create(br, source, addr, vid, 0, 0);
+ fdb = fdb_create(br, source, addr, vid, 0);
if (fdb) {
if (unlikely(added_by_user))
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
@@ -830,7 +826,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (!(flags & NLM_F_CREATE))
return -ENOENT;
- fdb = fdb_create(br, source, addr, vid, 0, 0);
+ fdb = fdb_create(br, source, addr, vid, 0);
if (!fdb)
return -ENOMEM;
@@ -1120,7 +1116,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
fdb = br_fdb_find(br, addr, vid);
if (!fdb) {
- fdb = fdb_create(br, p, addr, vid, 0, 0);
+ fdb = fdb_create(br, p, addr, vid, 0);
if (!fdb) {
err = -ENOMEM;
goto err_unlock;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [Bridge] [PATCH net-next 7/7] net: bridge: fdb: set flags directly in fdb_create
@ 2019-10-29 11:45 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-29 11:45 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov, roopa, bridge, davem
No need to have separate arguments for each flag, just set the flags to
whatever was passed to fdb_create() before the fdb is published.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/bridge/br_fdb.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index d4f6b398303d..f244f2ac7156 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -491,8 +491,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
struct net_bridge_port *source,
const unsigned char *addr,
__u16 vid,
- unsigned char is_local,
- unsigned char is_static)
+ unsigned long flags)
{
struct net_bridge_fdb_entry *fdb;
@@ -501,11 +500,7 @@ static struct net_bridge_fdb_entry *fdb_create(struct net_bridge *br,
memcpy(fdb->key.addr.addr, addr, ETH_ALEN);
fdb->dst = source;
fdb->key.vlan_id = vid;
- fdb->flags = 0;
- if (is_local)
- set_bit(BR_FDB_LOCAL, &fdb->flags);
- if (is_static)
- set_bit(BR_FDB_STATIC, &fdb->flags);
+ fdb->flags = flags;
fdb->updated = fdb->used = jiffies;
if (rhashtable_lookup_insert_fast(&br->fdb_hash_tbl,
&fdb->rhnode,
@@ -539,7 +534,8 @@ static int fdb_insert(struct net_bridge *br, struct net_bridge_port *source,
fdb_delete(br, fdb, true);
}
- fdb = fdb_create(br, source, addr, vid, 1, 1);
+ fdb = fdb_create(br, source, addr, vid,
+ BIT(BR_FDB_LOCAL) | BIT(BR_FDB_STATIC));
if (!fdb)
return -ENOMEM;
@@ -605,7 +601,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
}
} else {
spin_lock(&br->hash_lock);
- fdb = fdb_create(br, source, addr, vid, 0, 0);
+ fdb = fdb_create(br, source, addr, vid, 0);
if (fdb) {
if (unlikely(added_by_user))
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
@@ -830,7 +826,7 @@ static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
if (!(flags & NLM_F_CREATE))
return -ENOENT;
- fdb = fdb_create(br, source, addr, vid, 0, 0);
+ fdb = fdb_create(br, source, addr, vid, 0);
if (!fdb)
return -ENOMEM;
@@ -1120,7 +1116,7 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p,
fdb = br_fdb_find(br, addr, vid);
if (!fdb) {
- fdb = fdb_create(br, p, addr, vid, 0, 0);
+ fdb = fdb_create(br, p, addr, vid, 0);
if (!fdb) {
err = -ENOMEM;
goto err_unlock;
--
2.21.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-30 1:13 ` David Miller
-1 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2019-10-30 1:13 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Tue, 29 Oct 2019 13:45:52 +0200
> We'd like to have a well-defined behaviour when changing fdb flags. The
> problem is that we've added new fields which are changed from all
> contexts without any locking. We are aware of the bit test/change races
> and these are fine (we can remove them later), but it is considered
> undefined behaviour to change bitfields from multiple threads and also
> on some architectures that can result in unexpected results,
> specifically when all fields between the changed ones are also
> bitfields. The conversion to bitops shows the intent clearly and
> makes them use functions with well-defined behaviour in such cases.
> There is no overhead for the fast-path, the bit changing functions are
> used only in special cases when learning and in the slow path.
> In addition this conversion allows us to simplify fdb flag handling and
> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
> allocating a new fdb). All bridge selftests passed, also tried all of the
> converted bits manually in a VM.
Series applied, thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Bridge] [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
@ 2019-10-30 1:13 ` David Miller
0 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2019-10-30 1:13 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, bridge
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Tue, 29 Oct 2019 13:45:52 +0200
> We'd like to have a well-defined behaviour when changing fdb flags. The
> problem is that we've added new fields which are changed from all
> contexts without any locking. We are aware of the bit test/change races
> and these are fine (we can remove them later), but it is considered
> undefined behaviour to change bitfields from multiple threads and also
> on some architectures that can result in unexpected results,
> specifically when all fields between the changed ones are also
> bitfields. The conversion to bitops shows the intent clearly and
> makes them use functions with well-defined behaviour in such cases.
> There is no overhead for the fast-path, the bit changing functions are
> used only in special cases when learning and in the slow path.
> In addition this conversion allows us to simplify fdb flag handling and
> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
> allocating a new fdb). All bridge selftests passed, also tried all of the
> converted bits manually in a VM.
Series applied, thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
@ 2019-10-31 18:37 ` Joe Perches
-1 siblings, 0 replies; 22+ messages in thread
From: Joe Perches @ 2019-10-31 18:37 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev; +Cc: davem, roopa, bridge
On Tue, 2019-10-29 at 13:45 +0200, Nikolay Aleksandrov wrote:
> Hi,
> We'd like to have a well-defined behaviour when changing fdb flags. The
> problem is that we've added new fields which are changed from all
> contexts without any locking. We are aware of the bit test/change races
> and these are fine (we can remove them later), but it is considered
> undefined behaviour to change bitfields from multiple threads and also
> on some architectures that can result in unexpected results,
> specifically when all fields between the changed ones are also
> bitfields. The conversion to bitops shows the intent clearly and
> makes them use functions with well-defined behaviour in such cases.
> There is no overhead for the fast-path, the bit changing functions are
> used only in special cases when learning and in the slow path.
> In addition this conversion allows us to simplify fdb flag handling and
> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
> allocating a new fdb). All bridge selftests passed, also tried all of the
> converted bits manually in a VM.
>
> Thanks,
> Nik
>
> Nikolay Aleksandrov (7):
> net: bridge: fdb: convert is_local to bitops
> net: bridge: fdb: convert is_static to bitops
> net: bridge: fdb: convert is_sticky to bitops
> net: bridge: fdb: convert added_by_user to bitops
> net: bridge: fdb: convert added_by_external_learn to use bitops
> net: bridge: fdb: convert offloaded to use bitops
> net: bridge: fdb: set flags directly in fdb_create
Wouldn't it be simpler to change all these bitfields to bool?
The next member is ____cachline_aligned_in_smp so it's not
like the struct size matters or likely even changes.
---
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ce2ab1..46d2f10 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -183,12 +183,12 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
- unsigned char is_local:1,
- is_static:1,
- is_sticky:1,
- added_by_user:1,
- added_by_external_learn:1,
- offloaded:1;
+ bool is_local;
+ bool is_static;
+ bool is_sticky;
+ bool added_by_user;
+ bool added_by_external_learn;
+ bool offloaded;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Bridge] [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
@ 2019-10-31 18:37 ` Joe Perches
0 siblings, 0 replies; 22+ messages in thread
From: Joe Perches @ 2019-10-31 18:37 UTC (permalink / raw)
To: Nikolay Aleksandrov, netdev; +Cc: roopa, bridge, davem
On Tue, 2019-10-29 at 13:45 +0200, Nikolay Aleksandrov wrote:
> Hi,
> We'd like to have a well-defined behaviour when changing fdb flags. The
> problem is that we've added new fields which are changed from all
> contexts without any locking. We are aware of the bit test/change races
> and these are fine (we can remove them later), but it is considered
> undefined behaviour to change bitfields from multiple threads and also
> on some architectures that can result in unexpected results,
> specifically when all fields between the changed ones are also
> bitfields. The conversion to bitops shows the intent clearly and
> makes them use functions with well-defined behaviour in such cases.
> There is no overhead for the fast-path, the bit changing functions are
> used only in special cases when learning and in the slow path.
> In addition this conversion allows us to simplify fdb flag handling and
> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
> allocating a new fdb). All bridge selftests passed, also tried all of the
> converted bits manually in a VM.
>
> Thanks,
> Nik
>
> Nikolay Aleksandrov (7):
> net: bridge: fdb: convert is_local to bitops
> net: bridge: fdb: convert is_static to bitops
> net: bridge: fdb: convert is_sticky to bitops
> net: bridge: fdb: convert added_by_user to bitops
> net: bridge: fdb: convert added_by_external_learn to use bitops
> net: bridge: fdb: convert offloaded to use bitops
> net: bridge: fdb: set flags directly in fdb_create
Wouldn't it be simpler to change all these bitfields to bool?
The next member is ____cachline_aligned_in_smp so it's not
like the struct size matters or likely even changes.
---
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index ce2ab1..46d2f10 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -183,12 +183,12 @@ struct net_bridge_fdb_entry {
struct net_bridge_fdb_key key;
struct hlist_node fdb_node;
- unsigned char is_local:1,
- is_static:1,
- is_sticky:1,
- added_by_user:1,
- added_by_external_learn:1,
- offloaded:1;
+ bool is_local;
+ bool is_static;
+ bool is_sticky;
+ bool added_by_user;
+ bool added_by_external_learn;
+ bool offloaded;
/* write-heavy members should not affect lookups */
unsigned long updated ____cacheline_aligned_in_smp;
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
2019-10-31 18:37 ` [Bridge] " Joe Perches
@ 2019-10-31 19:28 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-31 19:28 UTC (permalink / raw)
To: Joe Perches, netdev; +Cc: davem, roopa, bridge
On 31/10/2019 20:37, Joe Perches wrote:
> On Tue, 2019-10-29 at 13:45 +0200, Nikolay Aleksandrov wrote:
>> Hi,
>> We'd like to have a well-defined behaviour when changing fdb flags. The
>> problem is that we've added new fields which are changed from all
>> contexts without any locking. We are aware of the bit test/change races
>> and these are fine (we can remove them later), but it is considered
>> undefined behaviour to change bitfields from multiple threads and also
>> on some architectures that can result in unexpected results,
>> specifically when all fields between the changed ones are also
>> bitfields. The conversion to bitops shows the intent clearly and
>> makes them use functions with well-defined behaviour in such cases.
>> There is no overhead for the fast-path, the bit changing functions are
>> used only in special cases when learning and in the slow path.
>> In addition this conversion allows us to simplify fdb flag handling and
>> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
>> allocating a new fdb). All bridge selftests passed, also tried all of the
>> converted bits manually in a VM.
>>
>> Thanks,
>> Nik
>>
>> Nikolay Aleksandrov (7):
>> net: bridge: fdb: convert is_local to bitops
>> net: bridge: fdb: convert is_static to bitops
>> net: bridge: fdb: convert is_sticky to bitops
>> net: bridge: fdb: convert added_by_user to bitops
>> net: bridge: fdb: convert added_by_external_learn to use bitops
>> net: bridge: fdb: convert offloaded to use bitops
>> net: bridge: fdb: set flags directly in fdb_create
>
> Wouldn't it be simpler to change all these bitfields to bool?
>
> The next member is ____cachline_aligned_in_smp so it's not
> like the struct size matters or likely even changes.
>
I guess it's just preference now, I'd prefer having 1 field which is well
packed and can contain more bits (and more are to come) instead of bunch
of bool or u8 fields which is a waste of space. We can set them together, it's more
compact and also the atomic bitops make it clear that these can change
without any locking. We're about to add new bits to these and it's nice
to have a clear understanding and well-defined API for them. Specifically
the test_and_set/clear_bit() can simplify code considerably.
> ---
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ce2ab1..46d2f10 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -183,12 +183,12 @@ struct net_bridge_fdb_entry {
>
> struct net_bridge_fdb_key key;
> struct hlist_node fdb_node;
> - unsigned char is_local:1,
> - is_static:1,
> - is_sticky:1,
> - added_by_user:1,
> - added_by_external_learn:1,
> - offloaded:1;
> + bool is_local;
> + bool is_static;
> + bool is_sticky;
> + bool added_by_user;
> + bool added_by_external_learn;
> + bool offloaded;
>
> /* write-heavy members should not affect lookups */
> unsigned long updated ____cacheline_aligned_in_smp;
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Bridge] [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops
@ 2019-10-31 19:28 ` Nikolay Aleksandrov
0 siblings, 0 replies; 22+ messages in thread
From: Nikolay Aleksandrov @ 2019-10-31 19:28 UTC (permalink / raw)
To: Joe Perches, netdev; +Cc: roopa, bridge, davem
On 31/10/2019 20:37, Joe Perches wrote:
> On Tue, 2019-10-29 at 13:45 +0200, Nikolay Aleksandrov wrote:
>> Hi,
>> We'd like to have a well-defined behaviour when changing fdb flags. The
>> problem is that we've added new fields which are changed from all
>> contexts without any locking. We are aware of the bit test/change races
>> and these are fine (we can remove them later), but it is considered
>> undefined behaviour to change bitfields from multiple threads and also
>> on some architectures that can result in unexpected results,
>> specifically when all fields between the changed ones are also
>> bitfields. The conversion to bitops shows the intent clearly and
>> makes them use functions with well-defined behaviour in such cases.
>> There is no overhead for the fast-path, the bit changing functions are
>> used only in special cases when learning and in the slow path.
>> In addition this conversion allows us to simplify fdb flag handling and
>> avoid bugs for future bits (e.g. a forgetting to clear the new bit when
>> allocating a new fdb). All bridge selftests passed, also tried all of the
>> converted bits manually in a VM.
>>
>> Thanks,
>> Nik
>>
>> Nikolay Aleksandrov (7):
>> net: bridge: fdb: convert is_local to bitops
>> net: bridge: fdb: convert is_static to bitops
>> net: bridge: fdb: convert is_sticky to bitops
>> net: bridge: fdb: convert added_by_user to bitops
>> net: bridge: fdb: convert added_by_external_learn to use bitops
>> net: bridge: fdb: convert offloaded to use bitops
>> net: bridge: fdb: set flags directly in fdb_create
>
> Wouldn't it be simpler to change all these bitfields to bool?
>
> The next member is ____cachline_aligned_in_smp so it's not
> like the struct size matters or likely even changes.
>
I guess it's just preference now, I'd prefer having 1 field which is well
packed and can contain more bits (and more are to come) instead of bunch
of bool or u8 fields which is a waste of space. We can set them together, it's more
compact and also the atomic bitops make it clear that these can change
without any locking. We're about to add new bits to these and it's nice
to have a clear understanding and well-defined API for them. Specifically
the test_and_set/clear_bit() can simplify code considerably.
> ---
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index ce2ab1..46d2f10 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -183,12 +183,12 @@ struct net_bridge_fdb_entry {
>
> struct net_bridge_fdb_key key;
> struct hlist_node fdb_node;
> - unsigned char is_local:1,
> - is_static:1,
> - is_sticky:1,
> - added_by_user:1,
> - added_by_external_learn:1,
> - offloaded:1;
> + bool is_local;
> + bool is_static;
> + bool is_sticky;
> + bool added_by_user;
> + bool added_by_external_learn;
> + bool offloaded;
>
> /* write-heavy members should not affect lookups */
> unsigned long updated ____cacheline_aligned_in_smp;
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-10-31 19:28 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-29 11:45 [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 1/7] net: bridge: fdb: convert is_local to bitops Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 2/7] net: bridge: fdb: convert is_static " Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 3/7] net: bridge: fdb: convert is_sticky " Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 4/7] net: bridge: fdb: convert added_by_user " Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 5/7] net: bridge: fdb: convert added_by_external_learn to use bitops Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 6/7] net: bridge: fdb: convert offloaded " Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-29 11:45 ` [PATCH net-next 7/7] net: bridge: fdb: set flags directly in fdb_create Nikolay Aleksandrov
2019-10-29 11:45 ` [Bridge] " Nikolay Aleksandrov
2019-10-30 1:13 ` [PATCH net-next 0/7] net: bridge: convert fdbs to use bitops David Miller
2019-10-30 1:13 ` [Bridge] " David Miller
2019-10-31 18:37 ` Joe Perches
2019-10-31 18:37 ` [Bridge] " Joe Perches
2019-10-31 19:28 ` Nikolay Aleksandrov
2019-10-31 19:28 ` [Bridge] " Nikolay Aleksandrov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.