| Message ID | 20240815161201.22021-1-cpaasch@apple.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4ae8420f6ebdabf97e0c4f5f99412768687985f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [netnext] mpls: Reduce skb re-allocations due to skb_cow() | expand |
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>
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
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!
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 :)
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);
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(-)