On Tue, Oct 2, 2018 at 2:54 PM Florian Fainelli <f.faine...@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.faine...@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.
