Re: [PATCH net-next] net: use in_softirq() to indicate the NAPI context in napi_consume_skb()

[Yunsheng Lin]

On 2020/9/21 16:17, Eric Dumazet wrote:
> On Mon, Sep 21, 2020 at 10:10 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>>
>> On 2020/9/21 15:19, Eric Dumazet wrote:
>>> On Mon, Sep 21, 2020 at 4:08 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>>>>
>>>> When napi_consume_skb() is called in the tx desc cleaning process,
>>>> it is usually in the softirq context(BH disabled, or are processing
>>>> softirqs), but it may also be in the task context, such as in the
>>>> netpoll or loopback selftest process.
>>>>
>>>> Currently napi_consume_skb() uses non-zero budget to indicate the
>>>> NAPI context, the driver writer may provide the wrong budget when
>>>> tx desc cleaning function is reused for both NAPI and non-NAPI
>>>> context, see [1].
>>>>
>>>> So this patch uses in_softirq() to indicate the NAPI context, which
>>>> doesn't necessarily mean in NAPI context, but it shouldn't care if
>>>> NAPI context or not as long as it runs in softirq context or with BH
>>>> disabled, then _kfree_skb_defer() will push the skb to the particular
>>>> cpu' napi_alloc_cache atomically.
>>>>
>>>> [1] https://lkml.org/lkml/2020/9/15/38
>>>>
>>>> Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
>>>> ---
>>>> note that budget parameter is not removed in this patch because it
>>>> involves many driver changes, we can remove it in separate patch if
>>>> this patch is accepted.
>>>> ---
>>>>  net/core/skbuff.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index e077447..03d0d28 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -895,8 +895,10 @@ void __kfree_skb_defer(struct sk_buff *skb)
>>>>
>>>>  void napi_consume_skb(struct sk_buff *skb, int budget)
>>>>  {
>>>> -       /* Zero budget indicate non-NAPI context called us, like netpoll */
>>>> -       if (unlikely(!budget)) {
>>>> +       /* called by non-softirq context, which usually means non-NAPI
>>>> +        * context, like netpoll.
>>>> +        */
>>>> +       if (unlikely(!in_softirq())) {
>>>>                 dev_consume_skb_any(skb);
>>>>                 return;
>>>>         }
>>>> --
>>>
>>>
>>> I do not think we should add this kind of fuzzy logic, just because
>>> _one_ driver author made a mistake.
>>>
>>> Add a disable_bh() in the driver slow path, and accept the _existing_
>>> semantic, the one that was understood by dozens.
>>
>> As my understanding, this patch did not change _existing_ semantic,
>> it still only call _kfree_skb_defer() in softirq context. This patch
>> just remove the requirement that a softirq context hint need to be
>> provided to decide whether calling _kfree_skb_defer().
> 
> I do not want to remove the requirement.
> 
>>
>> Yes, we can add DEBUG_NET() clauses to catch this kind of error as
>> you suggested.
>>
>> But why we need such a debug clauses, when we can decide if delaying
>> skb freeing is possible in napi_consume_skb(), why not just use
>> in_softirq() to make this API more easy to use? Just as __dev_kfree_skb_any()
>> API use "in_irq() || irqs_disabled()" checking to handle the irq context
>> and non-irq context.
> 
> 
> I just do not like your patch.
> 
> Copying another piece of fuzzy logic, inherited from legacy code is
> not an excuse.
> 
> Add a local_bh_disable() in the driver slow path to meet _existing_
> requirement, so that we can keep the hot path fast.

"!in_softirq()" checking make the napi_consume_skb() slower than
"!budget" checking? do I miss something?

As a matter of fact, the hns3 driver has fixed this problem by
passing zero-budget to napi_consume_skb() in non-NAPI context, this
patch is more about how to avoid or catch this kind of error.

So your opinion is still to catch this kind of error using something
like DEBUG_NET() clauses?

> .
>