[PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[Christoph Paasch]

mpls_xmit() needs to prepend the MPLS-labels to the packet. That implies
one needs to make sure there is enough space for it in the headers.

Calling skb_cow() implies however that one wants to change even the
playload part of the packet (which is not true for MPLS). Thus, call
skb_cow_head() instead, which is what other tunnelling protocols do.

Running a server with this comm it entirely removed the calls to
pskb_expand_head() from the callstack in mpls_xmit() thus having
significant CPU-reduction, especially at peak times.

Cc: Roopa Prabhu <roopa@nvidia.com>
Reported-by: Craig Taylor <cmtaylor@apple.com>
Signed-off-by: Christoph Paasch <cpaasch@apple.com>
---
 net/mpls/mpls_iptunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 4385fd3b13be..6e73da94af7f 100644
--- a/net/mpls/mpls_iptunnel.c
+++ b/net/mpls/mpls_iptunnel.c
@@ -106,7 +106,7 @@ static int mpls_xmit(struct sk_buff *skb)
 		hh_len = 0;
 
 	/* Ensure there is enough space for the headers in the skb */
-	if (skb_cow(skb, hh_len + new_header_size))
+	if (skb_cow_head(skb, hh_len + new_header_size))
 		goto drop;
 
 	skb_set_inner_protocol(skb, skb->protocol);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[Simon Horman]

On Thu, Aug 15, 2024 at 09:12:01AM -0700, Christoph Paasch wrote:
> mpls_xmit() needs to prepend the MPLS-labels to the packet. That implies
> one needs to make sure there is enough space for it in the headers.
> 
> Calling skb_cow() implies however that one wants to change even the
> playload part of the packet (which is not true for MPLS). Thus, call
> skb_cow_head() instead, which is what other tunnelling protocols do.
> 
> Running a server with this comm it entirely removed the calls to
> pskb_expand_head() from the callstack in mpls_xmit() thus having
> significant CPU-reduction, especially at peak times.

Hi Christoph and Craig,

Including some performance data here would be nice.

> Cc: Roopa Prabhu <roopa@nvidia.com>
> Reported-by: Craig Taylor <cmtaylor@apple.com>
> Signed-off-by: Christoph Paasch <cpaasch@apple.com>


And one minor nit, which I do not think warrants a repost:
netnext -> net-next.

In any case, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[Christoph Paasch]

Hello!

> On Aug 16, 2024, at 4:18 AM, Simon Horman <horms@kernel.org> wrote:
> 
> On Thu, Aug 15, 2024 at 09:12:01AM -0700, Christoph Paasch wrote:
>> mpls_xmit() needs to prepend the MPLS-labels to the packet. That implies
>> one needs to make sure there is enough space for it in the headers.
>> 
>> Calling skb_cow() implies however that one wants to change even the
>> playload part of the packet (which is not true for MPLS). Thus, call
>> skb_cow_head() instead, which is what other tunnelling protocols do.
>> 
>> Running a server with this comm it entirely removed the calls to
>> pskb_expand_head() from the callstack in mpls_xmit() thus having
>> significant CPU-reduction, especially at peak times.
> 
> Hi Christoph and Craig,
> 
> Including some performance data here would be nice.

Getting exact production performance data is going to be a major challenge. Not a technical challenge, but rather logistically, ...

>> Cc: Roopa Prabhu <roopa@nvidia.com>
>> Reported-by: Craig Taylor <cmtaylor@apple.com>
>> Signed-off-by: Christoph Paasch <cpaasch@apple.com>
> 
> 
> And one minor nit, which I do not think warrants a repost:
> netnext -> net-next.

Ugh - I was wondering why did patchwork thought it was a fix… that explains it.

If a respin is needed, please let me know.

> In any case, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>


Thanks,
Christoph

Re: [PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 15 Aug 2024 09:12:01 -0700 you wrote:
> mpls_xmit() needs to prepend the MPLS-labels to the packet. That implies
> one needs to make sure there is enough space for it in the headers.
> 
> Calling skb_cow() implies however that one wants to change even the
> playload part of the packet (which is not true for MPLS). Thus, call
> skb_cow_head() instead, which is what other tunnelling protocols do.
> 
> [...]

Here is the summary with links:
  - [netnext] mpls: Reduce skb re-allocations due to skb_cow()
    https://git.kernel.org/netdev/net-next/c/f4ae8420f6eb

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH netnext] mpls: Reduce skb re-allocations due to skb_cow()

[Simon Horman]

On Fri, Aug 16, 2024 at 03:20:03PM -0700, Christoph Paasch wrote:
> Hello!
> 
> > On Aug 16, 2024, at 4:18 AM, Simon Horman <horms@kernel.org> wrote:
> > 
> > On Thu, Aug 15, 2024 at 09:12:01AM -0700, Christoph Paasch wrote:
> >> mpls_xmit() needs to prepend the MPLS-labels to the packet. That implies
> >> one needs to make sure there is enough space for it in the headers.
> >> 
> >> Calling skb_cow() implies however that one wants to change even the
> >> playload part of the packet (which is not true for MPLS). Thus, call
> >> skb_cow_head() instead, which is what other tunnelling protocols do.
> >> 
> >> Running a server with this comm it entirely removed the calls to
> >> pskb_expand_head() from the callstack in mpls_xmit() thus having
> >> significant CPU-reduction, especially at peak times.
> > 
> > Hi Christoph and Craig,
> > 
> > Including some performance data here would be nice.
> 
> Getting exact production performance data is going to be a major challenge. Not a technical challenge, but rather logistically, ...

Understood :)