Re: [PATCH net-next 0/4] net: rps/rfs improvements

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Aiden Leong]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

Hi Eric,

I hope my email is not too off-topic but I have some confusion on how 
maintainers and should react to other people's work.

In short, you are stealing Jason's idea&&work by rewriting your implementation 
which not that ethical. Since your patch is based on his work, but you only 
sign-off it by your name, it's possible to raise lawsuit between Tencent and 
Linux community or Google.

I'm here to provoke a conflict because we know your name in this area and I'd 
to show my respect to you but I do have this kind of confusion in my mind and 
wish you could explain about it.

There's another story you or Tom Herbert may be interested in: I was working 
on Foo Over UDP and have implemented the missing features in the previous 
company I worked for. The proposal to contribute to the upstream community was 
rejected later by our boss for unhappy events very similar to this one.

Aiden Leong

> Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> unneeded NET_RX_SOFTIRQ raises: [1], [2]
> 
> This is quite complex to implement properly. I chose to implement
> the idea, and added a similar optimization in ____napi_schedule()
> 
> Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> invocations.
> 
> While this had no impact on throughput or cpu costs on this synthetic
> benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
> can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
> This can have a latency impact on stressed hosts.
> 
> [1] https://lore.kernel.org/lkml/20230325152417.5403-1-> 
kerneljasonxing@gmail.com/
> [2] https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gmail.com/
> 
> 
> Eric Dumazet (4):
>   net: napi_schedule_rps() cleanup
>   net: add softnet_data.in_net_rx_action
>   net: optimize napi_schedule_rps()
>   net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
> 
>  include/linux/netdevice.h |  1 +
>  net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
>  2 files changed, 37 insertions(+), 10 deletions(-)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Aiden Leong]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Wednesday, March 29, 2023 9:17:06 PM CST Aiden Leong wrote:
> Hi Eric,
> 
> I hope my email is not too off-topic but I have some confusion on how
> maintainers and should react to other people's work.
> 
> In short, you are stealing Jason's idea&&work by rewriting your
> implementation which not that ethical. Since your patch is based on his
> work, but you only sign-off it by your name, it's possible to raise lawsuit
> between Tencent and Linux community or Google.
> 
> I'm here to provoke a conflict because we know your name in this area and
> I'd to show my respect to you but I do have this kind of confusion in my
> mind and wish you could explain about it.
> 
Typo: I'm here NOT to provoke a conflict 
> There's another story you or Tom Herbert may be interested in: I was working
> on Foo Over UDP and have implemented the missing features in the previous
> company I worked for. The proposal to contribute to the upstream community
> was rejected later by our boss for unhappy events very similar to this one.
> 
> Aiden Leong
> 
> > Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> > unneeded NET_RX_SOFTIRQ raises: [1], [2]
> > 
> > This is quite complex to implement properly. I chose to implement
> > the idea, and added a similar optimization in ____napi_schedule()
> > 
> > Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> > I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> > invocations.
> > 
> > While this had no impact on throughput or cpu costs on this synthetic
> > benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
> > can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
> > This can have a latency impact on stressed hosts.
> > 
> > [1] https://lore.kernel.org/lkml/20230325152417.5403-1->
> 
> kerneljasonxing@gmail.com/
> 
> > [2]
> > https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gma
> > il.com/> 
> > Eric Dumazet (4):
> >   net: napi_schedule_rps() cleanup
> >   net: add softnet_data.in_net_rx_action
> >   net: optimize napi_schedule_rps()
> >   net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
> >  
> >  include/linux/netdevice.h |  1 +
> >  net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
> >  2 files changed, 37 insertions(+), 10 deletions(-)


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Eric Dumazet]

