| Message ID | 20230215121707.1936762-3-gal@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Couple of minor improvements to build_skb variants | 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, 8 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, 15 Feb 2023 14:17:07 +0200 Gal Pressman wrote: > - if (skb && frag_size) { > + if (likely(skb) && frag_size) { Should frag_size also be inside the likely? See the warning in __build_skb_around().
On Wed, 2023-02-15 at 14:17 +0200, Gal Pressman wrote: > Similarly to napi_build_skb(), it is likely the skb allocation in > build_skb() succeeded. > > Reviewed-by: Tariq Toukan <tariqt@nvidia.com> > Signed-off-by: Gal Pressman <gal@nvidia.com> > --- > net/core/skbuff.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 069604b9ff9d..3aa9687d7546 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -420,7 +420,7 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size) > { > struct sk_buff *skb = __build_skb(data, frag_size); > > - if (skb && frag_size) { > + if (likely(skb) && frag_size) { I concur with Jakub: frag_size != 0 is a likely event. Additionally, without including 'frag_size' into the likely() annotation the compiler could consider the whole branch not likely: I think should be: if (likely(skb && frag_size)) { Cheers, Paolo
On 16/02/2023 1:01, Jakub Kicinski wrote: > On Wed, 15 Feb 2023 14:17:07 +0200 Gal Pressman wrote: >> - if (skb && frag_size) { >> + if (likely(skb) && frag_size) { > > Should frag_size also be inside the likely? > See the warning in __build_skb_around(). Agree, thanks Jakub and Paolo. Do you want to fix it up when you take the patch, or should I submit a v2?
On Thu, 2023-02-16 at 16:55 +0200, Gal Pressman wrote: > On 16/02/2023 1:01, Jakub Kicinski wrote: > > On Wed, 15 Feb 2023 14:17:07 +0200 Gal Pressman wrote: > > > - if (skb && frag_size) { > > > + if (likely(skb) && frag_size) { > > > > Should frag_size also be inside the likely? > > See the warning in __build_skb_around(). > > Agree, thanks Jakub and Paolo. > > Do you want to fix it up when you take the patch, or should I submit a v2? The way to address even such small changes is via re-submissions, please send a v2, thanks! Paolo
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 069604b9ff9d..3aa9687d7546 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -420,7 +420,7 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size) { struct sk_buff *skb = __build_skb(data, frag_size); - if (skb && frag_size) { + if (likely(skb) && frag_size) { skb->head_frag = 1; skb_propagate_pfmemalloc(virt_to_head_page(data), skb); }