| Message ID | 20230529134430.492879-1-parav@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b1f2abcf817d82b54764d1474424649feda6fe1b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: Make gro complete function to return void | expand |
On Mon, May 29, 2023 at 04:44:30PM +0300, Parav Pandit wrote: > tcp_gro_complete() function only updates the skb fields related to GRO > and it always returns zero. All the 3 drivers which are using it > do not check for the return value either. > > Change it to return void instead which simplifies its callers as > error handing becomes unnecessary. > > Signed-off-by: Parav Pandit <parav@nvidia.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 5/29/23 7:44 AM, Parav Pandit wrote: > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > index 45dda7889387..88f9b0081ee7 100644 > --- a/net/ipv4/tcp_offload.c > +++ b/net/ipv4/tcp_offload.c > @@ -296,7 +296,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) > return pp; > } > > -int tcp_gro_complete(struct sk_buff *skb) > +void tcp_gro_complete(struct sk_buff *skb) > { > struct tcphdr *th = tcp_hdr(skb); > > @@ -311,8 +311,6 @@ int tcp_gro_complete(struct sk_buff *skb) > > if (skb->encapsulation) > skb->inner_transport_header = skb->transport_header; > - > - return 0; > } > EXPORT_SYMBOL(tcp_gro_complete); tcp_gro_complete seems fairly trivial. Any reason not to make it an inline and avoid another function call in the datapath? > > @@ -342,7 +340,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff) > if (NAPI_GRO_CB(skb)->is_atomic) > skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID; > > - return tcp_gro_complete(skb); > + tcp_gro_complete(skb); > + return 0; > } >
> From: David Ahern <dsahern@kernel.org> > Sent: Tuesday, May 30, 2023 11:26 AM > > On 5/29/23 7:44 AM, Parav Pandit wrote: > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index > > 45dda7889387..88f9b0081ee7 100644 > > --- a/net/ipv4/tcp_offload.c > > +++ b/net/ipv4/tcp_offload.c > > @@ -296,7 +296,7 @@ struct sk_buff *tcp_gro_receive(struct list_head > *head, struct sk_buff *skb) > > return pp; > > } > > > > -int tcp_gro_complete(struct sk_buff *skb) > > +void tcp_gro_complete(struct sk_buff *skb) > > { > > struct tcphdr *th = tcp_hdr(skb); > > > > @@ -311,8 +311,6 @@ int tcp_gro_complete(struct sk_buff *skb) > > > > if (skb->encapsulation) > > skb->inner_transport_header = skb->transport_header; > > - > > - return 0; > > } > > EXPORT_SYMBOL(tcp_gro_complete); > > tcp_gro_complete seems fairly trivial. Any reason not to make it an inline and > avoid another function call in the datapath? > Sounds good to me. With inline it should mostly improve the perf, but I do not have any of the 3 adapters which are calling this API to show perf results. Since, it is a different change touching the performance, I prefer to do follow up patch that bnx owners can test. Is that ok?
On Tue, May 30, 2023 at 5:25 PM David Ahern <dsahern@kernel.org> wrote: > > On 5/29/23 7:44 AM, Parav Pandit wrote: > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > > index 45dda7889387..88f9b0081ee7 100644 > > --- a/net/ipv4/tcp_offload.c > > +++ b/net/ipv4/tcp_offload.c > > @@ -296,7 +296,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) > > return pp; > > } > > > > -int tcp_gro_complete(struct sk_buff *skb) > > +void tcp_gro_complete(struct sk_buff *skb) > > { > > struct tcphdr *th = tcp_hdr(skb); > > > > @@ -311,8 +311,6 @@ int tcp_gro_complete(struct sk_buff *skb) > > > > if (skb->encapsulation) > > skb->inner_transport_header = skb->transport_header; > > - > > - return 0; > > } > > EXPORT_SYMBOL(tcp_gro_complete); > > tcp_gro_complete seems fairly trivial. Any reason not to make it an > inline and avoid another function call in the datapath? Probably, although it is a regular function call, not an indirect one. In the grand total of driver rx napi + GRO cost, saving a few cycles per GRO completed packet is quite small.
On 5/30/23 9:39 AM, Parav Pandit wrote: >> From: David Ahern <dsahern@kernel.org> >> Sent: Tuesday, May 30, 2023 11:26 AM >> >> On 5/29/23 7:44 AM, Parav Pandit wrote: >>> diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index >>> 45dda7889387..88f9b0081ee7 100644 >>> --- a/net/ipv4/tcp_offload.c >>> +++ b/net/ipv4/tcp_offload.c >>> @@ -296,7 +296,7 @@ struct sk_buff *tcp_gro_receive(struct list_head >> *head, struct sk_buff *skb) >>> return pp; >>> } >>> >>> -int tcp_gro_complete(struct sk_buff *skb) >>> +void tcp_gro_complete(struct sk_buff *skb) >>> { >>> struct tcphdr *th = tcp_hdr(skb); >>> >>> @@ -311,8 +311,6 @@ int tcp_gro_complete(struct sk_buff *skb) >>> >>> if (skb->encapsulation) >>> skb->inner_transport_header = skb->transport_header; >>> - >>> - return 0; >>> } >>> EXPORT_SYMBOL(tcp_gro_complete); >> >> tcp_gro_complete seems fairly trivial. Any reason not to make it an inline and >> avoid another function call in the datapath? >> > Sounds good to me. > With inline it should mostly improve the perf, but I do not have any of the 3 adapters which are calling this API to show perf results. > > Since, it is a different change touching the performance, I prefer to do follow up patch that bnx owners can test. > Is that ok? sure.
On Tue, 30 May 2023 17:48:22 +0200 Eric Dumazet wrote: > > tcp_gro_complete seems fairly trivial. Any reason not to make it an > > inline and avoid another function call in the datapath? > > Probably, although it is a regular function call, not an indirect one. > > In the grand total of driver rx napi + GRO cost, saving a few cycles > per GRO completed packet is quite small. IOW please make sure you include the performance analysis quantifying the win, if you want to make this a static inline. Or let us know if the patch is good as is, I'm keeping it in pw for now.
On 5/30/23 1:39 PM, Jakub Kicinski wrote: > On Tue, 30 May 2023 17:48:22 +0200 Eric Dumazet wrote: >>> tcp_gro_complete seems fairly trivial. Any reason not to make it an >>> inline and avoid another function call in the datapath? >> >> Probably, although it is a regular function call, not an indirect one. >> >> In the grand total of driver rx napi + GRO cost, saving a few cycles >> per GRO completed packet is quite small. > > IOW please make sure you include the performance analysis quantifying > the win, if you want to make this a static inline. Or let us know if > the patch is good as is, I'm keeping it in pw for now. I am not suggesting holding up this patch; just constantly looking for these little savings here and there to keep lowering the overhead. 100G, 1500 MTU, line rate is 8.3M pps so GRO wise that would be ~180k fewer function calls.
On Wed, May 31, 2023 at 12:36 AM David Ahern <dsahern@kernel.org> wrote: > > On 5/30/23 1:39 PM, Jakub Kicinski wrote: > > On Tue, 30 May 2023 17:48:22 +0200 Eric Dumazet wrote: > >>> tcp_gro_complete seems fairly trivial. Any reason not to make it an > >>> inline and avoid another function call in the datapath? > >> > >> Probably, although it is a regular function call, not an indirect one. > >> > >> In the grand total of driver rx napi + GRO cost, saving a few cycles > >> per GRO completed packet is quite small. > > > > IOW please make sure you include the performance analysis quantifying > > the win, if you want to make this a static inline. Or let us know if > > the patch is good as is, I'm keeping it in pw for now. > > I am not suggesting holding up this patch; just constantly looking for > these little savings here and there to keep lowering the overhead. > > 100G, 1500 MTU, line rate is 8.3M pps so GRO wise that would be ~180k > fewer function calls. Here with 4K MTU, this is called 67k per second An __skb_put() instead of skb_put() in a driver (eg mlx5e_build_linear_skb()) would have 45x more impact, and would still be noise.
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Mon, 29 May 2023 16:44:30 +0300 you wrote: > tcp_gro_complete() function only updates the skb fields related to GRO > and it always returns zero. All the 3 drivers which are using it > do not check for the return value either. > > Change it to return void instead which simplifies its callers as > error handing becomes unnecessary. > > [...] Here is the summary with links: - [net-next] net: Make gro complete function to return void https://git.kernel.org/netdev/net-next/c/b1f2abcf817d You are awesome, thank you!
> From: Eric Dumazet <edumazet@google.com> > Sent: Wednesday, May 31, 2023 12:39 AM > > On Wed, May 31, 2023 at 12:36 AM David Ahern <dsahern@kernel.org> wrote: > > > > On 5/30/23 1:39 PM, Jakub Kicinski wrote: > > > On Tue, 30 May 2023 17:48:22 +0200 Eric Dumazet wrote: > > >>> tcp_gro_complete seems fairly trivial. Any reason not to make it > > >>> an inline and avoid another function call in the datapath? > > >> > > >> Probably, although it is a regular function call, not an indirect one. > > >> > > >> In the grand total of driver rx napi + GRO cost, saving a few > > >> cycles per GRO completed packet is quite small. > > > > > > IOW please make sure you include the performance analysis > > > quantifying the win, if you want to make this a static inline. Or > > > let us know if the patch is good as is, I'm keeping it in pw for now. > > > > I am not suggesting holding up this patch; just constantly looking for > > these little savings here and there to keep lowering the overhead. > > > > 100G, 1500 MTU, line rate is 8.3M pps so GRO wise that would be ~180k > > fewer function calls. > > Here with 4K MTU, this is called 67k per second > > An __skb_put() instead of skb_put() in a driver (eg mlx5e_build_linear_skb()) > would have 45x more impact, and would still be noise. Thanks, Eric, for the suggestion, will evaluate with Tariq to use __skb_put().
diff --git a/include/net/tcp.h b/include/net/tcp.h index 0b755988e20c..14fa716cac50 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -2041,7 +2041,7 @@ INDIRECT_CALLABLE_DECLARE(int tcp4_gro_complete(struct sk_buff *skb, int thoff)) INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp4_gro_receive(struct list_head *head, struct sk_buff *skb)); INDIRECT_CALLABLE_DECLARE(int tcp6_gro_complete(struct sk_buff *skb, int thoff)); INDIRECT_CALLABLE_DECLARE(struct sk_buff *tcp6_gro_receive(struct list_head *head, struct sk_buff *skb)); -int tcp_gro_complete(struct sk_buff *skb); +void tcp_gro_complete(struct sk_buff *skb); void __tcp_v4_send_check(struct sk_buff *skb, __be32 saddr, __be32 daddr); diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index 45dda7889387..88f9b0081ee7 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -296,7 +296,7 @@ struct sk_buff *tcp_gro_receive(struct list_head *head, struct sk_buff *skb) return pp; } -int tcp_gro_complete(struct sk_buff *skb) +void tcp_gro_complete(struct sk_buff *skb) { struct tcphdr *th = tcp_hdr(skb); @@ -311,8 +311,6 @@ int tcp_gro_complete(struct sk_buff *skb) if (skb->encapsulation) skb->inner_transport_header = skb->transport_header; - - return 0; } EXPORT_SYMBOL(tcp_gro_complete); @@ -342,7 +340,8 @@ INDIRECT_CALLABLE_SCOPE int tcp4_gro_complete(struct sk_buff *skb, int thoff) if (NAPI_GRO_CB(skb)->is_atomic) skb_shinfo(skb)->gso_type |= SKB_GSO_TCP_FIXEDID; - return tcp_gro_complete(skb); + tcp_gro_complete(skb); + return 0; } static const struct net_offload tcpv4_offload = { diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c index 39db5a226855..bf0c957e4b5e 100644 --- a/net/ipv6/tcpv6_offload.c +++ b/net/ipv6/tcpv6_offload.c @@ -36,7 +36,8 @@ INDIRECT_CALLABLE_SCOPE int tcp6_gro_complete(struct sk_buff *skb, int thoff) &iph->daddr, 0); skb_shinfo(skb)->gso_type |= SKB_GSO_TCPV6; - return tcp_gro_complete(skb); + tcp_gro_complete(skb); + return 0; } static struct sk_buff *tcp6_gso_segment(struct sk_buff *skb,
tcp_gro_complete() function only updates the skb fields related to GRO and it always returns zero. All the 3 drivers which are using it do not check for the return value either. Change it to return void instead which simplifies its callers as error handing becomes unnecessary. Signed-off-by: Parav Pandit <parav@nvidia.com> --- include/net/tcp.h | 2 +- net/ipv4/tcp_offload.c | 7 +++---- net/ipv6/tcpv6_offload.c | 3 ++- 3 files changed, 6 insertions(+), 6 deletions(-)