| Message ID | 1503869593-30165-1-git-send-email-roopa@cumulusnetworks.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] bridge: fdb add and delete tracepoints | expand |
On 08/27/2017 02:33 PM, Roopa Prabhu wrote: > From: Roopa Prabhu <roopa@cumulusnetworks.com> > > Tracepoints to trace bridge forwarding database updates. Thanks for adding this! > > Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> > --- > include/trace/events/bridge.h | 98 +++++++++++++++++++++++++++++++++++++++++++ > net/bridge/br_fdb.c | 7 ++++ > net/core/net-traces.c | 6 +++ > 3 files changed, 111 insertions(+) > create mode 100644 include/trace/events/bridge.h > > diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h > new file mode 100644 > index 0000000..e2d52cf > --- /dev/null > +++ b/include/trace/events/bridge.h > @@ -0,0 +1,98 @@ > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM bridge > + > +#if !defined(_TRACE_BRIDGE_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_BRIDGE_H > + > +#include <linux/netdevice.h> > +#include <linux/tracepoint.h> > + > +#include "../../../net/bridge/br_private.h" > + > +TRACE_EVENT(br_fdb_add, > + > + TP_PROTO(struct ndmsg *ndm, struct net_device *dev, > + const unsigned char *addr, u16 vid, u16 nlh_flags), > + > + TP_ARGS(ndm, dev, addr, vid, nlh_flags), > + > + TP_STRUCT__entry( > + __field(u8, ndm_flags) > + __string(dev, dev->name) > + __array(unsigned char, addr, 6) Can you use ETH_ALEN instead of 6 here? > + __field(u16, vid) > + __field(u16, nlh_flags) > + ), > + > + TP_fast_assign( > + __assign_str(dev, dev->name); > + memcpy(__entry->addr, addr, 6); Likewise > + __entry->vid = vid; > + __entry->nlh_flags = nlh_flags; > + __entry->ndm_flags = ndm->ndm_flags; > + ), > + > + TP_printk("dev %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u nlh_flags %x ndm_flags = %x", I wonder if we could make %pM work for TP_printk() as this would simplify the argument list a bitt. Can you use %04x for vid, nlh_flags and %02x for ndm_flags? > + __get_str(dev), __entry->addr[0], __entry->addr[1], > + __entry->addr[2], __entry->addr[3], __entry->addr[4], > + __entry->addr[5], __entry->vid, > + __entry->nlh_flags, __entry->ndm_flags) > +); > + > +TRACE_EVENT(br_fdb_external_learn_add, > + > + TP_PROTO(struct net_bridge *br, struct net_bridge_port *p, > + const unsigned char *addr, u16 vid), > + > + TP_ARGS(br, p, addr, vid), > + > + TP_STRUCT__entry( > + __string(br_dev, br->dev->name) > + __string(dev, p->dev->name) > + __array(unsigned char, addr, 6) > + __field(u16, vid) > + ), > + > + TP_fast_assign( > + __assign_str(br_dev, br ? br->dev->name : "null"); > + __assign_str(dev, p ? p->dev->name : "null"); > + memcpy(__entry->addr, addr, 6); > + __entry->vid = vid; > + ), > + > + TP_printk("br_dev %s port %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u", > + __get_str(br_dev), __get_str(dev), __entry->addr[0], > + __entry->addr[1], __entry->addr[2], __entry->addr[3], > + __entry->addr[4], __entry->addr[5], __entry->vid) > +); > + > +TRACE_EVENT(fdb_delete, > + > + TP_PROTO(struct net_bridge *br, struct net_bridge_fdb_entry *f), > + > + TP_ARGS(br, f), > + > + TP_STRUCT__entry( > + __string(br_dev, br->dev->name) > + __string(dev, f->dst ? f->dst->dev->name : "null") > + __array(unsigned char, addr, 6) Same here, using ETH_ALEN would be clearer. > + __field(u16, vid) > + ), > + Thanks!
On Sun, Aug 27, 2017 at 7:11 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 08/27/2017 02:33 PM, Roopa Prabhu wrote: >> From: Roopa Prabhu <roopa@cumulusnetworks.com> >> >> Tracepoints to trace bridge forwarding database updates. > > Thanks for adding this! > >> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> --- >> include/trace/events/bridge.h | 98 +++++++++++++++++++++++++++++++++++++++++++ >> net/bridge/br_fdb.c | 7 ++++ >> net/core/net-traces.c | 6 +++ >> 3 files changed, 111 insertions(+) >> create mode 100644 include/trace/events/bridge.h >> >> diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h >> new file mode 100644 >> index 0000000..e2d52cf >> --- /dev/null >> +++ b/include/trace/events/bridge.h >> @@ -0,0 +1,98 @@ >> +#undef TRACE_SYSTEM >> +#define TRACE_SYSTEM bridge >> + >> +#if !defined(_TRACE_BRIDGE_H) || defined(TRACE_HEADER_MULTI_READ) >> +#define _TRACE_BRIDGE_H >> + >> +#include <linux/netdevice.h> >> +#include <linux/tracepoint.h> >> + >> +#include "../../../net/bridge/br_private.h" >> + >> +TRACE_EVENT(br_fdb_add, >> + >> + TP_PROTO(struct ndmsg *ndm, struct net_device *dev, >> + const unsigned char *addr, u16 vid, u16 nlh_flags), >> + >> + TP_ARGS(ndm, dev, addr, vid, nlh_flags), >> + >> + TP_STRUCT__entry( >> + __field(u8, ndm_flags) >> + __string(dev, dev->name) >> + __array(unsigned char, addr, 6) > > Can you use ETH_ALEN instead of 6 here? > >> + __field(u16, vid) >> + __field(u16, nlh_flags) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(dev, dev->name); >> + memcpy(__entry->addr, addr, 6); > > Likewise will do. > >> + __entry->vid = vid; >> + __entry->nlh_flags = nlh_flags; >> + __entry->ndm_flags = ndm->ndm_flags; >> + ), >> + >> + TP_printk("dev %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u nlh_flags %x ndm_flags = %x", > > I wonder if we could make %pM work for TP_printk() as this would > simplify the argument list a bitt. yeah i struggled with getting %pM to work here. > Can you use %04x for vid, nlh_flags > and %02x for ndm_flags? will do, > >> + __get_str(dev), __entry->addr[0], __entry->addr[1], >> + __entry->addr[2], __entry->addr[3], __entry->addr[4], >> + __entry->addr[5], __entry->vid, >> + __entry->nlh_flags, __entry->ndm_flags) >> +); >> + >> +TRACE_EVENT(br_fdb_external_learn_add, >> + >> + TP_PROTO(struct net_bridge *br, struct net_bridge_port *p, >> + const unsigned char *addr, u16 vid), >> + >> + TP_ARGS(br, p, addr, vid), >> + >> + TP_STRUCT__entry( >> + __string(br_dev, br->dev->name) >> + __string(dev, p->dev->name) >> + __array(unsigned char, addr, 6) >> + __field(u16, vid) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(br_dev, br ? br->dev->name : "null"); >> + __assign_str(dev, p ? p->dev->name : "null"); >> + memcpy(__entry->addr, addr, 6); >> + __entry->vid = vid; >> + ), >> + >> + TP_printk("br_dev %s port %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u", >> + __get_str(br_dev), __get_str(dev), __entry->addr[0], >> + __entry->addr[1], __entry->addr[2], __entry->addr[3], >> + __entry->addr[4], __entry->addr[5], __entry->vid) >> +); >> + >> +TRACE_EVENT(fdb_delete, >> + >> + TP_PROTO(struct net_bridge *br, struct net_bridge_fdb_entry *f), >> + >> + TP_ARGS(br, f), >> + >> + TP_STRUCT__entry( >> + __string(br_dev, br->dev->name) >> + __string(dev, f->dst ? f->dst->dev->name : "null") >> + __array(unsigned char, addr, 6) > > Same here, using ETH_ALEN would be clearer. > ack, thanks for the review.
On Sun, Aug 27, 2017 at 7:11 PM, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 08/27/2017 02:33 PM, Roopa Prabhu wrote: >> From: Roopa Prabhu <roopa@cumulusnetworks.com> >> >> Tracepoints to trace bridge forwarding database updates. > > Thanks for adding this! > >> >> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com> >> --- >> include/trace/events/bridge.h | 98 +++++++++++++++++++++++++++++++++++++++++++ >> net/bridge/br_fdb.c | 7 ++++ >> net/core/net-traces.c | 6 +++ >> 3 files changed, 111 insertions(+) >> create mode 100644 include/trace/events/bridge.h >> >> diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h >> new file mode 100644 >> index 0000000..e2d52cf >> --- /dev/null >> +++ b/include/trace/events/bridge.h >> @@ -0,0 +1,98 @@ >> +#undef TRACE_SYSTEM >> +#define TRACE_SYSTEM bridge >> + >> +#if !defined(_TRACE_BRIDGE_H) || defined(TRACE_HEADER_MULTI_READ) >> +#define _TRACE_BRIDGE_H >> + >> +#include <linux/netdevice.h> >> +#include <linux/tracepoint.h> >> + >> +#include "../../../net/bridge/br_private.h" >> + >> +TRACE_EVENT(br_fdb_add, >> + >> + TP_PROTO(struct ndmsg *ndm, struct net_device *dev, >> + const unsigned char *addr, u16 vid, u16 nlh_flags), >> + >> + TP_ARGS(ndm, dev, addr, vid, nlh_flags), >> + >> + TP_STRUCT__entry( >> + __field(u8, ndm_flags) >> + __string(dev, dev->name) >> + __array(unsigned char, addr, 6) > > Can you use ETH_ALEN instead of 6 here? > >> + __field(u16, vid) >> + __field(u16, nlh_flags) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(dev, dev->name); >> + memcpy(__entry->addr, addr, 6); > > Likewise > >> + __entry->vid = vid; >> + __entry->nlh_flags = nlh_flags; >> + __entry->ndm_flags = ndm->ndm_flags; >> + ), >> + >> + TP_printk("dev %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u nlh_flags %x ndm_flags = %x", > > I wonder if we could make %pM work for TP_printk() as this would > simplify the argument list a bitt. Can you use %04x for vid, nlh_flags > and %02x for ndm_flags? one thing here.., i think %u is better for vid. bridge driver debug prints also use %u for vid.
diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h new file mode 100644 index 0000000..e2d52cf --- /dev/null +++ b/include/trace/events/bridge.h @@ -0,0 +1,98 @@ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM bridge + +#if !defined(_TRACE_BRIDGE_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_BRIDGE_H + +#include <linux/netdevice.h> +#include <linux/tracepoint.h> + +#include "../../../net/bridge/br_private.h" + +TRACE_EVENT(br_fdb_add, + + TP_PROTO(struct ndmsg *ndm, struct net_device *dev, + const unsigned char *addr, u16 vid, u16 nlh_flags), + + TP_ARGS(ndm, dev, addr, vid, nlh_flags), + + TP_STRUCT__entry( + __field(u8, ndm_flags) + __string(dev, dev->name) + __array(unsigned char, addr, 6) + __field(u16, vid) + __field(u16, nlh_flags) + ), + + TP_fast_assign( + __assign_str(dev, dev->name); + memcpy(__entry->addr, addr, 6); + __entry->vid = vid; + __entry->nlh_flags = nlh_flags; + __entry->ndm_flags = ndm->ndm_flags; + ), + + TP_printk("dev %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u nlh_flags %x ndm_flags = %x", + __get_str(dev), __entry->addr[0], __entry->addr[1], + __entry->addr[2], __entry->addr[3], __entry->addr[4], + __entry->addr[5], __entry->vid, + __entry->nlh_flags, __entry->ndm_flags) +); + +TRACE_EVENT(br_fdb_external_learn_add, + + TP_PROTO(struct net_bridge *br, struct net_bridge_port *p, + const unsigned char *addr, u16 vid), + + TP_ARGS(br, p, addr, vid), + + TP_STRUCT__entry( + __string(br_dev, br->dev->name) + __string(dev, p->dev->name) + __array(unsigned char, addr, 6) + __field(u16, vid) + ), + + TP_fast_assign( + __assign_str(br_dev, br ? br->dev->name : "null"); + __assign_str(dev, p ? p->dev->name : "null"); + memcpy(__entry->addr, addr, 6); + __entry->vid = vid; + ), + + TP_printk("br_dev %s port %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u", + __get_str(br_dev), __get_str(dev), __entry->addr[0], + __entry->addr[1], __entry->addr[2], __entry->addr[3], + __entry->addr[4], __entry->addr[5], __entry->vid) +); + +TRACE_EVENT(fdb_delete, + + TP_PROTO(struct net_bridge *br, struct net_bridge_fdb_entry *f), + + TP_ARGS(br, f), + + TP_STRUCT__entry( + __string(br_dev, br->dev->name) + __string(dev, f->dst ? f->dst->dev->name : "null") + __array(unsigned char, addr, 6) + __field(u16, vid) + ), + + TP_fast_assign( + __assign_str(br_dev, br ? br->dev->name : "null"); + __assign_str(dev, f->dst ? f->dst->dev->name : "null"); + memcpy(__entry->addr, f->addr.addr, 6); + __entry->vid = f->vlan_id; + ), + + TP_printk("br_dev %s dev %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u", + __get_str(br_dev), __get_str(dev), __entry->addr[0], + __entry->addr[1], __entry->addr[2], __entry->addr[3], + __entry->addr[4], __entry->addr[5], __entry->vid) +); + +#endif /* _TRACE_BRIDGE_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c index a79b648..be5e1da 100644 --- a/net/bridge/br_fdb.c +++ b/net/bridge/br_fdb.c @@ -25,6 +25,7 @@ #include <asm/unaligned.h> #include <linux/if_vlan.h> #include <net/switchdev.h> +#include <trace/events/bridge.h> #include "br_private.h" static struct kmem_cache *br_fdb_cache __read_mostly; @@ -171,6 +172,8 @@ static void fdb_del_hw_addr(struct net_bridge *br, const unsigned char *addr) static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f) { + trace_fdb_delete(br, f); + if (f->is_static) fdb_del_hw_addr(br, f->addr.addr); @@ -870,6 +873,8 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[], struct net_bridge *br = NULL; int err = 0; + trace_br_fdb_add(ndm, dev, addr, vid, nlh_flags); + if (!(ndm->ndm_state & (NUD_PERMANENT|NUD_NOARP|NUD_REACHABLE))) { pr_info("bridge: RTM_NEWNEIGH with invalid state %#x\n", ndm->ndm_state); return -EINVAL; @@ -1066,6 +1071,8 @@ int br_fdb_external_learn_add(struct net_bridge *br, struct net_bridge_port *p, bool modified = false; int err = 0; + trace_br_fdb_external_learn_add(br, p, addr, vid); + spin_lock_bh(&br->hash_lock); head = &br->hash[br_mac_hash(addr, vid)]; diff --git a/net/core/net-traces.c b/net/core/net-traces.c index 4f1468c..4a0292c 100644 --- a/net/core/net-traces.c +++ b/net/core/net-traces.c @@ -37,6 +37,12 @@ #include <trace/events/fib6.h> EXPORT_TRACEPOINT_SYMBOL_GPL(fib6_table_lookup); #endif +#if IS_ENABLED(CONFIG_BRIDGE) +#include <trace/events/bridge.h> +EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_add); +EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_external_learn_add); +EXPORT_TRACEPOINT_SYMBOL_GPL(fdb_delete); +#endif EXPORT_TRACEPOINT_SYMBOL_GPL(kfree_skb);