thr3ads.net - Linux Ethernet Bridging - [Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb

If this information is useful, please help other people find it:
Share via:

Roopa Prabhu

2017-Aug-31 05:18 UTC

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

From: Roopa Prabhu <roopa at cumulusnetworks.com>

This extends bridge fdb table tracepoints to also cover
learned fdb entries in the br_fdb_update path. Note that
unlike other tracepoints I have moved this to when the fdb
is modified because this is in the datapath and can generate
a lot of noise in the trace output. br_fdb_update is also called
from added_by_user context in the NTF_USE case which is already
traced ..hence the !added_by_user check.

Signed-off-by: Roopa Prabhu <roopa at cumulusnetworks.com>
---
 include/trace/events/bridge.h | 31 +++++++++++++++++++++++++++++++
 net/bridge/br_fdb.c           |  5 ++++-
 net/core/net-traces.c         |  1 +
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h
index 0f1cde0..1bee3e7 100644
--- a/include/trace/events/bridge.h
+++ b/include/trace/events/bridge.h
@@ -92,6 +92,37 @@ TRACE_EVENT(fdb_delete,
 		  __entry->addr[4], __entry->addr[5], __entry->vid)
 );
 
+TRACE_EVENT(br_fdb_update,
+
+	TP_PROTO(struct net_bridge *br, struct net_bridge_port *source,
+		 const unsigned char *addr, u16 vid, bool added_by_user),
+
+	TP_ARGS(br, source, addr, vid, added_by_user),
+
+	TP_STRUCT__entry(
+		__string(br_dev, br->dev->name)
+		__string(dev, source->dev->name)
+		__array(unsigned char, addr, ETH_ALEN)
+		__field(u16, vid)
+		__field(bool, added_by_user)
+	),
+
+	TP_fast_assign(
+		__assign_str(br_dev, br->dev->name);
+		__assign_str(dev, source->dev->name);
+		memcpy(__entry->addr, addr, ETH_ALEN);
+		__entry->vid = vid;
+		__entry->added_by_user = added_by_user;
+	),
+
+	TP_printk("br_dev %s source %s addr %02x:%02x:%02x:%02x:%02x:%02x vid %u
added_by_user %d",
+		  __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,
+		  __entry->added_by_user)
+);
+
+
 #endif /* _TRACE_BRIDGE_H */
 
 /* This part must be outside protection */
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index be5e1da..4ea5c8b 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -583,8 +583,10 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
 				fdb->updated = now;
 			if (unlikely(added_by_user))
 				fdb->added_by_user = 1;
-			if (unlikely(fdb_modified))
+			if (unlikely(fdb_modified)) {
+				trace_br_fdb_update(br, source, addr, vid, added_by_user);
 				fdb_notify(br, fdb, RTM_NEWNEIGH);
+			}
 		}
 	} else {
 		spin_lock(&br->hash_lock);
@@ -593,6 +595,7 @@ void br_fdb_update(struct net_bridge *br, struct
net_bridge_port *source,
 			if (fdb) {
 				if (unlikely(added_by_user))
 					fdb->added_by_user = 1;
+				trace_br_fdb_update(br, source, addr, vid, added_by_user);
 				fdb_notify(br, fdb, RTM_NEWNEIGH);
 			}
 		}
diff --git a/net/core/net-traces.c b/net/core/net-traces.c
index 4a0292c..1132820 100644
--- a/net/core/net-traces.c
+++ b/net/core/net-traces.c
@@ -42,6 +42,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(fib6_table_lookup);
 EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_add);
 EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_external_learn_add);
 EXPORT_TRACEPOINT_SYMBOL_GPL(fdb_delete);
+EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_update);
 #endif
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(kfree_skb);
-- 
2.1.4

Jesper Dangaard Brouer

2017-Aug-31 12:38 UTC

head link

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