On Wed, Mar 29, 2023 at 3:18 PM Aiden Leong <aiden.leong@aibsd.com> wrote:
>
> On Wednesday, March 29, 2023 9:17:06 PM CST Aiden Leong wrote:
> > Hi Eric,
> >
> > I hope my email is not too off-topic but I have some confusion on how
> > maintainers and should react to other people's work.
> >
> > In short, you are stealing Jason's idea&&work by rewriting your
> > implementation which not that ethical. Since your patch is based on his
> > work, but you only sign-off it by your name, it's possible to raise lawsuit
> > between Tencent and Linux community or Google.

Seriously ?

I really gave enough credit to Jason's work, it seems you missed it.

I am quite tired of reviewing patches, and giving ideas of how to
improve things,
then having no credits for my work.

After my feedback on v1, seeing a quite silly v2, obviously not tested,
and with no numbers shown, I wanted to see how hard the complete
implementation would be.

This needed full understanding of RPS and RFS, not only an "idea" and
wrong claims about fixing
a "bug" in the initial implementation which was just fine.

Also, results are theoretical at this stage,I added numbers in the cover letter
showing the impact was tiny or not even mesurable.

I sent a series of 4 patches, Jason work on the 3rd one has been
completely documented.

If Jason managers are not able to see the credit in the patch series
(and cover letter),
this is their problem, not mine.

Also, my contributions to linux do not represent views of my employer,
this should be obvious.



> >
> > I'm here to provoke a conflict because we know your name in this area and
> > I'd to show my respect to you but I do have this kind of confusion in my
> > mind and wish you could explain about it.
> >
> Typo: I'm here NOT to provoke a conflict
> > There's another story you or Tom Herbert may be interested in: I was working
> > on Foo Over UDP and have implemented the missing features in the previous
> > company I worked for. The proposal to contribute to the upstream community
> > was rejected later by our boss for unhappy events very similar to this one.
> >
> > Aiden Leong
> >
> > > Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> > > unneeded NET_RX_SOFTIRQ raises: [1], [2]
> > >
> > > This is quite complex to implement properly. I chose to implement
> > > the idea, and added a similar optimization in ____napi_schedule()
> > >
> > > Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> > > I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> > > invocations.
> > >
> > > While this had no impact on throughput or cpu costs on this synthetic
> > > benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
> > > can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
> > > This can have a latency impact on stressed hosts.
> > >
> > > [1] https://lore.kernel.org/lkml/20230325152417.5403-1->
> >
> > kerneljasonxing@gmail.com/
> >
> > > [2]
> > > https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gma
> > > il.com/>
> > > Eric Dumazet (4):
> > >   net: napi_schedule_rps() cleanup
> > >   net: add softnet_data.in_net_rx_action
> > >   net: optimize napi_schedule_rps()
> > >   net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
> > >
> > >  include/linux/netdevice.h |  1 +
> > >  net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
> > >  2 files changed, 37 insertions(+), 10 deletions(-)
>

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Jason Xing]

On Wed, Mar 29, 2023 at 11:36 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Wed, Mar 29, 2023 at 3:18 PM Aiden Leong <aiden.leong@aibsd.com> wrote:
> >
> > On Wednesday, March 29, 2023 9:17:06 PM CST Aiden Leong wrote:
> > > Hi Eric,
> > >
> > > I hope my email is not too off-topic but I have some confusion on how
> > > maintainers and should react to other people's work.
> > >
> > > In short, you are stealing Jason's idea&&work by rewriting your
> > > implementation which not that ethical. Since your patch is based on his
> > > work, but you only sign-off it by your name, it's possible to raise lawsuit
> > > between Tencent and Linux community or Google.
>
> Seriously ?
>
> I really gave enough credit to Jason's work, it seems you missed it.
>
> I am quite tired of reviewing patches, and giving ideas of how to
> improve things,
> then having no credits for my work.
>
> After my feedback on v1, seeing a quite silly v2, obviously not tested,
> and with no numbers shown, I wanted to see how hard the complete
> implementation would be.

At first glance, I really don't have any interest in commenting on this.

