Re: [RFC net-next] openvswitch: add trace points

From: Aaron Conole
Date: Mon Jun 21 2021 - 08:00:07 EST

Next message: Mark Brown: "Re: [PATCH v1 1/1] spi: Remove unneeded parentheses in spi_map_buf()"
Previous message: Robin Murphy: "Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()"
In reply to: Dumitru Ceara: "Re: [RFC net-next] openvswitch: add trace points"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dumitru Ceara <dceara@xxxxxxxxxx> writes:

> On 5/27/21 9:15 PM, Aaron Conole wrote:
>> This makes openvswitch module use the event tracing framework
>> to log the upcall interface and action execution pipeline. When
>> using openvswitch as the packet forwarding engine, some types of
>> debugging are made possible simply by using the ovs-vswitchd's
>> ofproto/trace command. However, such a command has some
>> limitations:
>>
>> 1. When trying to trace packets that go through the CT action,
>> the state of the packet can't be determined, and probably
>> would be potentially wrong.
>>
>> 2. Deducing problem packets can sometimes be difficult as well
>> even if many of the flows are known
>>
>> 3. It's possible to use the openvswitch module even without
>> the ovs-vswitchd (although, not common use).
>>
>> Introduce the event tracing points here to make it possible for
>> working through these problems in kernel space. The style is
>> copied from the mac80211 driver-trace / trace code for
>> consistency.
>>
>> Signed-off-by: Aaron Conole <aconole@xxxxxxxxxx>
>> ---
>
> Hi Aaron,

Hi Dumitru,