On Wed, 30 Aug 2017 22:18:13 -0700
Roopa Prabhu <roopa at cumulusnetworks.com> wrote:
> From: Roopa Prabhu <roopa at cumulusnetworks.com>
> 
> This extends bridge fdb table tracepoints to also cover
> learned fdb entries in the br_fdb_update path. Note that
> unlike other tracepoints I have moved this to when the fdb
> is modified because this is in the datapath and can generate
> a lot of noise in the trace output. br_fdb_update is also called
> from added_by_user context in the NTF_USE case which is already
> traced ..hence the !added_by_user check.
> 
> Signed-off-by: Roopa Prabhu <roopa at cumulusnetworks.com>
> ---
>  include/trace/events/bridge.h | 31 +++++++++++++++++++++++++++++++
>  net/bridge/br_fdb.c           |  5 ++++-
>  net/core/net-traces.c         |  1 +
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h
> index 0f1cde0..1bee3e7 100644
> --- a/include/trace/events/bridge.h
> +++ b/include/trace/events/bridge.h
> @@ -92,6 +92,37 @@ TRACE_EVENT(fdb_delete,
>  		  __entry->addr[4], __entry->addr[5], __entry->vid)
>  );
>  
> +TRACE_EVENT(br_fdb_update,
> +
> +	TP_PROTO(struct net_bridge *br, struct net_bridge_port *source,
> +		 const unsigned char *addr, u16 vid, bool added_by_user),
> +
> +	TP_ARGS(br, source, addr, vid, added_by_user),
> +
> +	TP_STRUCT__entry(
> +		__string(br_dev, br->dev->name)
> +		__string(dev, source->dev->name)
I have found that using the device string name is 

(1) slow as it involves strcpy+strlen

 See [1]+[2] where a single dev-name costed me 16 ns, and the base
 overhead of a bpf attached tracepoint is 25 ns (see [3]).

 [1] https://git.kernel.org/davem/net-next/c/e7d12ce121a
 [2] https://git.kernel.org/davem/net-next/c/315ec3990ef
 [3] https://git.kernel.org/davem/net-next/c/25d4dae1a64

(2) strings are also harder to work-with/extract when attaching a bpf_prog

See the trouble I'm in accessing a dev string here napi:napi_poll here:
 https://github.com/netoptimizer/prototype-kernel/blob/103b955a080/kernel/samples/bpf/napi_monitor_kern.c#L52-L58

Using ifindex'es in userspace is fairly easy see man if_indextoname(3).
> +		__array(unsigned char, addr, ETH_ALEN)
> +		__field(u16, vid)
> +		__field(bool, added_by_user)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(br_dev, br->dev->name);
> +		__assign_str(dev, source->dev->name);
> +		memcpy(__entry->addr, addr, ETH_ALEN);
> +		__entry->vid = vid;
> +		__entry->added_by_user = added_by_user;
> +	),
> +
> +	TP_printk("br_dev %s source %s addr %02x:%02x:%02x:%02x:%02x:%02x
vid %u added_by_user %d",
> +		  __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,
> +		  __entry->added_by_user)
> +);
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Nikolay Aleksandrov

2017-Aug-31 14:19 UTC

head link

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

On 31/08/17 08:18, Roopa Prabhu wrote:> From: Roopa Prabhu <roopa at cumulusnetworks.com>
> 
> This extends bridge fdb table tracepoints to also cover
> learned fdb entries in the br_fdb_update path. Note that
> unlike other tracepoints I have moved this to when the fdb
> is modified because this is in the datapath and can generate
> a lot of noise in the trace output. br_fdb_update is also called
> from added_by_user context in the NTF_USE case which is already
> traced ..hence the !added_by_user check.
> 
> Signed-off-by: Roopa Prabhu <roopa at cumulusnetworks.com>
> ---
>  include/trace/events/bridge.h | 31 +++++++++++++++++++++++++++++++
>  net/bridge/br_fdb.c           |  5 ++++-
>  net/core/net-traces.c         |  1 +
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
Acked-by: Nikolay Aleksandrov <nikolay at cumulusnetworks.com>

Roopa Prabhu

2017-Aug-31 15:21 UTC

head link

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