However, those words 'silly' 'not tested' 'no numbers' make me feel
very uncomfortable. Actually I did all of them. What you said makes
others think I'm like a fool who just does not have any knowledge
about this and proposed one idea with no foundation (out of thin air).
You know that it's not real but I don't know why you're using these
terrible words in public?
In fact, last night in our private email exchange, you said "I need to
be convinced" to me and then it was me who listed nearly every step to
prove how it can have impacts on latency with high load at 3:00 AM.

Well, I wouldn't like to see any further conflicts because of this.
Let's stop here and focus on this patch series and then move on, shall
we?

>
> This needed full understanding of RPS and RFS, not only an "idea" and
> wrong claims about fixing
> a "bug" in the initial implementation which was just fine.
>
> Also, results are theoretical at this stage,I added numbers in the cover letter
> showing the impact was tiny or not even mesurable.
>
> I sent a series of 4 patches, Jason work on the 3rd one has been
> completely documented.
>
> If Jason managers are not able to see the credit in the patch series
> (and cover letter),
> this is their problem, not mine.
>
> Also, my contributions to linux do not represent views of my employer,
> this should be obvious.
>
>
>
> > >
> > > I'm here to provoke a conflict because we know your name in this area and
> > > I'd to show my respect to you but I do have this kind of confusion in my
> > > mind and wish you could explain about it.
> > >
> > Typo: I'm here NOT to provoke a conflict
> > > There's another story you or Tom Herbert may be interested in: I was working
> > > on Foo Over UDP and have implemented the missing features in the previous
> > > company I worked for. The proposal to contribute to the upstream community
> > > was rejected later by our boss for unhappy events very similar to this one.
> > >
> > > Aiden Leong
> > >
> > > > Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> > > > unneeded NET_RX_SOFTIRQ raises: [1], [2]
> > > >
> > > > This is quite complex to implement properly. I chose to implement
> > > > the idea, and added a similar optimization in ____napi_schedule()
> > > >
> > > > Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> > > > I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> > > > invocations.
> > > >
> > > > While this had no impact on throughput or cpu costs on this synthetic
> > > > benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
> > > > can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
> > > > This can have a latency impact on stressed hosts.
> > > >
> > > > [1] https://lore.kernel.org/lkml/20230325152417.5403-1->
> > >
> > > kerneljasonxing@gmail.com/
> > >
> > > > [2]
> > > > https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gma
> > > > il.com/>
> > > > Eric Dumazet (4):
> > > >   net: napi_schedule_rps() cleanup
> > > >   net: add softnet_data.in_net_rx_action
> > > >   net: optimize napi_schedule_rps()
> > > >   net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
> > > >
> > > >  include/linux/netdevice.h |  1 +
> > > >  net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
> > > >  2 files changed, 37 insertions(+), 10 deletions(-)
> >

[PATCH net-next 0/4] net: rps/rfs improvements

[Eric Dumazet]

Jason Xing attempted to optimize napi_schedule_rps() by avoiding
unneeded NET_RX_SOFTIRQ raises: [1], [2]

This is quite complex to implement properly. I chose to implement
the idea, and added a similar optimization in ____napi_schedule()

Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
invocations.

While this had no impact on throughput or cpu costs on this synthetic
benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
This can have a latency impact on stressed hosts.

[1] https://lore.kernel.org/lkml/20230325152417.5403-1-kerneljasonxing@gmail.com/
[2] https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gmail.com/


Eric Dumazet (4):
  net: napi_schedule_rps() cleanup
  net: add softnet_data.in_net_rx_action
  net: optimize napi_schedule_rps()
  net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ

 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
 2 files changed, 37 insertions(+), 10 deletions(-)

-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Jason Xing]

