[net] udp: fix memory schedule error

  From: Jason Xing <kernelxing@tencent.com>

Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
and sk_rmem_schedule() errors"):

"If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
we want to allocate 1 byte more (rounded up to one page),
instead of 150001"

After applied this patch, we could avoid receive path scheduling
extra amount of memory.

Fixes: f970bd9e3a06 ("udp: implement memory accounting helpers")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/ipv4/udp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
  

On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> and sk_rmem_schedule() errors"):
> 
> "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> we want to allocate 1 byte more (rounded up to one page),
> instead of 150001"

I'm wondering if this would cause measurable (even small) performance
regression? Specifically under high packet rate, with BH and user-space
processing happening on different CPUs.

Could you please provide the relevant performance figures?

Thanks!

Paolo
  

On Tue, Feb 21, 2023 at 1:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > and sk_rmem_schedule() errors"):
> >
> > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > we want to allocate 1 byte more (rounded up to one page),
> > instead of 150001"
>
> I'm wondering if this would cause measurable (even small) performance
> regression? Specifically under high packet rate, with BH and user-space
> processing happening on different CPUs.
>
> Could you please provide the relevant performance figures?
>
> Thanks!
>
> Paolo
>

Probably not a big deal.

TCP skb truesize can easily reach 180 KB, but for UDP it's 99% below
or close to a 4K page.

I doubt this change makes any difference for UDP.
  

On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > and sk_rmem_schedule() errors"):
> >
> > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > we want to allocate 1 byte more (rounded up to one page),
> > instead of 150001"
>
> I'm wondering if this would cause measurable (even small) performance
> regression? Specifically under high packet rate, with BH and user-space
> processing happening on different CPUs.
>
> Could you please provide the relevant performance figures?

Sure, I've done some basic tests on my machine as below.

Environment: 16 cpus, 60G memory
Server: run "iperf3 -s -p [port]" command and start 500 processes.
Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.

Running such tests makes sure that the util output of every cpu is
higher than 15% which is observed through top command.

Here're some before/after numbers by using the "sar -n DEV 10 2" command.
Before: rxpck/s 2000, txpck/s 2000, rxkB/s 64054.69, txkB/s 64054.69
After: rxpck/s 2000, txpck/s 2000, rxkB/s 64054.58, txkB/s 64054.58
So I don't see much impact on the results.

In theory, I have no clue about why it could cause some regression?
Maybe the memory allocation is not that enough compared to the
original code?

Thanks,
Jason

>
> Thanks!
>
> Paolo
>
  

On Tue, Feb 21, 2023 at 8:35 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Feb 21, 2023 at 1:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > and sk_rmem_schedule() errors"):
> > >
> > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > we want to allocate 1 byte more (rounded up to one page),
> > > instead of 150001"
> >
> > I'm wondering if this would cause measurable (even small) performance
> > regression? Specifically under high packet rate, with BH and user-space
> > processing happening on different CPUs.
> >
> > Could you please provide the relevant performance figures?
> >
> > Thanks!
> >
> > Paolo
> >
>
> Probably not a big deal.
>

> TCP skb truesize can easily reach 180 KB, but for UDP it's 99% below
> or close to a 4K page.

Yes.

>
> I doubt this change makes any difference for UDP.

Based on my understanding of this part, it could not neither cause
much regression nor improve much performance. I think what you've done
to the TCP stack is the right way to go so the UDP can probably follow
this.
Calculating extra memory is a little bit odd because we actually don't
need that much when receiving skb everytime.

Thanks,
Jason
  

On Tue, 2023-02-21 at 21:39 +0800, Jason Xing wrote:
> On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > > 
> > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > and sk_rmem_schedule() errors"):
> > > 
> > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > we want to allocate 1 byte more (rounded up to one page),
> > > instead of 150001"
> > 
> > I'm wondering if this would cause measurable (even small) performance
> > regression? Specifically under high packet rate, with BH and user-space
> > processing happening on different CPUs.
> > 
> > Could you please provide the relevant performance figures?
> 
> Sure, I've done some basic tests on my machine as below.
> 
> Environment: 16 cpus, 60G memory
> Server: run "iperf3 -s -p [port]" command and start 500 processes.
> Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.

Just for the records, with the above command each process will send
pkts at 1mbs - not very relevant performance wise.

Instead you could do:

taskset 0x2 iperf -s &
iperf -u -c 127.0.0.1 -b 0 -l 64


> In theory, I have no clue about why it could cause some regression?
> Maybe the memory allocation is not that enough compared to the
> original code?

As Eric noted, for UDP traffic, due to the expected average packet
size, sk_forward_alloc is touched quite frequently, both with and
without this patch, so there is little chance it will have any
performance impact.

Cheers,

Paolo
  

