| Message ID | 167293336279.249536.18331792118487373874.stgit@firesoul (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: use kmem_cache_free_bulk in kfree_skb_list | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1 this patch: 1 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 53 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 05 Jan 16:42, Jesper Dangaard Brouer wrote: >The SKB drop reason uses __builtin_return_address(0) to give the call >"location" to trace_kfree_skb() tracepoint skb:kfree_skb. > >To keep this stable for compilers kfree_skb_reason() is annotated with >__fix_address (noinline __noclone) as fixed in commit c205cc7534a9 >("net: skb: prevent the split of kfree_skb_reason() by gcc"). > >The function kfree_skb_list_reason() invoke kfree_skb_reason(), which >cause the __builtin_return_address(0) "location" to report the >unexpected address of kfree_skb_list_reason. > >Example output from 'perf script': > kpktgend_0 1337 [000] 81.002597: skb:kfree_skb: skbaddr=0xffff888144824700 protocol=2048 location=kfree_skb_list_reason+0x1e reason: QDISC_DROP > >Patch creates an __always_inline __kfree_skb_reason() helper call that >is called from both kfree_skb_list() and kfree_skb_list_reason(). >Suggestions for solutions that shares code better are welcome. > >As preparation for next patch move __kfree_skb() invocation out of >this helper function. > >Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >--- > net/core/skbuff.c | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > >diff --git a/net/core/skbuff.c b/net/core/skbuff.c >index 4a0eb5593275..007a5fbe284b 100644 >--- a/net/core/skbuff.c >+++ b/net/core/skbuff.c >@@ -932,6 +932,21 @@ void __kfree_skb(struct sk_buff *skb) > } > EXPORT_SYMBOL(__kfree_skb); > >+static __always_inline >+bool __kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) >+{ >+ if (unlikely(!skb_unref(skb))) >+ return false; >+ >+ DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); >+ >+ if (reason == SKB_CONSUMED) >+ trace_consume_skb(skb); >+ else >+ trace_kfree_skb(skb, __builtin_return_address(0), reason); >+ return true; why not just call __kfree_skb(skb); here instead of the boolean return ? if it because __kfree_skb() makes a call to skb_release_all()->..->kfree_skb_list_reason() then it's already too deep and the return address in that case isn't predictable, so you're not avoiding any breakage by keeping direct calls to __kfree_skb() from kfree_skb_reason and+kfree_skb_list_reason >+} >+ > /** > * kfree_skb_reason - free an sk_buff with special reason > * @skb: buffer to free >@@ -944,26 +959,19 @@ EXPORT_SYMBOL(__kfree_skb); > void __fix_address > kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) > { >- if (unlikely(!skb_unref(skb))) >- return; >- >- DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); >- >- if (reason == SKB_CONSUMED) >- trace_consume_skb(skb); >- else >- trace_kfree_skb(skb, __builtin_return_address(0), reason); >- __kfree_skb(skb); >+ if (__kfree_skb_reason(skb, reason)) >+ __kfree_skb(skb); > } > EXPORT_SYMBOL(kfree_skb_reason); > >-void kfree_skb_list_reason(struct sk_buff *segs, >- enum skb_drop_reason reason) >+void __fix_address >+kfree_skb_list_reason(struct sk_buff *segs, enum skb_drop_reason reason) > { > while (segs) { > struct sk_buff *next = segs->next; > >- kfree_skb_reason(segs, reason); >+ if (__kfree_skb_reason(segs, reason)) >+ __kfree_skb(segs); > segs = next; > } > } > >
On 06 Jan 11:54, Saeed Mahameed wrote: >On 05 Jan 16:42, Jesper Dangaard Brouer wrote: >>The SKB drop reason uses __builtin_return_address(0) to give the call >>"location" to trace_kfree_skb() tracepoint skb:kfree_skb. >> >>To keep this stable for compilers kfree_skb_reason() is annotated with >>__fix_address (noinline __noclone) as fixed in commit c205cc7534a9 >>("net: skb: prevent the split of kfree_skb_reason() by gcc"). >> >>The function kfree_skb_list_reason() invoke kfree_skb_reason(), which >>cause the __builtin_return_address(0) "location" to report the >>unexpected address of kfree_skb_list_reason. >> >>Example output from 'perf script': >>kpktgend_0 1337 [000] 81.002597: skb:kfree_skb: skbaddr=0xffff888144824700 protocol=2048 location=kfree_skb_list_reason+0x1e reason: QDISC_DROP >> >>Patch creates an __always_inline __kfree_skb_reason() helper call that >>is called from both kfree_skb_list() and kfree_skb_list_reason(). >>Suggestions for solutions that shares code better are welcome. >> >>As preparation for next patch move __kfree_skb() invocation out of >>this helper function. >> >>Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>--- >>net/core/skbuff.c | 34 +++++++++++++++++++++------------- >>1 file changed, 21 insertions(+), 13 deletions(-) >> >>diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>index 4a0eb5593275..007a5fbe284b 100644 >>--- a/net/core/skbuff.c >>+++ b/net/core/skbuff.c >>@@ -932,6 +932,21 @@ void __kfree_skb(struct sk_buff *skb) >>} >>EXPORT_SYMBOL(__kfree_skb); >> >>+static __always_inline >>+bool __kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) >>+{ >>+ if (unlikely(!skb_unref(skb))) >>+ return false; >>+ >>+ DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); >>+ >>+ if (reason == SKB_CONSUMED) >>+ trace_consume_skb(skb); >>+ else >>+ trace_kfree_skb(skb, __builtin_return_address(0), reason); >>+ return true; > >why not just call __kfree_skb(skb); here instead of the boolean return >? if it because __kfree_skb() makes a call to >skb_release_all()->..->kfree_skb_list_reason() >then it's already too deep and the return address in that case isn't >predictable, so you're not avoiding any breakage by keeping >direct calls to __kfree_skb() from kfree_skb_reason and+kfree_skb_list_reason > I see now, it's because of the next patch, you will use a different deallocator in kfree_skb_list_reason. Reviewed-by: Saeed Mahameed <saeed@kernel.org>
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 4a0eb5593275..007a5fbe284b 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -932,6 +932,21 @@ void __kfree_skb(struct sk_buff *skb) } EXPORT_SYMBOL(__kfree_skb); +static __always_inline +bool __kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) +{ + if (unlikely(!skb_unref(skb))) + return false; + + DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); + + if (reason == SKB_CONSUMED) + trace_consume_skb(skb); + else + trace_kfree_skb(skb, __builtin_return_address(0), reason); + return true; +} + /** * kfree_skb_reason - free an sk_buff with special reason * @skb: buffer to free @@ -944,26 +959,19 @@ EXPORT_SYMBOL(__kfree_skb); void __fix_address kfree_skb_reason(struct sk_buff *skb, enum skb_drop_reason reason) { - if (unlikely(!skb_unref(skb))) - return; - - DEBUG_NET_WARN_ON_ONCE(reason <= 0 || reason >= SKB_DROP_REASON_MAX); - - if (reason == SKB_CONSUMED) - trace_consume_skb(skb); - else - trace_kfree_skb(skb, __builtin_return_address(0), reason); - __kfree_skb(skb); + if (__kfree_skb_reason(skb, reason)) + __kfree_skb(skb); } EXPORT_SYMBOL(kfree_skb_reason); -void kfree_skb_list_reason(struct sk_buff *segs, - enum skb_drop_reason reason) +void __fix_address +kfree_skb_list_reason(struct sk_buff *segs, enum skb_drop_reason reason) { while (segs) { struct sk_buff *next = segs->next; - kfree_skb_reason(segs, reason); + if (__kfree_skb_reason(segs, reason)) + __kfree_skb(segs); segs = next; } }
The SKB drop reason uses __builtin_return_address(0) to give the call "location" to trace_kfree_skb() tracepoint skb:kfree_skb. To keep this stable for compilers kfree_skb_reason() is annotated with __fix_address (noinline __noclone) as fixed in commit c205cc7534a9 ("net: skb: prevent the split of kfree_skb_reason() by gcc"). The function kfree_skb_list_reason() invoke kfree_skb_reason(), which cause the __builtin_return_address(0) "location" to report the unexpected address of kfree_skb_list_reason. Example output from 'perf script': kpktgend_0 1337 [000] 81.002597: skb:kfree_skb: skbaddr=0xffff888144824700 protocol=2048 location=kfree_skb_list_reason+0x1e reason: QDISC_DROP Patch creates an __always_inline __kfree_skb_reason() helper call that is called from both kfree_skb_list() and kfree_skb_list_reason(). Suggestions for solutions that shares code better are welcome. As preparation for next patch move __kfree_skb() invocation out of this helper function. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- net/core/skbuff.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)