On Wed, Mar 29, 2023 at 7:53 AM Eric Dumazet <edumazet@google.com> wrote:
>
> Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> unneeded NET_RX_SOFTIRQ raises: [1], [2]
>
> This is quite complex to implement properly. I chose to implement
> the idea, and added a similar optimization in ____napi_schedule()
>
[...]
> Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> invocations.
>
> While this had no impact on throughput or cpu costs on this synthetic
> benchmark, we know that firing NET_RX_SOFTIRQ from softirq handler
> can force __do_softirq() to wakeup ksoftirqd when need_resched() is true.
> This can have a latency impact on stressed hosts.

Eric, nice work ! You got these numbers.

Could you also put this whole important description above into the 3/4
patch? I believe it is very useful information if any
readers/developers try to track this part through git blame. After
all, I spent a lot of time discovering this point. Thanks.

Otherwise it looks good to me. Please add:
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>

And I've done tests on this patchset. Please also add:
Tested-by: Jason Xing <kerneljasonxing@gmail.com>

Thanks!

>
> [1] https://lore.kernel.org/lkml/20230325152417.5403-1-kerneljasonxing@gmail.com/
> [2] https://lore.kernel.org/netdev/20230328142112.12493-1-kerneljasonxing@gmail.com/
>
>
> Eric Dumazet (4):
>   net: napi_schedule_rps() cleanup
>   net: add softnet_data.in_net_rx_action
>   net: optimize napi_schedule_rps()
>   net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
>
>  include/linux/netdevice.h |  1 +
>  net/core/dev.c            | 46 ++++++++++++++++++++++++++++++---------
>  2 files changed, 37 insertions(+), 10 deletions(-)
>
> --
> 2.40.0.348.gf938b09366-goog
>

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Jakub Kicinski]

On Tue, 28 Mar 2023 23:50:17 +0000 Eric Dumazet wrote:
> Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> invocations.

small clarification on the testing:

invocations == calls to net_rx_action()
or
invocations == calls to __raise_softirq_irqoff(NET_RX_SOFTIRQ)

?

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Eric Dumazet]

On Thu, Mar 30, 2023 at 5:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 28 Mar 2023 23:50:17 +0000 Eric Dumazet wrote:
> > Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> > I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> > invocations.
>
> small clarification on the testing:
>
> invocations == calls to net_rx_action()
> or
> invocations == calls to __raise_softirq_irqoff(NET_RX_SOFTIRQ)

This was from "grep NET_RX /proc/softirqs" (more exactly a tool
parsing /proc/softirqs)

So it should match the number of calls to net_rx_action(), but I can
double check if you want.

(I had a simple hack to enable/disable the optimizations with a hijacked sysctl)

Turn on/off them with

echo 1001 /proc/sys/net/core/netdev_max_backlog
<gather stats>
echo 1000 >/proc/sys/net/core/netdev_max_backlog
<gather stats>

diff --git a/net/core/dev.c b/net/core/dev.c
index 0c4b21291348d4558f036fb05842dab023f65dc3..f8c6fde6100c8e4812037bd070e11733409bd0a0
100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6653,7 +6653,7 @@ static __latent_entropy void
net_rx_action(struct softirq_action *h)
        LIST_HEAD(repoll);

 start:
-       sd->in_net_rx_action = true;
+       sd->in_net_rx_action = (netdev_max_backlog & 1);
        local_irq_disable();
        list_splice_init(&sd->poll_list, &list);
        local_irq_enable();

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Eric Dumazet]

On Thu, Mar 30, 2023 at 5:15 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Mar 30, 2023 at 5:04 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Tue, 28 Mar 2023 23:50:17 +0000 Eric Dumazet wrote:
> > > Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> > > I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> > > invocations.
> >
> > small clarification on the testing:
> >
> > invocations == calls to net_rx_action()
> > or
> > invocations == calls to __raise_softirq_irqoff(NET_RX_SOFTIRQ)
>
> This was from "grep NET_RX /proc/softirqs" (more exactly a tool
> parsing /proc/softirqs)
>
> So it should match the number of calls to net_rx_action(), but I can
> double check if you want.
>
> (I had a simple hack to enable/disable the optimizations with a hijacked sysctl)

