Received-SPF: pass (google.com: domain of linux-kernel+bounces-209427-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <18328cc2-c135-4b69-8c5f-cd45998e970f@kernel.org>
Date: Tue, 11 Jun 2024 09:55:11 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 net-next 14/15] net: Reference bpf_redirect_info via
 task_struct on PREEMPT_RT.
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
 "David S. Miller" <davem@davemloft.net>,
 Daniel Bristot de Oliveira <bristot@kernel.org>,
 Boqun Feng <boqun.feng@gmail.com>, Daniel Borkmann <daniel@iogearbox.net>,
 Eric Dumazet <edumazet@google.com>, Frederic Weisbecker
 <frederic@kernel.org>, Ingo Molnar <mingo@redhat.com>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
 Peter Zijlstra <peterz@infradead.org>, Thomas Gleixner <tglx@linutronix.de>,
 Waiman Long <longman@redhat.com>, Will Deacon <will@kernel.org>,
 Alexei Starovoitov <ast@kernel.org>, Andrii Nakryiko <andrii@kernel.org>,
 Eduard Zingerman <eddyz87@gmail.com>, Hao Luo <haoluo@google.com>,
 Jiri Olsa <jolsa@kernel.org>, John Fastabend <john.fastabend@gmail.com>,
 KP Singh <kpsingh@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>,
 Song Liu <song@kernel.org>, Stanislav Fomichev <sdf@google.com>,
 =?UTF-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@redhat.com>,
 Yonghong Song <yonghong.song@linux.dev>, bpf@vger.kernel.org
References: <20240607070427.1379327-1-bigeasy@linutronix.de>
 <20240607070427.1379327-15-bigeasy@linutronix.de>
 <045e3716-3c3a-4238-b38a-3616c8974e2c@kernel.org>
 <20240610165014.uWp_yZuW@linutronix.de>
Content-Language: en-US
From: Jesper Dangaard Brouer <hawk@kernel.org>
In-Reply-To: <20240610165014.uWp_yZuW@linutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 10/06/2024 18.50, Sebastian Andrzej Siewior wrote:
> On 2024-06-07 13:51:25 [+0200], Jesper Dangaard Brouer wrote:
>> The memset can be further optimized as it currently clears 64 bytes, but
>> it only need to clear 40 bytes, see pahole below.
>>
>> Replace memset with something like:
>>   memset(&bpf_net_ctx->ri, 0, offsetof(struct bpf_net_context, ri.nh));
>>
>> This is an optimization, because with 64 bytes this result in a rep-stos
>> (repeated string store operation) that on Intel touch CPU-flags (to be
>> IRQ safe) which is slow, while clearing 40 bytes doesn't cause compiler
>> to use this instruction, which is faster.  Memset benchmarked with [1]
> 
> I've been playing along with this and have to say that "rep stosq" is
> roughly 3x slower vs "movq" for 64 bytes on all x86 I've been looking
> at.

Thanks for confirming "rep stos" is 3x slower for small sizes.


> For gcc the stosq vs movq depends on the CPU settings. The generic uses
> movq up to 40 bytes, skylake uses movq even for 64bytes. clang…
> This could be tuned via -mmemset-strategy=libcall:64:align,rep_8byte:-1:align
> 

Cool I didn't know of this tuning.  Is this a compiler option?
Where do I change this setting, as I would like to experiment with this
for our prod kernels.

My other finding is, this primarily a kernel compile problem, because
for userspace compiler chooses to use MMX instructions (e.g. movaps
xmmword ptr[rsp], xmm0).  The kernel compiler options (-mno-sse -mno-mmx
-mno-sse2 -mno-3dnow -mno-avx) disables this, which aparently changes
the tipping point.


