| Message ID | 8834aadd89c1ebcbad32f591ea4d29c9f2684497.1689587539.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] udp: introduce and use indirect call wrapper for data ready() | expand |
Paolo Abeni wrote: > In most cases UDP sockets use the default data ready callback. > This patch Introduces and uses a specific indirect call wrapper for > such callback to avoid an indirect call in fastpath. > > The above gives small but measurable performance gain under UDP flood. Interesting. I recently wrote a patch to add indirect call wrappers around getfrag (ip_generic_getfrag), expecting that to improve UDP senders. Since it's an indirect call on each send call. Not sent, because I did not see measurable gains, at least with a udp_rr bench. > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > Note that this helper could be used for TCP, too. I did not send such > patch right away because in my tests the perf delta there is below the > noise level even in RR scenarios and the patch would be a little more > invasive - there are more sk_data_ready() invocation places. > --- > include/net/sock.h | 4 ++++ > net/ipv4/udp.c | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/include/net/sock.h b/include/net/sock.h > index 2eb916d1ff64..1b26dbecdcca 100644 > --- a/include/net/sock.h > +++ b/include/net/sock.h > @@ -2947,6 +2947,10 @@ static inline bool sk_dev_equal_l3scope(struct sock *sk, int dif) > } > > void sock_def_readable(struct sock *sk); > +static inline void sk_data_ready(struct sock *sk) > +{ > + INDIRECT_CALL_1(sk->sk_data_ready, sock_def_readable, sk); > +} > Why introduce a static inline in the header for this? To reuse it in other protocols later? > int sock_bindtoindex(struct sock *sk, int ifindex, bool lock_sk); > void sock_set_timestamp(struct sock *sk, int optname, bool valbool); > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c > index 42a96b3547c9..5aec1854b711 100644 > --- a/net/ipv4/udp.c > +++ b/net/ipv4/udp.c > @@ -1553,7 +1553,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) > spin_unlock(&list->lock); > > if (!sock_flag(sk, SOCK_DEAD)) > - sk->sk_data_ready(sk); > + sk_data_ready(sk); > > busylock_release(busy); > return 0; > -- > 2.41.0 >
On Mon, 2023-07-17 at 09:44 -0400, Willem de Bruijn wrote: > Paolo Abeni wrote: > > In most cases UDP sockets use the default data ready callback. > > This patch Introduces and uses a specific indirect call wrapper for > > such callback to avoid an indirect call in fastpath. > > > > The above gives small but measurable performance gain under UDP flood. > > Interesting. I recently wrote a patch to add indirect call wrappers > around getfrag (ip_generic_getfrag), expecting that to improve UDP > senders. Since it's an indirect call on each send call. Not sent, > because I did not see measurable gains, at least with a udp_rr bench. > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > Note that this helper could be used for TCP, too. I did not send such > > patch right away because in my tests the perf delta there is below the > > noise level even in RR scenarios and the patch would be a little more > > invasive - there are more sk_data_ready() invocation places. > > --- > > include/net/sock.h | 4 ++++ > > net/ipv4/udp.c | 2 +- > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/sock.h b/include/net/sock.h > > index 2eb916d1ff64..1b26dbecdcca 100644 > > --- a/include/net/sock.h > > +++ b/include/net/sock.h > > @@ -2947,6 +2947,10 @@ static inline bool sk_dev_equal_l3scope(struct sock *sk, int dif) > > } > > > > void sock_def_readable(struct sock *sk); > > +static inline void sk_data_ready(struct sock *sk) > > +{ > > + INDIRECT_CALL_1(sk->sk_data_ready, sock_def_readable, sk); > > +} > > > > Why introduce a static inline in the header for this? > > To reuse it in other protocols later? I originally thought about re-using it even for TCP, but showed no gain there. I think/hope there could be other users, and I found the code nicer this way ;) Cheers, Paolo
Paolo Abeni wrote: > On Mon, 2023-07-17 at 09:44 -0400, Willem de Bruijn wrote: > > Paolo Abeni wrote: > > > In most cases UDP sockets use the default data ready callback. > > > This patch Introduces and uses a specific indirect call wrapper for > > > such callback to avoid an indirect call in fastpath. > > > > > > The above gives small but measurable performance gain under UDP flood. > > > > Interesting. I recently wrote a patch to add indirect call wrappers > > around getfrag (ip_generic_getfrag), expecting that to improve UDP > > senders. Since it's an indirect call on each send call. Not sent, > > because I did not see measurable gains, at least with a udp_rr bench. > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > --- > > > Note that this helper could be used for TCP, too. I did not send such > > > patch right away because in my tests the perf delta there is below the > > > noise level even in RR scenarios and the patch would be a little more > > > invasive - there are more sk_data_ready() invocation places. > > > --- > > > include/net/sock.h | 4 ++++ > > > net/ipv4/udp.c | 2 +- > > > 2 files changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/net/sock.h b/include/net/sock.h > > > index 2eb916d1ff64..1b26dbecdcca 100644 > > > --- a/include/net/sock.h > > > +++ b/include/net/sock.h > > > @@ -2947,6 +2947,10 @@ static inline bool sk_dev_equal_l3scope(struct sock *sk, int dif) > > > } > > > > > > void sock_def_readable(struct sock *sk); > > > +static inline void sk_data_ready(struct sock *sk) > > > +{ > > > + INDIRECT_CALL_1(sk->sk_data_ready, sock_def_readable, sk); > > > +} > > > > > > > Why introduce a static inline in the header for this? > > > > To reuse it in other protocols later? > > I originally thought about re-using it even for TCP, but showed no gain > there. I think/hope there could be other users, and I found the code > nicer this way ;) Until there are other users I disagree. And maybe even then, as this is a single line function. It's more readable to see the actual code. That said, no other concerns from me, if no one else objects.
diff --git a/include/net/sock.h b/include/net/sock.h index 2eb916d1ff64..1b26dbecdcca 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2947,6 +2947,10 @@ static inline bool sk_dev_equal_l3scope(struct sock *sk, int dif) } void sock_def_readable(struct sock *sk); +static inline void sk_data_ready(struct sock *sk) +{ + INDIRECT_CALL_1(sk->sk_data_ready, sock_def_readable, sk); +} int sock_bindtoindex(struct sock *sk, int ifindex, bool lock_sk); void sock_set_timestamp(struct sock *sk, int optname, bool valbool); diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 42a96b3547c9..5aec1854b711 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1553,7 +1553,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb) spin_unlock(&list->lock); if (!sock_flag(sk, SOCK_DEAD)) - sk->sk_data_ready(sk); + sk_data_ready(sk); busylock_release(busy); return 0;
In most cases UDP sockets use the default data ready callback. This patch Introduces and uses a specific indirect call wrapper for such callback to avoid an indirect call in fastpath. The above gives small but measurable performance gain under UDP flood. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- Note that this helper could be used for TCP, too. I did not send such patch right away because in my tests the perf delta there is below the noise level even in RR scenarios and the patch would be a little more invasive - there are more sk_data_ready() invocation places. --- include/net/sock.h | 4 ++++ net/ipv4/udp.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)