Trace of real debug session, because numbers ;)
Old platform with two Intel(R) Xeon(R) Gold 6268L CPU @ 2.80GHz  (96
threads total)

600 tcp_rr flows

iroa23:/home/edumazet# cat /proc/sys/net/core/netdev_max_backlog
1000
iroa23:/home/edumazet# ./interrupts
hrtimer:99518 cal:2421783 timer:1034 sched:80505 rcu:7661 rps:2390765
net_tx:43 net_rx:3344637 eth1-rx:295134 eth1-tx:558933 eth1:854074
^C
iroa23:/home/edumazet# echo 1001 >/proc/sys/net/core/netdev_max_backlog
iroa23:/home/edumazet# ./interrupts
hrtimer:99545 cal:2358993 timer:1086 sched:77806 rcu:10928 rps:2419052
net_tx:21 net_rx:3016301 eth1-rx:294612 eth1-tx:560331 eth1:855083
^C

echo "(3344637 - 3016301)/3344637" | bc -ql
.09816790282473105452

-> ~10 % decrease.

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[Jakub Kicinski]

On Thu, 30 Mar 2023 05:39:35 +0200 Eric Dumazet wrote:
> > This was from "grep NET_RX /proc/softirqs" (more exactly a tool
> > parsing /proc/softirqs)
> >
> > So it should match the number of calls to net_rx_action(), but I can
> > double check if you want.
> >
> > (I had a simple hack to enable/disable the optimizations with a hijacked sysctl)  
> 
> Trace of real debug session, because numbers ;)
> Old platform with two Intel(R) Xeon(R) Gold 6268L CPU @ 2.80GHz  (96
> threads total)
> 
> 600 tcp_rr flows
> 
> iroa23:/home/edumazet# cat /proc/sys/net/core/netdev_max_backlog
> 1000
> iroa23:/home/edumazet# ./interrupts
> hrtimer:99518 cal:2421783 timer:1034 sched:80505 rcu:7661 rps:2390765
> net_tx:43 net_rx:3344637 eth1-rx:295134 eth1-tx:558933 eth1:854074
> ^C
> iroa23:/home/edumazet# echo 1001 >/proc/sys/net/core/netdev_max_backlog
> iroa23:/home/edumazet# ./interrupts
> hrtimer:99545 cal:2358993 timer:1086 sched:77806 rcu:10928 rps:2419052
> net_tx:21 net_rx:3016301 eth1-rx:294612 eth1-tx:560331 eth1:855083
> ^C
> 
> echo "(3344637 - 3016301)/3344637" | bc -ql
> .09816790282473105452

Very nice, thanks! (I didn't know about /proc/softirqs !)

Re: [PATCH net-next 0/4] net: rps/rfs improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue, 28 Mar 2023 23:50:17 +0000 you wrote:
> Jason Xing attempted to optimize napi_schedule_rps() by avoiding
> unneeded NET_RX_SOFTIRQ raises: [1], [2]
> 
> This is quite complex to implement properly. I chose to implement
> the idea, and added a similar optimization in ____napi_schedule()
> 
> Overall, in an intensive RPC workload, with 32 TX/RX queues with RFS
> I was able to observe a ~10% reduction of NET_RX_SOFTIRQ
> invocations.
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] net: napi_schedule_rps() cleanup
    https://git.kernel.org/netdev/net-next/c/8fcb76b934da
  - [net-next,2/4] net: add softnet_data.in_net_rx_action
    https://git.kernel.org/netdev/net-next/c/c59647c0dc67
  - [net-next,3/4] net: optimize napi_schedule_rps()
    https://git.kernel.org/netdev/net-next/c/821eba962d95
  - [net-next,4/4] net: optimize ____napi_schedule() to avoid extra NET_RX_SOFTIRQ
    https://git.kernel.org/netdev/net-next/c/8b43fd3d1d7d

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