Nikolay Aleksandrov
2020-Jun-23 20:47 UTC
[Bridge] [PATCH net-next 0/4] net: bridge: fdb activity tracking
Hi,
This set adds extensions needed for EVPN multi-homing proper and
efficient mac sync. User-space (e.g. FRR) needs to be able to track
non-dynamic entry activity on per-fdb basis depending if a tracked fdb is
currently peer active or locally active and needs to be able to add new
peer active fdb (static + track + inactive) without refreshing it to get
real activity tracking. Patch 02 adds a new NDA attribute - NDA_FDB_EXT_ATTRS
to avoid future pollution of NDA attributes by bridge or vxlan. New
bridge/vxlan specific fdb attributes are embedded in NDA_FDB_EXT_ATTRS,
which is used in patch 03 to pass the new NFEA_ACTIVITY_NOTIFY attribute
which controls if an fdb should be tracked and also reflects its current
state when dumping. It is treated as a bitfield, current valid bits are:
1 - mark an entry for activity tracking
2 - mark an entry as inactive to avoid multiple notifications and
reflect state properly
Patch 04 adds the ability to avoid refreshing an entry when changing it
via the NFEA_DONT_REFRESH flag. That allows user-space to mark a static
entry for tracking and keep its real activity unchanged.
The set has been extensively tested with FRR and those changes will
be upstreamed if/after it gets accepted.
Thanks,
Nik
Nikolay Aleksandrov (4):
net: bridge: fdb_add_entry takes ndm as argument
net: neighbor: add fdb extended attribute
net: bridge: add option to allow activity notifications for any fdb
entries
net: bridge: add a flag to avoid refreshing fdb when changing/adding
include/uapi/linux/neighbour.h | 24 +++++++
net/bridge/br_fdb.c | 127 ++++++++++++++++++++++++++++-----
net/bridge/br_private.h | 4 ++
net/core/neighbour.c | 1 +
4 files changed, 139 insertions(+), 17 deletions(-)
--
2.25.4
Nikolay Aleksandrov
2020-Jun-23 20:47 UTC
[Bridge] [PATCH net-next 1/4] net: bridge: fdb_add_entry takes ndm as argument
We can just pass ndm as an argument instead of its fields separately.
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
net/bridge/br_fdb.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 4877a0db16c6..ed80d9ab0fb9 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -793,11 +793,11 @@ int br_fdb_get(struct sk_buff *skb,
/* Update (create or replace) forwarding database entry */
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)
+ const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid)
{
- bool is_sticky = !!(ndm_flags & NTF_STICKY);
+ bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
struct net_bridge_fdb_entry *fdb;
+ u16 state = ndm->ndm_state;
bool modified = false;
/* If the port cannot learn allow only local and static entries */
@@ -893,8 +893,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge
*br,
err = br_fdb_external_learn_add(br, p, addr, vid, true);
} else {
spin_lock_bh(&br->hash_lock);
- err = fdb_add_entry(br, p, addr, ndm->ndm_state,
- nlh_flags, vid, ndm->ndm_flags);
+ err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid);
spin_unlock_bh(&br->hash_lock);
}
--
2.25.4
Nikolay Aleksandrov
2020-Jun-23 20:47 UTC
[Bridge] [PATCH net-next 2/4] net: neighbor: add fdb extended attribute
Add an attribute to NDA which will contain all future fdb-specific
attributes in order to avoid polluting the NDA namespace with e.g.
bridge or vxlan specific attributes. The attribute is called
NDA_FDB_EXT_ATTRS and the structure would look like:
[NDA_FDB_EXT_ATTRS] = {
[NFEA_xxx]
}
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
include/uapi/linux/neighbour.h | 12 ++++++++++++
net/core/neighbour.c | 1 +
2 files changed, 13 insertions(+)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index eefcda8ca44e..540ff48402a1 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -30,6 +30,7 @@ enum {
NDA_SRC_VNI,
NDA_PROTOCOL, /* Originator of entry */
NDA_NH_ID,
+ NDA_FDB_EXT_ATTRS,
__NDA_MAX
};
@@ -172,4 +173,15 @@ enum {
};
#define NDTA_MAX (__NDTA_MAX - 1)
+/* embedded into NDA_FDB_EXT_ATTRS:
+ * [NDA_FDB_EXT_ATTRS] = {
+ * ...
+ * }
+ */
+enum {
+ NFEA_UNSPEC,
+ __NFEA_MAX
+};
+#define NFEA_MAX (__NFEA_MAX - 1)
+
#endif
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index ef6b5a8f629c..8e39e28b0a8d 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1783,6 +1783,7 @@ const struct nla_policy nda_policy[NDA_MAX+1] = {
[NDA_MASTER] = { .type = NLA_U32 },
[NDA_PROTOCOL] = { .type = NLA_U8 },
[NDA_NH_ID] = { .type = NLA_U32 },
+ [NDA_FDB_EXT_ATTRS] = { .type = NLA_NESTED },
};
static int neigh_delete(struct sk_buff *skb, struct nlmsghdr *nlh,
--
2.25.4
Nikolay Aleksandrov
2020-Jun-23 20:47 UTC
[Bridge] [PATCH net-next 3/4] net: bridge: add option to allow activity notifications for any fdb entries
This patch adds the ability to notify about activity of any entries
(static, permanent or ext_learn). EVPN multihoming peers need it to
properly and efficiently handle mac sync (peer active/locally active).
We add a new NFEA_ACTIVITY_NOTIFY attribute which is used to dump the
current activity state and to control if static entries should be monitored
at all. We use 2 bits - one to activate fdb entry tracking (disabled by
default) and the second to denote that an entry is inactive. We need
the second bit in order to avoid multiple notifications of inactivity.
Obviously this makes no difference for dynamic entries since at the time
of inactivity they get deleted, while the tracked non-dynamic entries get
the inactive bit set and get a notification.
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
include/uapi/linux/neighbour.h | 11 ++++
net/bridge/br_fdb.c | 117 +++++++++++++++++++++++++++++----
net/bridge/br_private.h | 4 ++
3 files changed, 119 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 540ff48402a1..21e569297355 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -173,13 +173,24 @@ enum {
};
#define NDTA_MAX (__NDTA_MAX - 1)
+ /* FDB activity notification bits used in NFEA_ACTIVITY_NOTIFY:
+ * - FDB_NOTIFY_BIT - notify on activity/expire for any entry
+ * - FDB_NOTIFY_INACTIVE_BIT - mark as inactive to avoid multiple
notifications
+ */
+enum {
+ FDB_NOTIFY_BIT = (1 << 0),
+ FDB_NOTIFY_INACTIVE_BIT = (1 << 1)
+};
+
/* embedded into NDA_FDB_EXT_ATTRS:
* [NDA_FDB_EXT_ATTRS] = {
+ * [NFEA_ACTIVITY_NOTIFY]
* ...
* }
*/
enum {
NFEA_UNSPEC,
+ NFEA_ACTIVITY_NOTIFY,
__NFEA_MAX
};
#define NFEA_MAX (__NFEA_MAX - 1)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index ed80d9ab0fb9..642deb57c064 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -349,12 +349,21 @@ void br_fdb_cleanup(struct work_struct *work)
*/
rcu_read_lock();
hlist_for_each_entry_rcu(f, &br->fdb_list, fdb_node) {
- unsigned long this_timer;
+ unsigned long this_timer = f->updated + delay;
if (test_bit(BR_FDB_STATIC, &f->flags) ||
- test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags))
+ test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags)) {
+ if (test_bit(BR_FDB_NOTIFY, &f->flags)) {
+ if (time_after(this_timer, now))
+ work_delay = min(work_delay,
+ this_timer - now);
+ else if (!test_and_set_bit(BR_FDB_NOTIFY_INACTIVE,
+ &f->flags))
+ fdb_notify(br, f, RTM_NEWNEIGH, false);
+ }
continue;
- this_timer = f->updated + delay;
+ }
+
if (time_after(this_timer, now)) {
work_delay = min(work_delay, this_timer - now);
} else {
@@ -556,11 +565,17 @@ int br_fdb_insert(struct net_bridge *br, struct
net_bridge_port *source,
return ret;
}
+/* returns true if the fdb was modified */
+static bool __fdb_mark_active(struct net_bridge_fdb_entry *fdb)
+{
+ return !!(test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags) &&
+ test_and_clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags));
+}
+
void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source,
const unsigned char *addr, u16 vid, unsigned long flags)
{
struct net_bridge_fdb_entry *fdb;
- bool fdb_modified = false;
/* some users want to always flood. */
if (hold_time(br) == 0)
@@ -575,6 +590,12 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
source->dev->name, addr, vid);
} else {
unsigned long now = jiffies;
+ bool fdb_modified = false;
+
+ if (now != fdb->updated) {
+ fdb->updated = now;
+ fdb_modified = __fdb_mark_active(fdb);
+ }
/* fastpath: update of existing entry */
if (unlikely(source != fdb->dst &&
@@ -587,8 +608,7 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
clear_bit(BR_FDB_ADDED_BY_EXT_LEARN,
&fdb->flags);
}
- if (now != fdb->updated)
- fdb->updated = now;
+
if (unlikely(test_bit(BR_FDB_ADDED_BY_USER, &flags)))
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
if (unlikely(fdb_modified)) {
@@ -667,6 +687,23 @@ static int fdb_fill_info(struct sk_buff *skb, const struct
net_bridge *br,
&fdb->key.vlan_id))
goto nla_put_failure;
+ if (test_bit(BR_FDB_NOTIFY, &fdb->flags)) {
+ struct nlattr *nest = nla_nest_start(skb, NDA_FDB_EXT_ATTRS);
+ u8 notify_bits = FDB_NOTIFY_BIT;
+
+ if (!nest)
+ goto nla_put_failure;
+ if (test_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags))
+ notify_bits |= FDB_NOTIFY_INACTIVE_BIT;
+
+ if (nla_put_u8(skb, NFEA_ACTIVITY_NOTIFY, notify_bits)) {
+ nla_nest_cancel(skb, nest);
+ goto nla_put_failure;
+ }
+
+ nla_nest_end(skb, nest);
+ }
+
nlmsg_end(skb, nlh);
return 0;
@@ -681,7 +718,9 @@ static inline size_t fdb_nlmsg_size(void)
+ nla_total_size(ETH_ALEN) /* NDA_LLADDR */
+ nla_total_size(sizeof(u32)) /* NDA_MASTER */
+ nla_total_size(sizeof(u16)) /* NDA_VLAN */
- + nla_total_size(sizeof(struct nda_cacheinfo));
+ + nla_total_size(sizeof(struct nda_cacheinfo))
+ + nla_total_size(0) /* NDA_FDB_EXT_ATTRS */
+ + nla_total_size(sizeof(u8)); /* NFEA_ACTIVITY_NOTIFY */
}
static void fdb_notify(struct net_bridge *br,
@@ -791,14 +830,40 @@ int br_fdb_get(struct sk_buff *skb,
return err;
}
+/* returns true if the fdb is modified */
+static bool fdb_handle_notify(struct net_bridge_fdb_entry *fdb, u8 notify)
+{
+ bool modified = false;
+
+ /* allow to mark an entry as inactive, usually done on creation */
+ if ((notify & FDB_NOTIFY_INACTIVE_BIT) &&
+ !test_and_set_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags))
+ modified = true;
+
+ if ((notify & FDB_NOTIFY_BIT) &&
+ !test_and_set_bit(BR_FDB_NOTIFY, &fdb->flags)) {
+ /* enabled activity tracking */
+ modified = true;
+ } else if (!(notify & FDB_NOTIFY_BIT) &&
+ test_and_clear_bit(BR_FDB_NOTIFY, &fdb->flags)) {
+ /* disabled activity tracking, clear notify state */
+ clear_bit(BR_FDB_NOTIFY_INACTIVE, &fdb->flags);
+ modified = true;
+ }
+
+ return modified;
+}
+
/* Update (create or replace) forwarding database entry */
static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
- const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid)
+ const u8 *addr, struct ndmsg *ndm, u16 flags, u16 vid,
+ struct nlattr *nfea_tb[])
{
bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
struct net_bridge_fdb_entry *fdb;
u16 state = ndm->ndm_state;
bool modified = false;
+ u8 notify = 0;
/* If the port cannot learn allow only local and static entries */
if (source && !(state & NUD_PERMANENT) && !(state &
NUD_NOARP) &&
@@ -815,6 +880,13 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
if (is_sticky && (state & NUD_PERMANENT))
return -EINVAL;
+ if (nfea_tb[NFEA_ACTIVITY_NOTIFY]) {
+ notify = nla_get_u8(nfea_tb[NFEA_ACTIVITY_NOTIFY]);
+ if ((notify & ~BR_FDB_NOTIFY_SETTABLE_BITS) ||
+ (notify & BR_FDB_NOTIFY_SETTABLE_BITS) == FDB_NOTIFY_INACTIVE_BIT)
+ return -EINVAL;
+ }
+
fdb = br_fdb_find(br, addr, vid);
if (fdb == NULL) {
if (!(flags & NLM_F_CREATE))
@@ -858,6 +930,9 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
modified = true;
}
+ if (fdb_handle_notify(fdb, notify))
+ modified = true;
+
set_bit(BR_FDB_ADDED_BY_USER, &fdb->flags);
fdb->used = jiffies;
@@ -871,7 +946,7 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
struct net_bridge_port *p, const unsigned char *addr,
- u16 nlh_flags, u16 vid)
+ u16 nlh_flags, u16 vid, struct nlattr *nfea_tb[])
{
int err = 0;
@@ -893,19 +968,24 @@ static int __br_fdb_add(struct ndmsg *ndm, struct
net_bridge *br,
err = br_fdb_external_learn_add(br, p, addr, vid, true);
} else {
spin_lock_bh(&br->hash_lock);
- err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid);
+ err = fdb_add_entry(br, p, addr, ndm, nlh_flags, vid, nfea_tb);
spin_unlock_bh(&br->hash_lock);
}
return err;
}
+static const struct nla_policy br_nda_fdb_pol[NFEA_MAX + 1] = {
+ [NFEA_ACTIVITY_NOTIFY] = { .type = NLA_U8 },
+};
+
/* Add new permanent fdb entry with RTM_NEWNEIGH */
int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
struct net_device *dev,
const unsigned char *addr, u16 vid, u16 nlh_flags,
struct netlink_ext_ack *extack)
{
+ struct nlattr *nfea_tb[NFEA_MAX + 1], *attr;
struct net_bridge_vlan_group *vg;
struct net_bridge_port *p = NULL;
struct net_bridge_vlan *v;
@@ -938,6 +1018,16 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
vg = nbp_vlan_group(p);
}
+ if (tb[NDA_FDB_EXT_ATTRS]) {
+ attr = tb[NDA_FDB_EXT_ATTRS];
+ err = nla_parse_nested(nfea_tb, NFEA_MAX, attr,
+ br_nda_fdb_pol, extack);
+ if (err)
+ return err;
+ } else {
+ memset(nfea_tb, 0, sizeof(struct nlattr *) * (NFEA_MAX + 1));
+ }
+
if (vid) {
v = br_vlan_find(vg, vid);
if (!v || !br_vlan_should_use(v)) {
@@ -946,9 +1036,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
}
/* VID was specified, so use it. */
- err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid);
+ err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid, nfea_tb);
} else {
- err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0);
+ err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0, nfea_tb);
if (err || !vg || !vg->num_vlans)
goto out;
@@ -959,7 +1049,8 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
list_for_each_entry(v, &vg->vlan_list, vlist) {
if (!br_vlan_should_use(v))
continue;
- err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid);
+ err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid,
+ nfea_tb);
if (err)
goto out;
}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 7501be4eeba0..c0ae639e1b36 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -48,6 +48,8 @@ enum {
/* Path to usermode spanning tree program */
#define BR_STP_PROG "/sbin/bridge-stp"
+#define BR_FDB_NOTIFY_SETTABLE_BITS (FDB_NOTIFY_BIT | FDB_NOTIFY_INACTIVE_BIT)
+
typedef struct bridge_id bridge_id;
typedef struct mac_addr mac_addr;
typedef __u16 port_id;
@@ -184,6 +186,8 @@ enum {
BR_FDB_ADDED_BY_USER,
BR_FDB_ADDED_BY_EXT_LEARN,
BR_FDB_OFFLOADED,
+ BR_FDB_NOTIFY,
+ BR_FDB_NOTIFY_INACTIVE
};
struct net_bridge_fdb_key {
--
2.25.4
Nikolay Aleksandrov
2020-Jun-23 20:47 UTC
[Bridge] [PATCH net-next 4/4] net: bridge: add a flag to avoid refreshing fdb when changing/adding
When we modify or create a new fdb entry sometimes we want to avoid
refreshing its activity in order to track it properly. One example is
when a mac is received from EVPN multi-homing peer by FRR, which doesn't
want to change local activity accounting. It makes it static and sets a
flag to track its activity.
Signed-off-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>
---
include/uapi/linux/neighbour.h | 1 +
net/bridge/br_fdb.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 21e569297355..dc8b72201f6c 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -191,6 +191,7 @@ enum {
enum {
NFEA_UNSPEC,
NFEA_ACTIVITY_NOTIFY,
+ NFEA_DONT_REFRESH,
__NFEA_MAX
};
#define NFEA_MAX (__NFEA_MAX - 1)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index 642deb57c064..9db504baa094 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -860,6 +860,7 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
struct nlattr *nfea_tb[])
{
bool is_sticky = !!(ndm->ndm_flags & NTF_STICKY);
+ bool refresh = !nfea_tb[NFEA_DONT_REFRESH];
struct net_bridge_fdb_entry *fdb;
u16 state = ndm->ndm_state;
bool modified = false;
@@ -937,7 +938,8 @@ static int fdb_add_entry(struct net_bridge *br, struct
net_bridge_port *source,
fdb->used = jiffies;
if (modified) {
- fdb->updated = jiffies;
+ if (refresh)
+ fdb->updated = jiffies;
fdb_notify(br, fdb, RTM_NEWNEIGH, true);
}
@@ -977,6 +979,7 @@ static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge
*br,
static const struct nla_policy br_nda_fdb_pol[NFEA_MAX + 1] = {
[NFEA_ACTIVITY_NOTIFY] = { .type = NLA_U8 },
+ [NFEA_DONT_REFRESH] = { .type = NLA_FLAG },
};
/* Add new permanent fdb entry with RTM_NEWNEIGH */
--
2.25.4
David Miller
2020-Jun-24 21:37 UTC
[Bridge] [PATCH net-next 0/4] net: bridge: fdb activity tracking
From: Nikolay Aleksandrov <nikolay at cumulusnetworks.com> Date: Tue, 23 Jun 2020 23:47:14 +0300> This set adds extensions needed for EVPN multi-homing proper and > efficient mac sync. User-space (e.g. FRR) needs to be able to track > non-dynamic entry activity on per-fdb basis depending if a tracked fdb is > currently peer active or locally active and needs to be able to add new > peer active fdb (static + track + inactive) without refreshing it to get > real activity tracking. Patch 02 adds a new NDA attribute - NDA_FDB_EXT_ATTRS > to avoid future pollution of NDA attributes by bridge or vxlan. New > bridge/vxlan specific fdb attributes are embedded in NDA_FDB_EXT_ATTRS, > which is used in patch 03 to pass the new NFEA_ACTIVITY_NOTIFY attribute > which controls if an fdb should be tracked and also reflects its current > state when dumping. It is treated as a bitfield, current valid bits are: > 1 - mark an entry for activity tracking > 2 - mark an entry as inactive to avoid multiple notifications and > reflect state properly > > Patch 04 adds the ability to avoid refreshing an entry when changing it > via the NFEA_DONT_REFRESH flag. That allows user-space to mark a static > entry for tracking and keep its real activity unchanged. > The set has been extensively tested with FRR and those changes will > be upstreamed if/after it gets accepted.Series applied, thanks.