[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fe6f2325-7454-413e-acba-b3c5f3313dfe@intel.com>
Date: Thu, 11 Apr 2024 11:11:20 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jason Xing <kerneljasonxing@...il.com>
CC: Eric Dumazet <edumazet@...gle.com>, <pablo@...filter.org>,
<kuba@...nel.org>, <pabeni@...hat.com>, <davem@...emloft.net>,
<horms@...nel.org>, <netdev@...r.kernel.org>, Jason Xing
<kernelxing@...cent.com>
Subject: Re: [PATCH net-next] net: save some cycles when doing
skb_attempt_defer_free()
From: Jason Xing <kerneljasonxing@...il.com>
Date: Thu, 11 Apr 2024 15:31:23 +0800
> On Thu, Apr 11, 2024 at 3:12 PM Eric Dumazet <edumazet@...gle.com> wrote:
>>
>> On Thu, Apr 11, 2024 at 8:33 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>>>
>>> On Thu, Apr 11, 2024 at 1:27 PM Eric Dumazet <edumazet@...gle.com> wrote:
>>>>
>>>> On Thu, Apr 11, 2024 at 5:25 AM Jason Xing <kerneljasonxing@...il.com> wrote:
>>>>>
>>>>> From: Jason Xing <kernelxing@...cent.com>
>>>>>
>>>>> Normally, we don't face these two exceptions very often meanwhile
>>>>> we have some chance to meet the condition where the current cpu id
>>>>> is the same as skb->alloc_cpu.
>>>>>
>>>>> One simple test that can help us see the frequency of this statement
>>>>> 'cpu == raw_smp_processor_id()':
>>>>> 1. running iperf -s and iperf -c [ip] -P [MAX CPU]
>>>>> 2. using BPF to capture skb_attempt_defer_free()
>>>>>
>>>>> I can see around 4% chance that happens to satisfy the statement.
>>>>> So moving this statement at the beginning can save some cycles in
>>>>> most cases.
>>>>>
>>>>> Signed-off-by: Jason Xing <kernelxing@...cent.com>
>>>>> ---
>>>>> net/core/skbuff.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>>> index ab970ded8a7b..b4f252dc91fb 100644
>>>>> --- a/net/core/skbuff.c
>>>>> +++ b/net/core/skbuff.c
>>>>> @@ -7002,9 +7002,9 @@ void skb_attempt_defer_free(struct sk_buff *skb)
>>>>> unsigned int defer_max;
>>>>> bool kick;
>>>>>
>>>>> - if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
>>>>> + if (cpu == raw_smp_processor_id() ||
>>>>> !cpu_online(cpu) ||
>>>>> - cpu == raw_smp_processor_id()) {
>>>>> + WARN_ON_ONCE(cpu >= nr_cpu_ids)) {
>>>>> nodefer: kfree_skb_napi_cache(skb);
>>>>> return;
>>>>> }
>>>>
>>>> Wrong patch.
>>>>
>>>> cpu_online(X) is undefined and might crash if X is out of bounds on CONFIG_SMP=y
>>>
>>> Even if skb->alloc_cpu is larger than nr_cpu_ids, I don't know why the
>>> integer test statement could cause crashing the kernel. It's just a
>>> simple comparison. And if the statement is true,
>>> raw_smp_processor_id() can guarantee the validation, right?
>>
>> Please read again the code you wrote, or run it with skb->alloc_cpu
>> being set to 45000 on a full DEBUG kernel.
>>
>> You are focusing on skb->alloc_cpu == raw_smp_processor_id(), I am
>> focusing on what happens
>> when this condition is not true.
>
> Sorry. My bad. I put the wrong order of '!cpu_online(cpu)' and 'cpu >=
> nr_cpu_ids'. I didn't consider the out-of-bound issue. I should have
> done more checks :(
>
> The correct patch should be:
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index ab970ded8a7b..6dc577a3ea6a 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -7002,9 +7002,9 @@ void skb_attempt_defer_free(struct sk_buff *skb)
> unsigned int defer_max;
> bool kick;
>
> - if (WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
> - !cpu_online(cpu) ||
> - cpu == raw_smp_processor_id()) {
> + if (cpu == raw_smp_processor_id() ||
> + WARN_ON_ONCE(cpu >= nr_cpu_ids) ||
> + !cpu_online(cpu)) {
This one looks good to me.
Feel free to add
Reviewed-by: Alexander Lobakin <aleksander.lobakin@...el.com>
To your v2 before sending.
> nodefer: kfree_skb_napi_cache(skb);
> return;
> }
>
> I will submit V2 tomorrow.
>
> Thanks,
> Jason
Thanks,
Olek
Powered by blists - more mailing lists