>> net/openvswitch/Makefile | 3 +
>> net/openvswitch/actions.c | 4 +
>> net/openvswitch/datapath.c | 7 ++
>> net/openvswitch/openvswitch_trace.c | 10 ++
>> net/openvswitch/openvswitch_trace.h | 152 ++++++++++++++++++++++++++++
>> 5 files changed, 176 insertions(+)
>> create mode 100644 net/openvswitch/openvswitch_trace.c
>> create mode 100644 net/openvswitch/openvswitch_trace.h
>>
>> diff --git a/net/openvswitch/Makefile b/net/openvswitch/Makefile
>> index 41109c326f3a..28982630bef3 100644
>> --- a/net/openvswitch/Makefile
>> +++ b/net/openvswitch/Makefile
>> @@ -13,6 +13,7 @@ openvswitch-y := \
>> flow_netlink.o \
>> flow_table.o \
>> meter.o \
>> + openvswitch_trace.o \
>> vport.o \
>> vport-internal_dev.o \
>> vport-netdev.o
>> @@ -24,3 +25,5 @@ endif
>> obj-$(CONFIG_OPENVSWITCH_VXLAN)+= vport-vxlan.o
>> obj-$(CONFIG_OPENVSWITCH_GENEVE)+= vport-geneve.o
>> obj-$(CONFIG_OPENVSWITCH_GRE) += vport-gre.o
>> +
>> +CFLAGS_openvswitch_trace.o = -I$(src)
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index d858ea580e43..62285453ca79 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -30,6 +30,7 @@
>> #include "conntrack.h"
>> #include "vport.h"
>> #include "flow_netlink.h"
>> +#include "openvswitch_trace.h"
>>
>> struct deferred_action {
>> struct sk_buff *skb;
>> @@ -1242,6 +1243,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> a = nla_next(a, &rem)) {
>> int err = 0;
>>
>> + if (trace_openvswitch_probe_action_enabled())
>> + trace_openvswitch_probe_action(dp, skb, key, a, rem);
>> +
>> switch (nla_type(a)) {
>> case OVS_ACTION_ATTR_OUTPUT: {
>> int port = nla_get_u32(a);
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index 9d6ef6cb9b26..63f19a6ed472 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -43,6 +43,7 @@
>> #include "flow_table.h"
>> #include "flow_netlink.h"
>> #include "meter.h"
>> +#include "openvswitch_trace.h"
>> #include "vport-internal_dev.h"
>> #include "vport-netdev.h"
>>
>> @@ -275,6 +276,12 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
>> struct dp_stats_percpu *stats;
>> int err;
>>
>> + if (trace_openvswitch_probe_userspace_enabled()) {
>> + struct sw_flow_key ukey = *key;
>> +
>> + trace_openvswitch_probe_userspace(dp, skb, &ukey, upcall_info);
>> + }
>> +
>> if (upcall_info->portid == 0) {
>> err = -ENOTCONN;
>> goto err;
>> diff --git a/net/openvswitch/openvswitch_trace.c b/net/openvswitch/openvswitch_trace.c
>> new file mode 100644
>> index 000000000000..62c5f7d6f023
>> --- /dev/null
>> +++ b/net/openvswitch/openvswitch_trace.c
>> @@ -0,0 +1,10 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* bug in tracepoint.h, it should include this */
>> +#include <linux/module.h>
>> +
>> +/* sparse isn't too happy with all macros... */
>> +#ifndef __CHECKER__
>> +#define CREATE_TRACE_POINTS
>> +#include "openvswitch_trace.h"
>> +
>> +#endif
>> diff --git a/net/openvswitch/openvswitch_trace.h b/net/openvswitch/openvswitch_trace.h
>> new file mode 100644
>> index 000000000000..1b350306f622
>> --- /dev/null
>> +++ b/net/openvswitch/openvswitch_trace.h
>> @@ -0,0 +1,152 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM openvswitch
>> +
>> +#if !defined(_TRACE_OPENVSWITCH_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_OPENVSWITCH_H
>> +
>> +#include <linux/tracepoint.h>
>> +
>> +#include "datapath.h"
>> +
>> +TRACE_EVENT(openvswitch_probe_action,
>> +
>> + TP_PROTO(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_key *key, const struct nlattr *a, int rem),
>> +
>> + TP_ARGS(dp, skb, key, a, rem),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, dpaddr )
>> + __string( dp_name, ovs_dp_name(dp) )
>> + __string( dev_name, skb->dev->name )
>> + __field( void *, skbaddr )
>> + __field( unsigned int, len )
>> + __field( unsigned int, data_len )
>> + __field( unsigned int, truesize )
>> + __field( u8, nr_frags )
>> + __field( u16, gso_size )
>> + __field( u16, gso_type )
>> + __field( u32, ovs_flow_hash )
>> + __field( u32, recirc_id )
>> + __field( void *, keyaddr )
>> + __field( u16, key_eth_type )
>> + __field( u8, key_ct_state )
>> + __field( u8, key_ct_orig_proto )
>
> I think tracing ct_zone would be useful too.

ACK, I'll add it. I will post a new version today.

Thanks for the review!

>> + __field( unsigned int, flow_key_valid )
>> + __field( u8, action_type )
>> + __field( unsigned int, action_len )
>> + __field( void *, action_data )
>> + __field( u8, is_last )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->dpaddr = dp;
>> + __assign_str(dp_name, ovs_dp_name(dp));
>> + __assign_str(dev_name, skb->dev->name);
>> + __entry->skbaddr = skb;
>> + __entry->len = skb->len;
>> + __entry->data_len = skb->data_len;
>> + __entry->truesize = skb->truesize;
>> + __entry->nr_frags = skb_shinfo(skb)->nr_frags;
>> + __entry->gso_size = skb_shinfo(skb)->gso_size;
>> + __entry->gso_type = skb_shinfo(skb)->gso_type;
>> + __entry->ovs_flow_hash = key->ovs_flow_hash;
>> + __entry->recirc_id = key->recirc_id;
>> + __entry->keyaddr = key;
>> + __entry->key_eth_type = key->eth.type;
>> + __entry->key_ct_state = key->ct_state;
>> + __entry->key_ct_orig_proto = key->ct_orig_proto;
>> + __entry->flow_key_valid = !(key->mac_proto & SW_FLOW_KEY_INVALID);
>> + __entry->action_type = nla_type(a);
>> + __entry->action_len = nla_len(a);
>> + __entry->action_data = nla_data(a);
>> + __entry->is_last = nla_is_last(a, rem);
>> + ),
>> +
>> + TP_printk("dpaddr=%p dp_name=%s dev=%s skbaddr=%p len=%u
>> data_len=%u truesize=%u nr_frags=%d gso_size=%d gso_type=%#x
>> ovs_flow_hash=0x%08x recirc_id=0x%08x keyaddr=%p eth_type=0x%04x
>> ct_state=%02x ct_orig_proto=%02x flow_key_valid=%d action_type=%u
>> action_len=%u action_data=%p is_last=%d",
>> + __entry->dpaddr, __get_str(dp_name), __get_str(dev_name),
>> + __entry->skbaddr, __entry->len, __entry->data_len,
>> + __entry->truesize, __entry->nr_frags, __entry->gso_size,
>> + __entry->gso_type, __entry->ovs_flow_hash,
>> + __entry->recirc_id, __entry->keyaddr, __entry->key_eth_type,
>> + __entry->key_ct_state, __entry->key_ct_orig_proto,
>> + __entry->flow_key_valid,
>> + __entry->action_type, __entry->action_len,
>> + __entry->action_data, __entry->is_last)
>> +);
>> +
>> +TRACE_EVENT(openvswitch_probe_userspace,
>> +
>> + TP_PROTO(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_key *key,
>> + const struct dp_upcall_info *upcall_info),
>> +
>> + TP_ARGS(dp, skb, key, upcall_info),
>> +
>> + TP_STRUCT__entry(
>> + __field( void *, dpaddr )
>> + __string( dp_name, ovs_dp_name(dp) )
>> + __string( dev_name, skb->dev->name )
>> + __field( void *, skbaddr )
>> + __field( unsigned int, len )
>> + __field( unsigned int, data_len )
>> + __field( unsigned int, truesize )
>> + __field( u8, nr_frags )
>> + __field( u16, gso_size )
>> + __field( u16, gso_type )
>> + __field( u32, ovs_flow_hash )
>> + __field( u32, recirc_id )
>> + __field( void *, keyaddr )
>> + __field( u16, key_eth_type )
>> + __field( u8, key_ct_state )
>> + __field( u8, key_ct_orig_proto )
>
> Same here.
>
> Thanks,
> Dumitru
>
>> + __field( unsigned int, flow_key_valid )
>> + __field( u8, upcall_cmd )
>> + __field( u32, upcall_port )
>> + __field( u16, upcall_mru )
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->dpaddr = dp;
>> + __assign_str(dp_name, ovs_dp_name(dp));
>> + __assign_str(dev_name, skb->dev->name);
>> + __entry->skbaddr = skb;
>> + __entry->len = skb->len;
>> + __entry->data_len = skb->data_len;
>> + __entry->truesize = skb->truesize;
>> + __entry->nr_frags = skb_shinfo(skb)->nr_frags;
>> + __entry->gso_size = skb_shinfo(skb)->gso_size;
>> + __entry->gso_type = skb_shinfo(skb)->gso_type;
>> + __entry->ovs_flow_hash = key->ovs_flow_hash;
>> + __entry->recirc_id = key->recirc_id;
>> + __entry->keyaddr = key;
>> + __entry->key_eth_type = key->eth.type;
>> + __entry->key_ct_state = key->ct_state;
>> + __entry->key_ct_orig_proto = key->ct_orig_proto;
>> + __entry->flow_key_valid = !(key->mac_proto & SW_FLOW_KEY_INVALID);
>> + __entry->upcall_cmd = upcall_info->cmd;
>> + __entry->upcall_port = upcall_info->portid;
>> + __entry->upcall_mru = upcall_info->mru;
>> + ),
>> +
>> + TP_printk("dpaddr=%p dp_name=%s dev=%s skbaddr=%p len=%u
>> data_len=%u truesize=%u nr_frags=%d gso_size=%d gso_type=%#x
>> ovs_flow_hash=0x%08x recirc_id=0x%08x keyaddr=%p eth_type=0x%04x
>> ct_state=%02x ct_orig_proto=%02x flow_key_valid=%d upcall_cmd=%u
>> upcall_port=%u upcall_mru=%u",
>> + __entry->dpaddr, __get_str(dp_name), __get_str(dev_name),
>> + __entry->skbaddr, __entry->len, __entry->data_len,
>> + __entry->truesize, __entry->nr_frags, __entry->gso_size,
>> + __entry->gso_type, __entry->ovs_flow_hash,
>> + __entry->recirc_id, __entry->keyaddr, __entry->key_eth_type,
>> + __entry->key_ct_state, __entry->key_ct_orig_proto,
>> + __entry->flow_key_valid,
>> + __entry->upcall_cmd, __entry->upcall_port,
>> + __entry->upcall_mru)
>> +);
>> +
>> +#endif /* _TRACE_OPENVSWITCH_H */
>> +
>> +/* This part must be outside protection */
>> +#undef TRACE_INCLUDE_PATH
>> +#define TRACE_INCLUDE_PATH .
>> +#undef TRACE_INCLUDE_FILE
>> +#define TRACE_INCLUDE_FILE openvswitch_trace
>> +#include <trace/define_trace.h>
>>

Next message: Mark Brown: "Re: [PATCH v1 1/1] spi: Remove unneeded parentheses in spi_map_buf()"
Previous message: Robin Murphy: "Re: [PATCH v14 6/6] iommu: Remove mode argument from iommu_set_dma_strict()"
In reply to: Dumitru Ceara: "Re: [RFC net-next] openvswitch: add trace points"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]