On Thu, Aug 31, 2017 at 5:38 AM, Jesper Dangaard Brouer
<brouer at redhat.com> wrote:> On Wed, 30 Aug 2017 22:18:13 -0700
> Roopa Prabhu <roopa at cumulusnetworks.com> wrote:
>
>> From: Roopa Prabhu <roopa at cumulusnetworks.com>
>>
>> This extends bridge fdb table tracepoints to also cover
>> learned fdb entries in the br_fdb_update path. Note that
>> unlike other tracepoints I have moved this to when the fdb
>> is modified because this is in the datapath and can generate
>> a lot of noise in the trace output. br_fdb_update is also called
>> from added_by_user context in the NTF_USE case which is already
>> traced ..hence the !added_by_user check.
>>
>> Signed-off-by: Roopa Prabhu <roopa at cumulusnetworks.com>
>> ---
>>  include/trace/events/bridge.h | 31 +++++++++++++++++++++++++++++++
>>  net/bridge/br_fdb.c           |  5 ++++-
>>  net/core/net-traces.c         |  1 +
>>  3 files changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/trace/events/bridge.h
b/include/trace/events/bridge.h
>> index 0f1cde0..1bee3e7 100644
>> --- a/include/trace/events/bridge.h
>> +++ b/include/trace/events/bridge.h
>> @@ -92,6 +92,37 @@ TRACE_EVENT(fdb_delete,
>>                 __entry->addr[4], __entry->addr[5],
__entry->vid)
>>  );
>>
>> +TRACE_EVENT(br_fdb_update,
>> +
>> +     TP_PROTO(struct net_bridge *br, struct net_bridge_port *source,
>> +              const unsigned char *addr, u16 vid, bool added_by_user),
>> +
>> +     TP_ARGS(br, source, addr, vid, added_by_user),
>> +
>> +     TP_STRUCT__entry(
>> +             __string(br_dev, br->dev->name)
>> +             __string(dev, source->dev->name)
>
> I have found that using the device string name is
>
> (1) slow as it involves strcpy+strlen
>
>  See [1]+[2] where a single dev-name costed me 16 ns, and the base
>  overhead of a bpf attached tracepoint is 25 ns (see [3]).
>
>  [1] https://git.kernel.org/davem/net-next/c/e7d12ce121a
>  [2] https://git.kernel.org/davem/net-next/c/315ec3990ef
>  [3] https://git.kernel.org/davem/net-next/c/25d4dae1a64
>
> (2) strings are also harder to work-with/extract when attaching a bpf_prog
>
> See the trouble I'm in accessing a dev string here napi:napi_poll here:
> 
https://github.com/netoptimizer/prototype-kernel/blob/103b955a080/kernel/samples/bpf/napi_monitor_kern.c#L52-L58
>
> Using ifindex'es in userspace is fairly easy see man if_indextoname(3).
>
Jesper thanks for the data!. GTK. Looking at include/trace/events,
currently almost all tracepoints use dev->name.
These bridge tracepoints in context are primarily for debugging fdb
updates only, not for every packet and hence not in the performance
path.
In large scale deployments with thousands of bridge ports and fdb
entries, dev->name will definately make it easier to trouble-shoot.
So, I did like to leave these with dev->name unless there are strong
objections.

thanks.

David Miller

2017-Aug-31 18:43 UTC

head link

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

From: Roopa Prabhu <roopa at cumulusnetworks.com>
Date: Wed, 30 Aug 2017 22:18:13 -0700
> From: Roopa Prabhu <roopa at cumulusnetworks.com>
> 
> This extends bridge fdb table tracepoints to also cover
> learned fdb entries in the br_fdb_update path. Note that
> unlike other tracepoints I have moved this to when the fdb
> is modified because this is in the datapath and can generate
> a lot of noise in the trace output. br_fdb_update is also called
> from added_by_user context in the NTF_USE case which is already
> traced ..hence the !added_by_user check.
> 
> Signed-off-by: Roopa Prabhu <roopa at cumulusnetworks.com>
Applied.

Let's use dev->name for now and if the tooling can eventually
do transparent ifindex->name then we can consider redoing
a bunch of networking tracepoints.

Thanks.

Linux Ethernet Bridging - Aug 2017 - [Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update

[Bridge] [PATCH net-next] bridge: add tracepoint in br_fdb_update