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

[David Ahern]

On 8/31/17 9:21 AM, Roopa Prabhu wrote:
> 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.

+1 for user friendliness for debugging tracepoints. The device name is
also more user friendly when adding filters to the data collection.

Being able to add bpf everywhere certainly changes the game a bit, but
we should not relinquish ease of use and understanding for the potential
that someone might want to put a bpf program on the tracepoint and want
to maintain high performance.