On 10/02/2018 03:05 PM, Eric Dumazet wrote: > 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.
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