On Tue, Feb 21, 2023 at 10:46 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2023-02-21 at 21:39 +0800, Jason Xing wrote:
> > On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > > and sk_rmem_schedule() errors"):
> > > >
> > > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > > we want to allocate 1 byte more (rounded up to one page),
> > > > instead of 150001"
> > >
> > > I'm wondering if this would cause measurable (even small) performance
> > > regression? Specifically under high packet rate, with BH and user-space
> > > processing happening on different CPUs.
> > >
> > > Could you please provide the relevant performance figures?
> >
> > Sure, I've done some basic tests on my machine as below.
> >
> > Environment: 16 cpus, 60G memory
> > Server: run "iperf3 -s -p [port]" command and start 500 processes.
> > Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.
>
> Just for the records, with the above command each process will send
> pkts at 1mbs - not very relevant performance wise.
>
> Instead you could do:
>

> taskset 0x2 iperf -s &
> iperf -u -c 127.0.0.1 -b 0 -l 64
>

Thanks for your guidance.

Here're some numbers according to what you suggested, which I tested
several times.
----------|IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s
Before: lo 411073.41 411073.41  36932.38  36932.38
After:   lo 410308.73 410308.73  36863.81  36863.81

Above is one of many results which does not mean that the original
code absolutely outperforms.
The output is not that constant and stable, I think.

Please help me review those numbers.

>
> > In theory, I have no clue about why it could cause some regression?
> > Maybe the memory allocation is not that enough compared to the
> > original code?
>
> As Eric noted, for UDP traffic, due to the expected average packet
> size, sk_forward_alloc is touched quite frequently, both with and
> without this patch, so there is little chance it will have any
> performance impact.

Well, I see.

Thanks,
Jason

>
> Cheers,
>
> Paolo
>
  

On Tue, Feb 21, 2023 at 11:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
>
> On Tue, Feb 21, 2023 at 10:46 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Tue, 2023-02-21 at 21:39 +0800, Jason Xing wrote:
> > > On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > >
> > > > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > > > and sk_rmem_schedule() errors"):
> > > > >
> > > > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > > > we want to allocate 1 byte more (rounded up to one page),
> > > > > instead of 150001"
> > > >
> > > > I'm wondering if this would cause measurable (even small) performance
> > > > regression? Specifically under high packet rate, with BH and user-space
> > > > processing happening on different CPUs.
> > > >
> > > > Could you please provide the relevant performance figures?
> > >
> > > Sure, I've done some basic tests on my machine as below.
> > >
> > > Environment: 16 cpus, 60G memory
> > > Server: run "iperf3 -s -p [port]" command and start 500 processes.
> > > Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.
> >
> > Just for the records, with the above command each process will send
> > pkts at 1mbs - not very relevant performance wise.
> >
> > Instead you could do:
> >
>
> > taskset 0x2 iperf -s &
> > iperf -u -c 127.0.0.1 -b 0 -l 64
> >
>
> Thanks for your guidance.
>
> Here're some numbers according to what you suggested, which I tested
> several times.
> ----------|IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s
> Before: lo 411073.41 411073.41  36932.38  36932.38
> After:   lo 410308.73 410308.73  36863.81  36863.81
>
> Above is one of many results which does not mean that the original
> code absolutely outperforms.
> The output is not that constant and stable, I think.

Today, I ran the same test on other servers, it looks the same as
above. Those results fluctuate within ~2%.

Oh, one more thing I forgot to say is the output of iperf itself which
doesn't show any difference.
Before: Bitrate is 211 - 212 Mbits/sec
After: Bitrate is 211 - 212 Mbits/sec
So this result is relatively constant especially if we keep running
the test over 2 minutes.

Jason

>
> Please help me review those numbers.
>
> >
> > > In theory, I have no clue about why it could cause some regression?
> > > Maybe the memory allocation is not that enough compared to the
> > > original code?
> >
> > As Eric noted, for UDP traffic, due to the expected average packet
> > size, sk_forward_alloc is touched quite frequently, both with and
> > without this patch, so there is little chance it will have any
> > performance impact.
>
> Well, I see.
>
> Thanks,
> Jason
>
> >
> > Cheers,
> >
> > Paolo
> >
  

