* [PATCH] packet: uses kfree_skb() for drop. @ 2016-04-06 16:54 Weongyo Jeong 2016-04-06 17:27 ` Willem de Bruijn 0 siblings, 1 reply; 3+ messages in thread From: Weongyo Jeong @ 2016-04-06 16:54 UTC (permalink / raw) To: netdev; +Cc: Weongyo Jeong, David S. Miller, Willem de Bruijn consume_skb() isn't for drop or error cases. kfree_skb() is more proper one. Signed-off-by: Weongyo Jeong <weongyo.linux@gmail•com> --- net/packet/af_packet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 1ecfa71..a75d5bf 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2141,7 +2141,7 @@ drop_n_restore: skb->len = skb_len; } drop: - consume_skb(skb); + kfree_skb(skb); return 0; } -- 2.1.3 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] packet: uses kfree_skb() for drop. 2016-04-06 16:54 [PATCH] packet: uses kfree_skb() for drop Weongyo Jeong @ 2016-04-06 17:27 ` Willem de Bruijn 2016-04-06 18:10 ` Weongyo Jeong 0 siblings, 1 reply; 3+ messages in thread From: Willem de Bruijn @ 2016-04-06 17:27 UTC (permalink / raw) To: Weongyo Jeong; +Cc: Network Development, David S. Miller, Willem de Bruijn On Wed, Apr 6, 2016 at 12:54 PM, Weongyo Jeong <weongyo.linux@gmail•com> wrote: > consume_skb() isn't for drop or error cases. kfree_skb() is more proper > one. > Signed-off-by: Weongyo Jeong <weongyo.linux@gmail•com> > --- > net/packet/af_packet.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 1ecfa71..a75d5bf 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -2141,7 +2141,7 @@ drop_n_restore: > skb->len = skb_len; > } > drop: > - consume_skb(skb); > + kfree_skb(skb); This does show an inconsistency between packet_rcv and tpacket_rcv, which calls kfree_skb. A comment at consume_skb mentions that kfree_skb is intended for drops that signal a failure condition, and indeed, that makes it a useful way to track errors (e.g., with perf record -a -g -e skb:kfree_skb). This drop path is not always an error path, though. These network taps will legitimately drop references to any packets not destined to them. To be precise, only the drop_n_acct label cases are delivery errors (drops after the filter accepted the packet). Changing unconditionally to kfree_skb does pollute that useful counter with false positives. A pedantic solution is to change both functions to only call kfree_skb on drop_n_acct and consume_skb otherwise. This shorthand change does at least makes packet_rcv and tpacket_rcv more alike. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] packet: uses kfree_skb() for drop. 2016-04-06 17:27 ` Willem de Bruijn @ 2016-04-06 18:10 ` Weongyo Jeong 0 siblings, 0 replies; 3+ messages in thread From: Weongyo Jeong @ 2016-04-06 18:10 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, David S. Miller, Willem de Bruijn On Wed, Apr 06, 2016 at 01:27:11PM -0400, Willem de Bruijn wrote: > On Wed, Apr 6, 2016 at 12:54 PM, Weongyo Jeong <weongyo.linux@gmail•com> wrote: > > consume_skb() isn't for drop or error cases. kfree_skb() is more proper > > one. > > Signed-off-by: Weongyo Jeong <weongyo.linux@gmail•com> > > --- > > net/packet/af_packet.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > > index 1ecfa71..a75d5bf 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -2141,7 +2141,7 @@ drop_n_restore: > > skb->len = skb_len; > > } > > drop: > > - consume_skb(skb); > > + kfree_skb(skb); > > This does show an inconsistency between packet_rcv and tpacket_rcv, > which calls kfree_skb. > > A comment at consume_skb mentions that kfree_skb is intended for drops > that signal a failure condition, and indeed, that makes it a useful > way to track errors (e.g., with perf record -a -g -e skb:kfree_skb). > > This drop path is not always an error path, though. These network taps > will legitimately drop references to any packets not destined to them. > To be precise, only the drop_n_acct label cases are delivery errors > (drops after the filter accepted the packet). Changing unconditionally > to kfree_skb does pollute that useful counter with false positives. A > pedantic solution is to change both functions to only call kfree_skb > on drop_n_acct and consume_skb otherwise. > > This shorthand change does at least makes packet_rcv and tpacket_rcv more alike. Thank you for comments. I'll try to submit patch v2 for this case. Regards, Weongyo Jeong ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-06 18:10 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-06 16:54 [PATCH] packet: uses kfree_skb() for drop Weongyo Jeong 2016-04-06 17:27 ` Willem de Bruijn 2016-04-06 18:10 ` Weongyo Jeong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox