__dev_kfree_skb_any() and use of dev_kfree_skb()

__dev_kfree_skb_any() and use of dev_kfree_skb()

[Florian Fainelli]

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().

Thanks!
-- 
Florian

Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Eric Dumazet]

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.

(dev_kfree_skb_any() calls were added way before the drop monitor stuff)

Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Florian Fainelli]

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?

> 
> (dev_kfree_skb_any() calls were added way before the drop monitor stuff)
> 


-- 
Florian

Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Eric Dumazet]

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.

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

Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Florian Fainelli]

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

Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

[Neil Horman]

On Tue, Oct 02, 2018 at 03:20:48PM -0700, Florian Fainelli wrote:
> 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.
> 
As Eric noted, dev_kfree_skb_any was added before the drop monitor code, and so
I don't think much commentary can be made of in the way of 'intent'.  The way to
reconcile all these code points is to look at each one in turn, determine from
the location and the purpose of the call if it is really a drop, or simply the
end of the useful life of the skb (a consume), and either change the call to the
appropriate one, or decide that whats there is correct.  I've done this in the
past in several locations, and honestly, its just a tedium.  I find that, if you
are using drop monitor, and come accross a false positive (or false negative),
submit a patch for that location to reconcile it, and slowly they will all get
corrected.

Neil

> > 
> > 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
>