Received-SPF: pass (google.com: domain of linux-kernel+bounces-158165-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <542ed8dd-2d9c-4e4f-81dc-e2a9bdaac3b0@redhat.com>
Date: Thu, 25 Apr 2024 10:06:20 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next 4/8] net: psample: add tracepoint
To: Ido Schimmel <idosch@idosch.org>
Cc: netdev@vger.kernel.org, aconole@redhat.com, echaudro@redhat.com,
 horms@kernel.org, i.maximets@ovn.org, Yotam Gigi <yotam.gi@gmail.com>,
 "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 linux-kernel@vger.kernel.org
References: <20240424135109.3524355-1-amorenoz@redhat.com>
 <20240424135109.3524355-5-amorenoz@redhat.com> <ZioDvluh7ymBI8qF@shredder>
Content-Language: en-US
From: Adrian Moreno <amorenoz@redhat.com>
In-Reply-To: <ZioDvluh7ymBI8qF@shredder>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/25/24 09:18, Ido Schimmel wrote:
> On Wed, Apr 24, 2024 at 03:50:51PM +0200, Adrian Moreno wrote:
>> Currently there are no widely-available tools to dump the metadata and
>> group information when a packet is sampled, making it difficult to
>> troubleshoot related issues.
>>
>> This makes psample use the event tracing framework to log the sampling
>> of a packet so that it's easier to quickly identify the source
>> (i.e: group) and context (i.e: metadata) of a packet being sampled.
>>
>> This patch creates some checkpatch splats, but the style of the
>> tracepoint definition mimics that of other modules so it seems
>> acceptable.
> 
> I don't see a good reason to add this tracepoint (which we won't be able
> to remove) when you can easily do that with bpftrace which by now should
> be widely available:
> 
> #!/usr/bin/bpftrace
> 
> kfunc:psample_sample_packet
> {
>          $ts_us = nsecs() / 1000;
>          $secs = $ts_us / 1000000;
>          $us = $ts_us % 1000000;
>          $group = args.group;
>          $skb = args.skb;
>          $md = args.md;
> 
>          printf("%-16s %-6d %6llu.%6llu group_num = %u refcount=%u seq=%u skbaddr=%p len=%u data_len=%u sample_rate=%u in_ifindex=%d out_ifindex=%d user_cookie=%rx\n",
>                 comm, pid, $secs, $us, $group->group_num, $group->refcount, $group->seq,
>                 $skb, $skb->len, $skb->data_len, args.sample_rate,
>                 $md->in_ifindex, $md->out_ifindex,
>                 buf($md->user_cookie, $md->user_cookie_len));
> }
> 
> Example output:
> 
> mausezahn        984      3299.200626 group_num = 1 refcount=1 seq=13775 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
> \xde\xad\xbe\xef
> mausezahn        984      3299.281424 group_num = 1 refcount=1 seq=13776 skbaddr=0xffffa21143fd4000 len=42 data_len=0 sample_rate=10 in_ifindex=0 out_ifindex=20 user_cookie=
> \xde\xad\xbe\xef
> 
> Note that it prints the cookie itself unlike the tracepoint which only
> prints the hashed pointer.
> 

I agree that bpftrace can do the work relying on kfuncs/kprobes. But I guess 
that also true for many other tracepoints out there, right?

For development and labs bpftrace is perfectly fine, but using kfuncs and 
requiring recompilation is harder in production systems compared with using 
smaller CO-RE tools.

If OVS starts using psample heavily and users need to troubleshoot or merely 
observe packets as they are sampled in a more efficient way, they are likely to 
use ebpf for that. I think making it a bit easier (as in, providing a sligthly 
more stable tracepoint) is worth considering.

Can you please expand on your concerns about the tracepoint? It's on the main 
internal function of the module so, even though the function name or its 
arguments might change, it doesn't seem probable that it'll disappear 
altogether. Why else would we want to remove the tracepoint?

>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>   net/psample/psample.c |  9 +++++++
>>   net/psample/trace.h   | 62 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 71 insertions(+)
>>   create mode 100644 net/psample/trace.h
>>
>> diff --git a/net/psample/psample.c b/net/psample/psample.c
>> index 476aaad7a885..92db8ffa2ba2 100644
>> --- a/net/psample/psample.c
>> +++ b/net/psample/psample.c
>> @@ -18,6 +18,12 @@
>>   #include <net/ip_tunnels.h>
>>   #include <net/dst_metadata.h>
>>   
>> +#ifndef __CHECKER__
>> +#define CREATE_TRACE_POINTS
>> +#endif
>> +
>> +#include "trace.h"
>> +
>>   #define PSAMPLE_MAX_PACKET_SIZE 0xffff
>>   
>>   static LIST_HEAD(psample_groups_list);
>> @@ -470,6 +476,9 @@ void psample_sample_packet(struct psample_group *group, struct sk_buff *skb,
>>   	void *data;
>>   	int ret;
>>   
>> +	if (trace_psample_sample_packet_enabled())
>> +		trace_psample_sample_packet(group, skb, sample_rate, md);
> 
> My understanding is that trace_x_enabled() should only be used if you
> need to do some work to prepare parameters for the tracepoint.
> 

Oh, thanks for that! I was not aware.

>> +
>>   	meta_len = (in_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>>   		   (out_ifindex ? nla_total_size(sizeof(u16)) : 0) +
>>   		   (md->out_tc_valid ? nla_total_size(sizeof(u16)) : 0) +
>