Date:   Wed, 3 Oct 2018 07:26:29 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        netdev <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: __dev_kfree_skb_any() and use of dev_kfree_skb()

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@...il.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@...il.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
> 

Powered by blists - more mailing lists