Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/02/2018 03:05 PM, Eric Dumazet wrote:
> On Tue, Oct 2, 2018 at 2:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> On 10/02/2018 02:17 PM, Eric Dumazet wrote:
>>> On Tue, Oct 2, 2018 at 1:07 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>
>>>> Hi Eric, Neil,
>>>>
>>>> Should not __dev_kfree_skb_any() call kfree_skb() instead of
>>>> dev_kfree_skb() which is aliased to consumes_skb() and therefore does
>>>> not flag the skb with SKB_REASON_DROPPED?
>>>>
>>>> If we take the in_irq() || irqs_disabled() branch, we will be calling
>>>> __dev_kfree_skb_irq() which takes care of setting the skb_free_reason
>>>> frmo the caller.
>>>>
>>>> Is there an implied semantic with dev_kfree_skb() that it means it was
>>>> freed by the network device and therefore this equals to a consumption
>>>> (not a drop)? The comment above dev_kfree_skb_any() seems to imply this
>>>> should be a context unaware replacement for kfree_skb().
>>>
>>>
>>> Really the problem here is that we have more than one thousand calls
>>> to dev_kfree_skb_any()
>>> (compared to ~ 90 calls to dev_consume_skb_any())
>>>
>>> So it will be a huge task cleaning all this.
>>
>> So you are kind of saying this is an established behavior, don't change
>> it :)
>>
>> One could argue that if people were happily sprinkling
>> dev_kfree_skb_any() in error or success paths, and all SKB freeing was
>> accounted for as "consumed" instead of "dropped" in non-atomic context,
>> this may not be such a big deal to reverse that and make it "dropped" in
>> all contexts?
>>
> 
> Most of these calls happening on typical hosts are from TX completion path,
> so they really are consumed, not dropped.
> 
> So if you intend to pretend they are drops, this will not please
> people using drop monitor.

I am not intending to pretend they are drops, just trying to make their
behavior consistent depending on the calling context, hence my question
whether this was intentional or not because __dev_kfree_skb_irq9() will
flag them as dropped correctly. Right now this is not consistent with
either the function name nor its comment in include/linux/netdevice.h.

> 
> Really the only way would to review all call sites and perform a
> cleanup, then propagate the ' reason' properly
> in the helper.
> 

Alright, thanks!
-- 
Florian