[PATCH net-next 0/2] Couple of minor improvements to build_skb variants

[PATCH net-next 0/2] Couple of minor improvements to build_skb variants

[Gal Pressman]

First patch replaces open-coded occurrences of
skb_propagate_pfmemalloc() in build_skb() and build_skb_around().
The secnod patch adds a likely() to the skb allocation in build_skb().

Thanks

Gal Pressman (2):
  skbuff: Replace open-coded skb_propagate_pfmemalloc()s
  skbuff: Add likely to skb pointer in build_skb()

 net/core/skbuff.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

-- 
2.39.0

[PATCH net-next 1/2] skbuff: Replace open-coded skb_propagate_pfmemalloc()s

[Gal Pressman]

Use skb_propagate_pfmemalloc() in build_skb()/build_skb_around() instead
of open-coding it.

Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Gal Pressman <gal@nvidia.com>
---
 net/core/skbuff.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 13ea10cf8544..069604b9ff9d 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -422,8 +422,7 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size)
 
 	if (skb && frag_size) {
 		skb->head_frag = 1;
-		if (page_is_pfmemalloc(virt_to_head_page(data)))
-			skb->pfmemalloc = 1;
+		skb_propagate_pfmemalloc(virt_to_head_page(data), skb);
 	}
 	return skb;
 }
@@ -445,8 +444,7 @@ struct sk_buff *build_skb_around(struct sk_buff *skb,
 
 	if (frag_size) {
 		skb->head_frag = 1;
-		if (page_is_pfmemalloc(virt_to_head_page(data)))
-			skb->pfmemalloc = 1;
+		skb_propagate_pfmemalloc(virt_to_head_page(data), skb);
 	}
 	return skb;
 }
-- 
2.39.0

[PATCH net-next 2/2] skbuff: Add likely to skb pointer in build_skb()

[Gal Pressman]

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) {
 		skb->head_frag = 1;
 		skb_propagate_pfmemalloc(virt_to_head_page(data), skb);
 	}
-- 
2.39.0

Re: [PATCH net-next 0/2] Couple of minor improvements to build_skb variants

[Larysa Zaremba]

On Wed, Feb 15, 2023 at 02:17:05PM +0200, Gal Pressman wrote:
> First patch replaces open-coded occurrences of
> skb_propagate_pfmemalloc() in build_skb() and build_skb_around().
> The secnod patch adds a likely() to the skb allocation in build_skb().
> 
> Thanks
> 
> Gal Pressman (2):
>   skbuff: Replace open-coded skb_propagate_pfmemalloc()s
>   skbuff: Add likely to skb pointer in build_skb()
> 
>  net/core/skbuff.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
 
> -- 
> 2.39.0
> 

Re: [PATCH net-next 2/2] skbuff: Add likely to skb pointer in build_skb()

[Jakub Kicinski]

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().

Re: [PATCH net-next 2/2] skbuff: Add likely to skb pointer in build_skb()

[Paolo Abeni]

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

Re: [PATCH net-next 2/2] skbuff: Add likely to skb pointer in build_skb()

[Gal Pressman]

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?

Re: [PATCH net-next 2/2] skbuff: Add likely to skb pointer in build_skb()

[Paolo Abeni]

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