On Wed, 2023-02-22 at 11:47 +0800, Jason Xing wrote:
> On Tue, Feb 21, 2023 at 11:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > 
> > On Tue, Feb 21, 2023 at 10:46 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > 
> > > On Tue, 2023-02-21 at 21:39 +0800, Jason Xing wrote:
> > > > On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > > 
> > > > > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > > 
> > > > > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > > > > and sk_rmem_schedule() errors"):
> > > > > > 
> > > > > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > > > > we want to allocate 1 byte more (rounded up to one page),
> > > > > > instead of 150001"
> > > > > 
> > > > > I'm wondering if this would cause measurable (even small) performance
> > > > > regression? Specifically under high packet rate, with BH and user-space
> > > > > processing happening on different CPUs.
> > > > > 
> > > > > Could you please provide the relevant performance figures?
> > > > 
> > > > Sure, I've done some basic tests on my machine as below.
> > > > 
> > > > Environment: 16 cpus, 60G memory
> > > > Server: run "iperf3 -s -p [port]" command and start 500 processes.
> > > > Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.
> > > 
> > > Just for the records, with the above command each process will send
> > > pkts at 1mbs - not very relevant performance wise.
> > > 
> > > Instead you could do:
> > > 
> > 
> > > taskset 0x2 iperf -s &
> > > iperf -u -c 127.0.0.1 -b 0 -l 64
> > > 
> > 
> > Thanks for your guidance.
> > 
> > Here're some numbers according to what you suggested, which I tested
> > several times.
> > ----------|IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s
> > Before: lo 411073.41 411073.41  36932.38  36932.38
> > After:   lo 410308.73 410308.73  36863.81  36863.81
> > 
> > Above is one of many results which does not mean that the original
> > code absolutely outperforms.
> > The output is not that constant and stable, I think.
> 
> Today, I ran the same test on other servers, it looks the same as
> above. Those results fluctuate within ~2%.
> 
> Oh, one more thing I forgot to say is the output of iperf itself which
> doesn't show any difference.
> Before: Bitrate is 211 - 212 Mbits/sec
> After: Bitrate is 211 - 212 Mbits/sec
> So this result is relatively constant especially if we keep running
> the test over 2 minutes.

Thanks for the testing. My personal take on this one is that is more a
refactor than a bug fix - as the amount forward allocated memory should
always be negligible for UDP. 

Still it could make sense keep the accounting schema consistent across
different protocols. I suggest to repost for net-next, when it will re-
open, additionally introducing __sk_mem_schedule() usage to avoid code
duplication.

Thanks,

Paolo
  

On Thu, Feb 23, 2023 at 4:39 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Wed, 2023-02-22 at 11:47 +0800, Jason Xing wrote:
> > On Tue, Feb 21, 2023 at 11:46 PM Jason Xing <kerneljasonxing@gmail.com> wrote:
> > >
> > > On Tue, Feb 21, 2023 at 10:46 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > >
> > > > On Tue, 2023-02-21 at 21:39 +0800, Jason Xing wrote:
> > > > > On Tue, Feb 21, 2023 at 8:27 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, 2023-02-21 at 19:03 +0800, Jason Xing wrote:
> > > > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > > > >
> > > > > > > Quoting from the commit 7c80b038d23e ("net: fix sk_wmem_schedule()
> > > > > > > and sk_rmem_schedule() errors"):
> > > > > > >
> > > > > > > "If sk->sk_forward_alloc is 150000, and we need to schedule 150001 bytes,
> > > > > > > we want to allocate 1 byte more (rounded up to one page),
> > > > > > > instead of 150001"
> > > > > >
> > > > > > I'm wondering if this would cause measurable (even small) performance
> > > > > > regression? Specifically under high packet rate, with BH and user-space
> > > > > > processing happening on different CPUs.
> > > > > >
> > > > > > Could you please provide the relevant performance figures?
> > > > >
> > > > > Sure, I've done some basic tests on my machine as below.
> > > > >
> > > > > Environment: 16 cpus, 60G memory
> > > > > Server: run "iperf3 -s -p [port]" command and start 500 processes.
> > > > > Client: run "iperf3 -u -c 127.0.0.1 -p [port]" command and start 500 processes.
> > > >
> > > > Just for the records, with the above command each process will send
> > > > pkts at 1mbs - not very relevant performance wise.
> > > >
> > > > Instead you could do:
> > > >
> > >
> > > > taskset 0x2 iperf -s &
> > > > iperf -u -c 127.0.0.1 -b 0 -l 64
> > > >
> > >
> > > Thanks for your guidance.
> > >
> > > Here're some numbers according to what you suggested, which I tested
> > > several times.
> > > ----------|IFACE   rxpck/s   txpck/s    rxkB/s    txkB/s
> > > Before: lo 411073.41 411073.41  36932.38  36932.38
> > > After:   lo 410308.73 410308.73  36863.81  36863.81
> > >
> > > Above is one of many results which does not mean that the original
> > > code absolutely outperforms.
> > > The output is not that constant and stable, I think.
> >
> > Today, I ran the same test on other servers, it looks the same as
> > above. Those results fluctuate within ~2%.
> >
> > Oh, one more thing I forgot to say is the output of iperf itself which
> > doesn't show any difference.
> > Before: Bitrate is 211 - 212 Mbits/sec
> > After: Bitrate is 211 - 212 Mbits/sec
> > So this result is relatively constant especially if we keep running
> > the test over 2 minutes.
>
> Thanks for the testing. My personal take on this one is that is more a
> refactor than a bug fix - as the amount forward allocated memory should
> always be negligible for UDP.
>

> Still it could make sense keep the accounting schema consistent across
> different protocols. I suggest to repost for net-next, when it will re-
> open, additionally introducing __sk_mem_schedule() usage to avoid code
> duplication.
>

Thanks for the review. I will replace this part with
__sk_mem_schedule() and then repost it after Mar 6th.

Thanks,
Jason

> Thanks,
>
> Paolo
>