On Mon 07 Mar 2022 at 23:49, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Tue, Feb 22, 2022 at 05:10:03PM +0200, Vlad Buslov wrote:
>> Add tracepoints to trace creation and start of execution of flowtable
>> hardware offload 'add', 'del' and 'stats' tasks. Move struct
>> flow_offload_work from source into header file to allow access to structure
>> fields from tracepoint code.
>
> This patch, I would prefer to keep it back and explore exposing trace
> infrastructure for the flowtable through netlink.
>
What approach do you have in mind with netlink? I used tracepoints here
because they are:
- Incur no performance penalty when disabled.
- Handy to attach BPF programs to.
According to my experience with optimizing TC control path parsing
Netlink is CPU-intensive. I am also not aware of mechanisms to leverage
it to attach BPF.
>> Signed-off-by: Vlad Buslov <vladbu@xxxxxxxxxx>
>> Signed-off-by: Oz Shlomo <ozsh@xxxxxxxxxx>
>> Reviewed-by: Paul Blakey <paulb@xxxxxxxxxx>
>> ---
>> include/net/netfilter/nf_flow_table.h | 9 ++++
>> net/netfilter/nf_flow_table_offload.c | 20 +++++----
>> net/netfilter/nf_flow_table_offload_trace.h | 48 +++++++++++++++++++++
>> 3 files changed, 68 insertions(+), 9 deletions(-)
>> create mode 100644 net/netfilter/nf_flow_table_offload_trace.h
>>
>> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
>> index a3647fadf1cc..5e2aef34acaa 100644
>> --- a/include/net/netfilter/nf_flow_table.h
>> +++ b/include/net/netfilter/nf_flow_table.h
>> @@ -174,6 +174,15 @@ struct flow_offload {
>> struct rcu_head rcu_head;
>> };
>>
>> +struct flow_offload_work {
>> + struct list_head list;
>> + enum flow_cls_command cmd;
>> + int priority;
>> + struct nf_flowtable *flowtable;
>> + struct flow_offload *flow;
>> + struct work_struct work;
>> +};
>> +
>> #define NF_FLOW_TIMEOUT (30 * HZ)
>> #define nf_flowtable_time_stamp (u32)jiffies
>>
>> diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
>> index ff52d903aad9..bf94050d5b54 100644
>> --- a/net/netfilter/nf_flow_table_offload.c
>> +++ b/net/netfilter/nf_flow_table_offload.c
>> @@ -12,20 +12,13 @@
>> #include <net/netfilter/nf_conntrack_acct.h>
>> #include <net/netfilter/nf_conntrack_core.h>
>> #include <net/netfilter/nf_conntrack_tuple.h>
>> +#define CREATE_TRACE_POINTS
>> +#include "nf_flow_table_offload_trace.h"
>>
>> static struct workqueue_struct *nf_flow_offload_add_wq;
>> static struct workqueue_struct *nf_flow_offload_del_wq;
>> static struct workqueue_struct *nf_flow_offload_stats_wq;
>>
>> -struct flow_offload_work {
>> - struct list_head list;
>> - enum flow_cls_command cmd;
>> - int priority;
>> - struct nf_flowtable *flowtable;
>> - struct flow_offload *flow;
>> - struct work_struct work;
>> -};
>> -
>> #define NF_FLOW_DISSECTOR(__match, __type, __field) \
>> (__match)->dissector.offset[__type] = \
>> offsetof(struct nf_flow_key, __field)
>> @@ -895,6 +888,8 @@ static void flow_offload_work_add(struct flow_offload_work *offload)
>> struct nf_flow_rule *flow_rule[FLOW_OFFLOAD_DIR_MAX];
>> int err;
>>
>> + trace_flow_offload_work_add(offload);
>> +
>> err = nf_flow_offload_alloc(offload, flow_rule);
>> if (err < 0)
>> return;
>> @@ -911,6 +906,8 @@ static void flow_offload_work_add(struct flow_offload_work *offload)
>>
>> static void flow_offload_work_del(struct flow_offload_work *offload)
>> {
>> + trace_flow_offload_work_del(offload);
>> +
>> clear_bit(IPS_HW_OFFLOAD_BIT, &offload->flow->ct->status);
>> flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
>> flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
>> @@ -931,6 +928,8 @@ static void flow_offload_work_stats(struct flow_offload_work *offload)
>> struct flow_stats stats[FLOW_OFFLOAD_DIR_MAX] = {};
>> u64 lastused;
>>
>> + trace_flow_offload_work_stats(offload);
>> +
>> flow_offload_tuple_stats(offload, FLOW_OFFLOAD_DIR_ORIGINAL, &stats[0]);
>> flow_offload_tuple_stats(offload, FLOW_OFFLOAD_DIR_REPLY, &stats[1]);
>>
>> @@ -1034,6 +1033,7 @@ void nf_flow_offload_add(struct nf_flowtable *flowtable,
>> return;
>> }
>>
>> + trace_flow_offload_add(offload);
>> flow_offload_queue_work(offload);
>> }
>>
>> @@ -1048,6 +1048,7 @@ void nf_flow_offload_del(struct nf_flowtable *flowtable,
>> return;
>>
>> atomic_inc(&net->nft.count_wq_del);
>> + trace_flow_offload_del(offload);
>> set_bit(NF_FLOW_HW_DYING, &flow->flags);
>> flow_offload_queue_work(offload);
>> }
>> @@ -1068,6 +1069,7 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
>> return;
>>
>> atomic_inc(&net->nft.count_wq_stats);
>> + trace_flow_offload_stats(offload);
>> flow_offload_queue_work(offload);
>> }
>>
>> diff --git a/net/netfilter/nf_flow_table_offload_trace.h b/net/netfilter/nf_flow_table_offload_trace.h
>> new file mode 100644
>> index 000000000000..49cfbc2ec35d
>> --- /dev/null
>> +++ b/net/netfilter/nf_flow_table_offload_trace.h
>> @@ -0,0 +1,48 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM nf
>> +
>> +#if !defined(_NF_FLOW_TABLE_OFFLOAD_TRACE_) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _NF_FLOW_TABLE_OFFLOAD_TRACE_
>> +
>> +#include <linux/tracepoint.h>
>> +#include <net/netfilter/nf_tables.h>
>> +
>> +DECLARE_EVENT_CLASS(
>> + nf_flow_offload_work_template,
>> + TP_PROTO(struct flow_offload_work *w),
>> + TP_ARGS(w),
>> + TP_STRUCT__entry(
>> + __field(void *, work)
>> + __field(void *, flowtable)
>> + __field(void *, flow)
>> + ),
>> + TP_fast_assign(
>> + __entry->work = w;
>> + __entry->flowtable = w->flowtable;
>> + __entry->flow = w->flow;
>> + ),
>> + TP_printk("work=%p flowtable=%p flow=%p",
>> + __entry->work, __entry->flowtable, __entry->flow)
>> +);
>> +
>> +#define DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(name) \
>> + DEFINE_EVENT(nf_flow_offload_work_template, name, \
>> + TP_PROTO(struct flow_offload_work *w), TP_ARGS(w))
>> +
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_add);
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_work_add);
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_del);
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_work_del);
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_stats);
>> +DEFINE_NF_FLOW_OFFLOAD_WORK_EVENT(flow_offload_work_stats);
>> +
>> +#endif
>> +
>> +/* This part must be outside protection */
>> +#undef TRACE_INCLUDE_PATH
>> +#define TRACE_INCLUDE_PATH ../../net/netfilter
>> +#undef TRACE_INCLUDE_FILE
>> +#define TRACE_INCLUDE_FILE nf_flow_table_offload_trace
>> +#include <trace/define_trace.h>
>> --
>> 2.31.1
>>