> I folded this into the last two patches:
> 
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index d2b4260d9d0be..1588d208f1348 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -744,27 +744,40 @@ struct bpf_redirect_info {
>   	struct bpf_nh_params nh;
>   };
>   
> +enum bpf_ctx_init_type {
> +	bpf_ctx_ri_init,
> +	bpf_ctx_cpu_map_init,
> +	bpf_ctx_dev_map_init,
> +	bpf_ctx_xsk_map_init,
> +};
> +
>   struct bpf_net_context {
>   	struct bpf_redirect_info ri;
>   	struct list_head cpu_map_flush_list;
>   	struct list_head dev_map_flush_list;
>   	struct list_head xskmap_map_flush_list;
> +	unsigned int flags;

Why have yet another flags variable, when we already have two flags in 
bpf_redirect_info ?

>   };
>   
> +static inline bool bpf_net_ctx_need_init(struct bpf_net_context *bpf_net_ctx,
> +					 enum bpf_ctx_init_type flag)
> +{
> +	return !(bpf_net_ctx->flags & (1 << flag));
> +}
> +
> +static inline bool bpf_net_ctx_set_flag(struct bpf_net_context *bpf_net_ctx,
> +					enum bpf_ctx_init_type flag)
> +{
> +	return bpf_net_ctx->flags |= 1 << flag;
> +}
> +
>   static inline struct bpf_net_context *bpf_net_ctx_set(struct bpf_net_context *bpf_net_ctx)
>   {
>   	struct task_struct *tsk = current;
>   
>   	if (tsk->bpf_net_context != NULL)
>   		return NULL;
> -	memset(&bpf_net_ctx->ri, 0, sizeof(bpf_net_ctx->ri));
> -
> -	if (IS_ENABLED(CONFIG_BPF_SYSCALL)) {
> -		INIT_LIST_HEAD(&bpf_net_ctx->cpu_map_flush_list);
> -		INIT_LIST_HEAD(&bpf_net_ctx->dev_map_flush_list);
> -	}
> -	if (IS_ENABLED(CONFIG_XDP_SOCKETS))
> -		INIT_LIST_HEAD(&bpf_net_ctx->xskmap_map_flush_list);
> +	bpf_net_ctx->flags = 0;
>   
>   	tsk->bpf_net_context = bpf_net_ctx;
>   	return bpf_net_ctx;
> @@ -785,6 +798,11 @@ static inline struct bpf_redirect_info *bpf_net_ctx_get_ri(void)
>   {
>   	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
>   
> +	if (bpf_net_ctx_need_init(bpf_net_ctx, bpf_ctx_ri_init)) {
> +		memset(&bpf_net_ctx->ri, 0, offsetof(struct bpf_net_context, ri.nh));
> +		bpf_net_ctx_set_flag(bpf_net_ctx, bpf_ctx_ri_init);
> +	}
> +
>   	return &bpf_net_ctx->ri;
>   }
>   
> @@ -792,6 +810,11 @@ static inline struct list_head *bpf_net_ctx_get_cpu_map_flush_list(void)
>   {
>   	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
>   
> +	if (bpf_net_ctx_need_init(bpf_net_ctx, bpf_ctx_cpu_map_init)) {
> +		INIT_LIST_HEAD(&bpf_net_ctx->cpu_map_flush_list);
> +		bpf_net_ctx_set_flag(bpf_net_ctx, bpf_ctx_cpu_map_init);
> +	}
> +
>   	return &bpf_net_ctx->cpu_map_flush_list;
>   }
>   
> @@ -799,6 +822,11 @@ static inline struct list_head *bpf_net_ctx_get_dev_flush_list(void)
>   {
>   	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
>   
> +	if (bpf_net_ctx_need_init(bpf_net_ctx, bpf_ctx_dev_map_init)) {
> +		INIT_LIST_HEAD(&bpf_net_ctx->dev_map_flush_list);
> +		bpf_net_ctx_set_flag(bpf_net_ctx, bpf_ctx_dev_map_init);
> +	}
> +
>   	return &bpf_net_ctx->dev_map_flush_list;
>   }
>   
> @@ -806,6 +834,11 @@ static inline struct list_head *bpf_net_ctx_get_xskmap_flush_list(void)
>   {
>   	struct bpf_net_context *bpf_net_ctx = bpf_net_ctx_get();
>   
> +	if (bpf_net_ctx_need_init(bpf_net_ctx, bpf_ctx_xsk_map_init)) {
> +		INIT_LIST_HEAD(&bpf_net_ctx->xskmap_map_flush_list);
> +		bpf_net_ctx_set_flag(bpf_net_ctx, bpf_ctx_xsk_map_init);
> +	}
> +
>   	return &bpf_net_ctx->xskmap_map_flush_list;
>   }
>   
> 
> Sebastian