[PATCH net-next 0/2] sctp: fully support memory accounting

[PATCH net-next 0/2] sctp: fully support memory accounting

[Xin Long]

sctp memory accounting is added in this patchset by using
these kernel APIs on send side:

  - sk_mem_charge()
  - sk_mem_uncharge()
  - sk_wmem_schedule()
  - sk_under_memory_pressure()
  - sk_mem_reclaim()

and these on receive side:

  - sk_mem_charge()
  - sk_mem_uncharge()
  - sk_rmem_schedule()
  - sk_under_memory_pressure()
  - sk_mem_reclaim()

With sctp memory accounting, we can limit the memory allocation by
either sysctl:

  # sysctl -w net.sctp.sctp_mem="10 20 50"

or cgroup:

  # echo $((8<<14)) > \
    /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes

When the socket is under memory pressure, the send side will block
and wait, while the receive side will renege or drop.

Xin Long (2):
  sctp: implement memory accounting on tx path
  sctp: implement memory accounting on rx path

 include/net/sctp/sctp.h |  2 +-
 net/sctp/sm_statefuns.c |  6 ++++--
 net/sctp/socket.c       | 10 ++++++++--
 net/sctp/ulpevent.c     | 19 ++++++++-----------
 net/sctp/ulpqueue.c     |  3 ++-
 5 files changed, 23 insertions(+), 17 deletions(-)

-- 
2.1.0

[PATCH net-next 1/2] sctp: implement memory accounting on tx path

[Xin Long]

Now when sending packets, sk_mem_charge() and sk_mem_uncharge() have been
used to set sk_forward_alloc. We just need to call sk_wmem_schedule() to
check if the allocated should be raised, and call sk_mem_reclaim() to
check if the allocated should be reduced when it's under memory pressure.

If sk_wmem_schedule() returns false, which means no memory is allowed to
allocate, it will block and wait for memory to become available.

Note different from tcp, sctp wait_for_buf happens before allocating any
skb, so memory accounting check is done with the whole msg_len before it
too.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 6140471..06c6f4a 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1913,7 +1913,10 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
 	if (sctp_wspace(asoc) < (int)msg_len)
 		sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc));
 
-	if (sctp_wspace(asoc) <= 0) {
+	if (sk_under_memory_pressure(sk))
+		sk_mem_reclaim(sk);
+
+	if (sctp_wspace(asoc) <= 0 || !sk_wmem_schedule(sk, msg_len)) {
 		timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 		err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
 		if (err)
@@ -8891,7 +8894,10 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
 			goto do_error;
 		if (signal_pending(current))
 			goto do_interrupted;
-		if ((int)msg_len <= sctp_wspace(asoc))
+		if (sk_under_memory_pressure(sk))
+			sk_mem_reclaim(sk);
+		if ((int)msg_len <= sctp_wspace(asoc) &&
+		    sk_wmem_schedule(sk, msg_len))
 			break;
 
 		/* Let another process have a go.  Since we are going
-- 
2.1.0

[PATCH net-next 2/2] sctp: implement memory accounting on rx path

[Xin Long]

sk_forward_alloc's updating is also done on rx path, but to be consistent
we change to use sk_mem_charge() in sctp_skb_set_owner_r().

In sctp_eat_data(), it's not enough to check sctp_memory_pressure only,
which doesn't work for mem_cgroup_sockets_enabled, so we change to use
sk_under_memory_pressure().

When it's under memory pressure, sk_mem_reclaim() and sk_rmem_schedule()
should be called on both RENEGE or CHUNK DELIVERY path exit the memory
pressure status as soon as possible.

Note that sk_rmem_schedule() is using datalen to make things easy there.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/sctp.h |  2 +-
 net/sctp/sm_statefuns.c |  6 ++++--
 net/sctp/ulpevent.c     | 19 ++++++++-----------
 net/sctp/ulpqueue.c     |  3 ++-
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 1d13ec3..eefdfa5 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -421,7 +421,7 @@ static inline void sctp_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 	/*
 	 * This mimics the behavior of skb_set_owner_r
 	 */
-	sk->sk_forward_alloc -= event->rmem_len;
+	sk_mem_charge(sk, event->rmem_len);
 }
 
 /* Tests if the list has one and only one entry. */
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index c9ae340..7dfc34b 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -6412,13 +6412,15 @@ static int sctp_eat_data(const struct sctp_association *asoc,
 	 * in sctp_ulpevent_make_rcvmsg will drop the frame if we grow our
 	 * memory usage too much
 	 */
-	if (*sk->sk_prot_creator->memory_pressure) {
+	if (sk_under_memory_pressure(sk)) {
 		if (sctp_tsnmap_has_gap(map) &&
 		    (sctp_tsnmap_get_ctsn(map) + 1) == tsn) {
 			pr_debug("%s: under pressure, reneging for tsn:%u\n",
 				 __func__, tsn);
 			deliver = SCTP_CMD_RENEGE;
-		 }
+		} else {
+			sk_mem_reclaim(sk);
+		}
 	}
 
 	/*
diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
index 8cb7d98..c2a7478 100644
--- a/net/sctp/ulpevent.c
+++ b/net/sctp/ulpevent.c
@@ -634,8 +634,9 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 						gfp_t gfp)
 {
 	struct sctp_ulpevent *event = NULL;
-	struct sk_buff *skb;
-	size_t padding, len;
+	struct sk_buff *skb = chunk->skb;
+	struct sock *sk = asoc->base.sk;
+	size_t padding, datalen;
 	int rx_count;
 
 	/*
@@ -646,15 +647,12 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 	if (asoc->ep->rcvbuf_policy)
 		rx_count = atomic_read(&asoc->rmem_alloc);
 	else
-		rx_count = atomic_read(&asoc->base.sk->sk_rmem_alloc);
+		rx_count = atomic_read(&sk->sk_rmem_alloc);
 
-	if (rx_count >= asoc->base.sk->sk_rcvbuf) {
+	datalen = ntohs(chunk->chunk_hdr->length);
 
-		if ((asoc->base.sk->sk_userlocks & SOCK_RCVBUF_LOCK) ||
-		    (!sk_rmem_schedule(asoc->base.sk, chunk->skb,
-				       chunk->skb->truesize)))
-			goto fail;
-	}
+	if (rx_count >= sk->sk_rcvbuf || !sk_rmem_schedule(sk, skb, datalen))
+		goto fail;
 
 	/* Clone the original skb, sharing the data.  */
 	skb = skb_clone(chunk->skb, gfp);
@@ -681,8 +679,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
 	 * The sender should never pad with more than 3 bytes.  The receiver
 	 * MUST ignore the padding bytes.
 	 */
-	len = ntohs(chunk->chunk_hdr->length);
-	padding = SCTP_PAD4(len) - len;
+	padding = SCTP_PAD4(datalen) - datalen;
 
 	/* Fixup cloned skb with just this chunks data.  */
 	skb_trim(skb, chunk->chunk_end - padding - skb->data);
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 5dde921..770ff1f 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -1106,7 +1106,8 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 			freed += sctp_ulpq_renege_frags(ulpq, needed - freed);
 	}
 	/* If able to free enough room, accept this chunk. */
-	if (freed >= needed) {
+	if (sk_rmem_schedule(asoc->base.sk, chunk->skb, needed) &&
+	    freed >= needed) {
 		int retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
 		/*
 		 * Enter partial delivery if chunk has not been
-- 
2.1.0

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Xin Long]

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

Simple tests are as attached.

On Sun, Mar 31, 2019 at 4:53 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> sctp memory accounting is added in this patchset by using
> these kernel APIs on send side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_wmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> and these on receive side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_rmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> With sctp memory accounting, we can limit the memory allocation by
> either sysctl:
>
>   # sysctl -w net.sctp.sctp_mem="10 20 50"
>
> or cgroup:
>
>   # echo $((8<<14)) > \
>     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
>
> When the socket is under memory pressure, the send side will block
> and wait, while the receive side will renege or drop.
>
> Xin Long (2):
>   sctp: implement memory accounting on tx path
>   sctp: implement memory accounting on rx path
>
>  include/net/sctp/sctp.h |  2 +-
>  net/sctp/sm_statefuns.c |  6 ++++--
>  net/sctp/socket.c       | 10 ++++++++--
>  net/sctp/ulpevent.c     | 19 ++++++++-----------
>  net/sctp/ulpqueue.c     |  3 ++-
>  5 files changed, 23 insertions(+), 17 deletions(-)
>
> --
> 2.1.0
>

[-- Attachment #2: sysctl_sockets_memctl.sh --]
[-- Type: text/x-sh, Size: 1473 bytes --]

#!/bin/sh

# TOPO: local (10.73.131.158) <--> peer (10.73.131.202) [hosts not netns]

peer_run()
{
	ssh root@10.73.131.202 "$@"
	# echo " - peer run: $@"
	# echo " - print any key to continue after it's done"
	# read
}

modprobe sctp

echo "sctp sysctl memory control rmem:"
echo "--------------------------------"

echo "10 20 50 rmem limit ->"
pkill sctp_test 2> /dev/null
sysctl -w  net.sctp.sctp_mem="10 20 50"
sctp_test -H 10.73.131.158 -P 1234 -l > /dev/null 2>&1 &
sleep 3
peer_run "time sctp_test -H 10.73.131.202 -P 8000 -h 10.73.131.158 -p 1234 -s -c 5  > /dev/null 2>&1"
echo ""

echo "20 50 100 rmem limit ->"
pkill sctp_test 2> /dev/null
sysctl -w  net.sctp.sctp_mem="20 50 100"
sctp_test -H 10.73.131.158 -P 1234 -l > /dev/null 2>&1 &
sleep 3
peer_run "time sctp_test -H 10.73.131.202 -P 8000 -h 10.73.131.158 -p 1234 -s -c 5  > /dev/null 2>&1"
echo ""

echo "sctp sysctl memory control wmem:"
echo "--------------------------------"

peer_run "sctp_test -H 10.73.131.202 -P 8000 -l > /dev/null 2>&1 &"
tc qdisc add dev eth0 root netem delay 50ms
echo "10 20 50 wmem limit ->"
sysctl -w  net.sctp.sctp_mem="5 10 20"
time sctp_test -h 10.73.131.202 -p 8000 -H 10.73.131.158 -P 1234 -s -c 5  > /dev/null 2>&1
echo ""

echo "10 50 100 wmem limit ->"
sysctl -w  net.sctp.sctp_mem="10 50 100 ->"
time sctp_test -h 10.73.131.202 -p 8000 -H 10.73.131.158 -P 1234 -s -c 5  > /dev/null 2>&1
echo ""
tc qdisc delete dev eth0 root
peer_run "pkill sctp_test 2>/dev/null"

[-- Attachment #3: cgroup_sockets_memctl.sh --]
[-- Type: text/x-sh, Size: 2076 bytes --]

#!/bin/sh

# TOPO: 127.0.0.1 lo

modprobe sctp
pkill sctp_test 2> /dev/null
sctp_test -H 127.0.0.1 -P 1234 -l > /dev/null 2>&1 &
sleep 3
mkdir /sys/fs/cgroup/memory/sctp_mem > /dev/null 2>&1;

echo "sctp_wmem testing:"
echo "------------------"

echo "$((8<<14)) bytes for wmem limit ->"
echo $((8<<14)) >/sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
{
	echo $BASHPID > /sys/fs/cgroup/memory/sctp_mem/cgroup.procs
	time sctp_test -H 127.0.0.1 -P 8000 -h 127.0.0.1 -p 1234 -s -c 5  > /dev/null 2>&1
	echo ""
} &
wait $!

echo "$((8<<16)) bytes for wmem limit ->"
echo $((8<<16)) >/sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
{
	echo $BASHPID > /sys/fs/cgroup/memory/sctp_mem/cgroup.procs
	time sctp_test -H 127.0.0.1 -P 8000 -h 127.0.0.1 -p 1234 -s -c 5  > /dev/null 2>&1
	echo ""
} &
wait $!


echo "sctp_rmem testing:"
echo "------------------"

echo "$((8<<13)) bytes for rmem limit ->"
pkill sctp_test 2> /dev/null
echo $((8<<13)) >/sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
{
	echo $BASHPID > /sys/fs/cgroup/memory/sctp_mem/cgroup.procs
	sctp_test -H 127.0.0.1 -P 1234 -l > /dev/null 2>&1
} &
sleep 3
time sctp_test -H 127.0.0.1 -P 8000 -h 127.0.0.1 -p 1234 -s -c 5  > /dev/null 2>&1
echo ""

echo "$((8<<14)) bytes for rmem limit ->"
pkill sctp_test 2> /dev/null
echo $((8<<14)) >/sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
{
	echo $BASHPID > /sys/fs/cgroup/memory/sctp_mem/cgroup.procs
	sctp_test -H 127.0.0.1 -P 1234 -l > /dev/null 2>&1
} &
sleep 3
time sctp_test -H 127.0.0.1 -P 8000 -h 127.0.0.1 -p 1234 -s -c 5  > /dev/null 2>&1
echo ""

echo "sctp_wmem_and_rmem testing:"
echo "---------------------------"

echo "$((8<<14)) bytes for wmem AND rmem limit ->"
pkill sctp_test 2> /dev/null
echo $((8<<14)) >/sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
{
	echo $BASHPID > /sys/fs/cgroup/memory/sctp_mem/cgroup.procs
	sctp_test -H 127.0.0.1 -P 1234 -l > /dev/null 2>&1 &
	sleep 3
	time sctp_test -H 127.0.0.1 -P 8000 -h 127.0.0.1 -p 1234 -s -c 5  > /dev/null 2>&1
	echo ""
} &
wait

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Sun, 31 Mar 2019 16:56:40 +0800

> Simple tests are as attached.

Are these integratable into selftests somehow?

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Matteo Croce]

On Sun, Mar 31, 2019 at 10:53 AM Xin Long <lucien.xin@gmail.com> wrote:
>
> sctp memory accounting is added in this patchset by using
> these kernel APIs on send side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_wmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> and these on receive side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_rmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> With sctp memory accounting, we can limit the memory allocation by
> either sysctl:
>
>   # sysctl -w net.sctp.sctp_mem="10 20 50"
>
> or cgroup:
>
>   # echo $((8<<14)) > \
>     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
>
> When the socket is under memory pressure, the send side will block
> and wait, while the receive side will renege or drop.
>

I have tested this series with a tool which creates lot of SCTP
sockets and writes into them, without never reading.
Before the patch I was able to escape the cgroup limit and fill the
system memory, and the OOM killer killed random processes:

[  317.348911] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),global_oom,task_memcg=/system.slice/ifup@eth0.service,task=dhclient,pid=188,uid=0
[  317.349084] Out of memory: Killed process 188 (dhclient)
total-vm:9484kB, anon-rss:1280kB, file-rss:1424kB, shmem-rss:0kB
[  317.743943] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),global_oom,task_memcg=/system.slice/systemd-journald.service,task=systemd-journal,pid=85,uid=0
[  317.744093] Out of memory: Killed process 85 (systemd-journal)
total-vm:24592kB, anon-rss:1024kB, file-rss:1112kB, shmem-rss:652kB
[  317.921049] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),global_oom,task_memcg=/system.slice/cron.service,task=cron,pid=222,uid=0
[  317.921209] Out of memory: Killed process 222 (cron)
total-vm:8692kB, anon-rss:276kB, file-rss:1540kB, shmem-rss:0kB

Now the OOM killer behaves correctly and always kills the processes in
the right cgroup:

[  512.100054] Tasks state (memory values in pages):
[  512.100122] [  pid  ]   uid  tgid total_vm      rss pgtables_bytes
swapents oom_score_adj name
[  512.100256] [    838]     0   838      550      184    36864
0             0 sctprank
[  512.100452] [    841]     0   841      550       18    36864
0             0 sctprank
[  512.100573] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),oom_memcg=/sctp,task_memcg=/sctp,task=sctprank,pid=838,uid=0
[  512.100700] Memory cgroup out of memory: Killed process 838
(sctprank) total-vm:2200kB, anon-rss:64kB, file-rss:672kB,
shmem-rss:0kB
[  512.100899] oom_reaper: reaped process 838 (sctprank), now
anon-rss:0kB, file-rss:0kB, shmem-rss:0kB

Series ACK.

Tested-by: Matteo Croce <mcroce@redhat.com>


--
Matteo Croce
per aspera ad upstream

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Marcelo Ricardo Leitner]

On Sun, Mar 31, 2019 at 04:53:45PM +0800, Xin Long wrote:
> sctp memory accounting is added in this patchset by using
> these kernel APIs on send side:

Please give me till tomorrow to review this one.

Thanks,
Marcelo

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Sun, 31 Mar 2019 17:10:48 -0300

> On Sun, Mar 31, 2019 at 04:53:45PM +0800, Xin Long wrote:
>> sctp memory accounting is added in this patchset by using
>> these kernel APIs on send side:
> 
> Please give me till tomorrow to review this one.

No problem.

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Xin Long]

On Sun, Mar 31, 2019 at 4:53 PM Xin Long <lucien.xin@gmail.com> wrote:
>
> sctp memory accounting is added in this patchset by using
> these kernel APIs on send side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_wmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> and these on receive side:
>
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_rmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
>
> With sctp memory accounting, we can limit the memory allocation by
> either sysctl:
>
>   # sysctl -w net.sctp.sctp_mem="10 20 50"
>
> or cgroup:
>
>   # echo $((8<<14)) > \
>     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
>
> When the socket is under memory pressure, the send side will block
> and wait, while the receive side will renege or drop.
>
> Xin Long (2):
>   sctp: implement memory accounting on tx path
>   sctp: implement memory accounting on rx path
>
>  include/net/sctp/sctp.h |  2 +-
>  net/sctp/sm_statefuns.c |  6 ++++--
>  net/sctp/socket.c       | 10 ++++++++--
>  net/sctp/ulpevent.c     | 19 ++++++++-----------
>  net/sctp/ulpqueue.c     |  3 ++-
>  5 files changed, 23 insertions(+), 17 deletions(-)
>
> --
> 2.1.0
>
Reported-by: Matteo Croce <mcroce@redhat.com>

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Neil Horman]

On Sun, Mar 31, 2019 at 04:53:45PM +0800, Xin Long wrote:
> sctp memory accounting is added in this patchset by using
> these kernel APIs on send side:
> 
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_wmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
> 
> and these on receive side:
> 
>   - sk_mem_charge()
>   - sk_mem_uncharge()
>   - sk_rmem_schedule()
>   - sk_under_memory_pressure()
>   - sk_mem_reclaim()
> 
> With sctp memory accounting, we can limit the memory allocation by
> either sysctl:
> 
>   # sysctl -w net.sctp.sctp_mem="10 20 50"
> 
> or cgroup:
> 
>   # echo $((8<<14)) > \
>     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
> 
> When the socket is under memory pressure, the send side will block
> and wait, while the receive side will renege or drop.
> 
> Xin Long (2):
>   sctp: implement memory accounting on tx path
>   sctp: implement memory accounting on rx path
> 
>  include/net/sctp/sctp.h |  2 +-
>  net/sctp/sm_statefuns.c |  6 ++++--
>  net/sctp/socket.c       | 10 ++++++++--
>  net/sctp/ulpevent.c     | 19 ++++++++-----------
>  net/sctp/ulpqueue.c     |  3 ++-
>  5 files changed, 23 insertions(+), 17 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
I don't have a problem with either of these patches in terms of altering memory
accounting, but SCTP has the notion of accounting buffers based on either
sockets space or association space (based on the sndbuf_policy and rcvbuf_policy
sysctls).  This patch eliminates them.  I don't see this patch addressing either
the removal of that functionality (as the proposed accounting scheme renders
those sysctls useless and ignored, which may cause regressions in some
environments), nor does it address the possibiliy of one association starving
others on the same socket when they share the same socket level accounting.  I
think you need to look how to address that (either by re-adding the ability to
account in either case based on the sysctls, or deprecating eliminating the
sysctls and addressing the starvation issue.

Best
Neil

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Marcelo Ricardo Leitner]

On Mon, Apr 01, 2019 at 07:31:10AM -0400, Neil Horman wrote:
> On Sun, Mar 31, 2019 at 04:53:45PM +0800, Xin Long wrote:
> > sctp memory accounting is added in this patchset by using
> > these kernel APIs on send side:
> > 
> >   - sk_mem_charge()
> >   - sk_mem_uncharge()
> >   - sk_wmem_schedule()
> >   - sk_under_memory_pressure()
> >   - sk_mem_reclaim()
> > 
> > and these on receive side:
> > 
> >   - sk_mem_charge()
> >   - sk_mem_uncharge()
> >   - sk_rmem_schedule()
> >   - sk_under_memory_pressure()
> >   - sk_mem_reclaim()
> > 
> > With sctp memory accounting, we can limit the memory allocation by
> > either sysctl:
> > 
> >   # sysctl -w net.sctp.sctp_mem="10 20 50"
> > 
> > or cgroup:
> > 
> >   # echo $((8<<14)) > \
> >     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
> > 
> > When the socket is under memory pressure, the send side will block
> > and wait, while the receive side will renege or drop.
> > 
> > Xin Long (2):
> >   sctp: implement memory accounting on tx path
> >   sctp: implement memory accounting on rx path
> > 
> >  include/net/sctp/sctp.h |  2 +-
> >  net/sctp/sm_statefuns.c |  6 ++++--
> >  net/sctp/socket.c       | 10 ++++++++--
> >  net/sctp/ulpevent.c     | 19 ++++++++-----------
> >  net/sctp/ulpqueue.c     |  3 ++-
> >  5 files changed, 23 insertions(+), 17 deletions(-)
> > 
> > -- 
> > 2.1.0
> > 
> > 
> I don't have a problem with either of these patches in terms of altering memory
> accounting, but SCTP has the notion of accounting buffers based on either
> sockets space or association space (based on the sndbuf_policy and rcvbuf_policy
> sysctls).  This patch eliminates them.  I don't see this patch addressing either
> the removal of that functionality (as the proposed accounting scheme renders
> those sysctls useless and ignored, which may cause regressions in some
> environments), nor does it address the possibiliy of one association starving
> others on the same socket when they share the same socket level accounting.  I
> think you need to look how to address that (either by re-adding the ability to
> account in either case based on the sysctls, or deprecating eliminating the
> sysctls and addressing the starvation issue.

That's not how I'm reading these. All original conditions are still
there while these patches are adding a couple of restrictions more.
What that means is that they are adding a ceiling to it, even if the
limits are per socket or per assoc. Considering the idea of the cgroup
limit being added here, it makes sense to me. If the cgroup is
configured to allow at most X MB, it doesn't matter how that is
allocated. That's a sysadmin task then, to adjust the other sysctls
(net.sctp.sctp_mem & cia) and balance the usage, be it per socket or
per asoc.

Cheers,
  Marcelo

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Neil Horman]

On Tue, Apr 02, 2019 at 12:36:10AM -0300, Marcelo Ricardo Leitner wrote:
> On Mon, Apr 01, 2019 at 07:31:10AM -0400, Neil Horman wrote:
> > On Sun, Mar 31, 2019 at 04:53:45PM +0800, Xin Long wrote:
> > > sctp memory accounting is added in this patchset by using
> > > these kernel APIs on send side:
> > > 
> > >   - sk_mem_charge()
> > >   - sk_mem_uncharge()
> > >   - sk_wmem_schedule()
> > >   - sk_under_memory_pressure()
> > >   - sk_mem_reclaim()
> > > 
> > > and these on receive side:
> > > 
> > >   - sk_mem_charge()
> > >   - sk_mem_uncharge()
> > >   - sk_rmem_schedule()
> > >   - sk_under_memory_pressure()
> > >   - sk_mem_reclaim()
> > > 
> > > With sctp memory accounting, we can limit the memory allocation by
> > > either sysctl:
> > > 
> > >   # sysctl -w net.sctp.sctp_mem="10 20 50"
> > > 
> > > or cgroup:
> > > 
> > >   # echo $((8<<14)) > \
> > >     /sys/fs/cgroup/memory/sctp_mem/memory.kmem.tcp.limit_in_bytes
> > > 
> > > When the socket is under memory pressure, the send side will block
> > > and wait, while the receive side will renege or drop.
> > > 
> > > Xin Long (2):
> > >   sctp: implement memory accounting on tx path
> > >   sctp: implement memory accounting on rx path
> > > 
> > >  include/net/sctp/sctp.h |  2 +-
> > >  net/sctp/sm_statefuns.c |  6 ++++--
> > >  net/sctp/socket.c       | 10 ++++++++--
> > >  net/sctp/ulpevent.c     | 19 ++++++++-----------
> > >  net/sctp/ulpqueue.c     |  3 ++-
> > >  5 files changed, 23 insertions(+), 17 deletions(-)
> > > 
> > > -- 
> > > 2.1.0
> > > 
> > > 
> > I don't have a problem with either of these patches in terms of altering memory
> > accounting, but SCTP has the notion of accounting buffers based on either
> > sockets space or association space (based on the sndbuf_policy and rcvbuf_policy
> > sysctls).  This patch eliminates them.  I don't see this patch addressing either
> > the removal of that functionality (as the proposed accounting scheme renders
> > those sysctls useless and ignored, which may cause regressions in some
> > environments), nor does it address the possibiliy of one association starving
> > others on the same socket when they share the same socket level accounting.  I
> > think you need to look how to address that (either by re-adding the ability to
> > account in either case based on the sysctls, or deprecating eliminating the
> > sysctls and addressing the starvation issue.
> 
> That's not how I'm reading these. All original conditions are still
> there while these patches are adding a couple of restrictions more.
> What that means is that they are adding a ceiling to it, even if the
> limits are per socket or per assoc. Considering the idea of the cgroup
> limit being added here, it makes sense to me. If the cgroup is
> configured to allow at most X MB, it doesn't matter how that is
> allocated. That's a sysadmin task then, to adjust the other sysctls
> (net.sctp.sctp_mem & cia) and balance the usage, be it per socket or
> per asoc.
> 

You're right, I had the sense on the conditional backwards in my head. My bad

Series

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 1/2] sctp: implement memory accounting on tx path

[Marcelo Ricardo Leitner]

On Sun, Mar 31, 2019 at 04:53:46PM +0800, Xin Long wrote:
> Now when sending packets, sk_mem_charge() and sk_mem_uncharge() have been
> used to set sk_forward_alloc. We just need to call sk_wmem_schedule() to
> check if the allocated should be raised, and call sk_mem_reclaim() to
> check if the allocated should be reduced when it's under memory pressure.
> 
> If sk_wmem_schedule() returns false, which means no memory is allowed to
> allocate, it will block and wait for memory to become available.
> 
> Note different from tcp, sctp wait_for_buf happens before allocating any
> skb, so memory accounting check is done with the whole msg_len before it
> too.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  net/sctp/socket.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 6140471..06c6f4a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1913,7 +1913,10 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc,
>  	if (sctp_wspace(asoc) < (int)msg_len)
>  		sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc));
>  
> -	if (sctp_wspace(asoc) <= 0) {
> +	if (sk_under_memory_pressure(sk))
> +		sk_mem_reclaim(sk);
> +
> +	if (sctp_wspace(asoc) <= 0 || !sk_wmem_schedule(sk, msg_len)) {
>  		timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
>  		err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len);
>  		if (err)
> @@ -8891,7 +8894,10 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p,
>  			goto do_error;
>  		if (signal_pending(current))
>  			goto do_interrupted;
> -		if ((int)msg_len <= sctp_wspace(asoc))
> +		if (sk_under_memory_pressure(sk))
> +			sk_mem_reclaim(sk);
> +		if ((int)msg_len <= sctp_wspace(asoc) &&
> +		    sk_wmem_schedule(sk, msg_len))
>  			break;
>  
>  		/* Let another process have a go.  Since we are going
> -- 
> 2.1.0
> 

Re: [PATCH net-next 2/2] sctp: implement memory accounting on rx path

[Marcelo Ricardo Leitner]

On Sun, Mar 31, 2019 at 04:53:47PM +0800, Xin Long wrote:
> sk_forward_alloc's updating is also done on rx path, but to be consistent
> we change to use sk_mem_charge() in sctp_skb_set_owner_r().
> 
> In sctp_eat_data(), it's not enough to check sctp_memory_pressure only,
> which doesn't work for mem_cgroup_sockets_enabled, so we change to use
> sk_under_memory_pressure().
> 
> When it's under memory pressure, sk_mem_reclaim() and sk_rmem_schedule()
> should be called on both RENEGE or CHUNK DELIVERY path exit the memory
> pressure status as soon as possible.
> 
> Note that sk_rmem_schedule() is using datalen to make things easy there.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/net/sctp/sctp.h |  2 +-
>  net/sctp/sm_statefuns.c |  6 ++++--
>  net/sctp/ulpevent.c     | 19 ++++++++-----------
>  net/sctp/ulpqueue.c     |  3 ++-
>  4 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 1d13ec3..eefdfa5 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -421,7 +421,7 @@ static inline void sctp_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
>  	/*
>  	 * This mimics the behavior of skb_set_owner_r
>  	 */
> -	sk->sk_forward_alloc -= event->rmem_len;
> +	sk_mem_charge(sk, event->rmem_len);
>  }
>  
>  /* Tests if the list has one and only one entry. */
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index c9ae340..7dfc34b 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -6412,13 +6412,15 @@ static int sctp_eat_data(const struct sctp_association *asoc,
>  	 * in sctp_ulpevent_make_rcvmsg will drop the frame if we grow our
>  	 * memory usage too much
>  	 */
> -	if (*sk->sk_prot_creator->memory_pressure) {
> +	if (sk_under_memory_pressure(sk)) {
>  		if (sctp_tsnmap_has_gap(map) &&
>  		    (sctp_tsnmap_get_ctsn(map) + 1) == tsn) {
>  			pr_debug("%s: under pressure, reneging for tsn:%u\n",
>  				 __func__, tsn);
>  			deliver = SCTP_CMD_RENEGE;
> -		 }
> +		} else {
> +			sk_mem_reclaim(sk);
> +		}
>  	}
>  
>  	/*
> diff --git a/net/sctp/ulpevent.c b/net/sctp/ulpevent.c
> index 8cb7d98..c2a7478 100644
> --- a/net/sctp/ulpevent.c
> +++ b/net/sctp/ulpevent.c
> @@ -634,8 +634,9 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
>  						gfp_t gfp)
>  {
>  	struct sctp_ulpevent *event = NULL;
> -	struct sk_buff *skb;
> -	size_t padding, len;
> +	struct sk_buff *skb = chunk->skb;
> +	struct sock *sk = asoc->base.sk;
> +	size_t padding, datalen;
>  	int rx_count;
>  
>  	/*
> @@ -646,15 +647,12 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
>  	if (asoc->ep->rcvbuf_policy)
>  		rx_count = atomic_read(&asoc->rmem_alloc);
>  	else
> -		rx_count = atomic_read(&asoc->base.sk->sk_rmem_alloc);
> +		rx_count = atomic_read(&sk->sk_rmem_alloc);
>  
> -	if (rx_count >= asoc->base.sk->sk_rcvbuf) {
> +	datalen = ntohs(chunk->chunk_hdr->length);
>  
> -		if ((asoc->base.sk->sk_userlocks & SOCK_RCVBUF_LOCK) ||
> -		    (!sk_rmem_schedule(asoc->base.sk, chunk->skb,
> -				       chunk->skb->truesize)))
> -			goto fail;
> -	}
> +	if (rx_count >= sk->sk_rcvbuf || !sk_rmem_schedule(sk, skb, datalen))
> +		goto fail;
>  
>  	/* Clone the original skb, sharing the data.  */
>  	skb = skb_clone(chunk->skb, gfp);
> @@ -681,8 +679,7 @@ struct sctp_ulpevent *sctp_ulpevent_make_rcvmsg(struct sctp_association *asoc,
>  	 * The sender should never pad with more than 3 bytes.  The receiver
>  	 * MUST ignore the padding bytes.
>  	 */
> -	len = ntohs(chunk->chunk_hdr->length);
> -	padding = SCTP_PAD4(len) - len;
> +	padding = SCTP_PAD4(datalen) - datalen;
>  
>  	/* Fixup cloned skb with just this chunks data.  */
>  	skb_trim(skb, chunk->chunk_end - padding - skb->data);
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index 5dde921..770ff1f 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -1106,7 +1106,8 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
>  			freed += sctp_ulpq_renege_frags(ulpq, needed - freed);
>  	}
>  	/* If able to free enough room, accept this chunk. */
> -	if (freed >= needed) {
> +	if (sk_rmem_schedule(asoc->base.sk, chunk->skb, needed) &&
> +	    freed >= needed) {
>  		int retval = sctp_ulpq_tail_data(ulpq, chunk, gfp);
>  		/*
>  		 * Enter partial delivery if chunk has not been
> -- 
> 2.1.0
> 

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Xin Long]

On Mon, Apr 1, 2019 at 2:02 AM David Miller <davem@davemloft.net> wrote:
>
> From: Xin Long <lucien.xin@gmail.com>
> Date: Sun, 31 Mar 2019 16:56:40 +0800
>
> > Simple tests are as attached.
>
> Are these integratable into selftests somehow?
We're thinking about adding some sctp tests into selftests,
but these ones are not good to be the first one, I think.
sysctl_sockets_memctl.sh is a multi-host test, even netns can't work for it.

Each subcomponent seems to have its own test case in other git repo,
can I ask what kind of tests should I put into kernel selftests in the future?

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Thu, 4 Apr 2019 17:45:07 +0800

> We're thinking about adding some sctp tests into selftests,
> but these ones are not good to be the first one, I think.
> sysctl_sockets_memctl.sh is a multi-host test, even netns can't work for it.

You can't set per-netns memory controls appropriately to make the
tests work?

> Each subcomponent seems to have its own test case in other git repo,
> can I ask what kind of tests should I put into kernel selftests in
> the future?

It would be ideal that we have a decent base set of SCTP tests and
then when bug fixes are added, we get a unit test with the fix.

I guess a good baseline would be testing basic comminucation between
SCTP sockets in different netns, and then adding tests for setting
the various socket options and making sure the socket option had the
desired effect.

Re: [PATCH net-next 0/2] sctp: fully support memory accounting

[Xin Long]

On Fri, Apr 5, 2019 at 1:38 AM David Miller <davem@davemloft.net> wrote:
>
> From: Xin Long <lucien.xin@gmail.com>
> Date: Thu, 4 Apr 2019 17:45:07 +0800
>
> > We're thinking about adding some sctp tests into selftests,
> > but these ones are not good to be the first one, I think.
> > sysctl_sockets_memctl.sh is a multi-host test, even netns can't work for it.
>
> You can't set per-netns memory controls appropriately to make the
> tests work?
static atomic_long_t sctp_memory_allocated;

struct proto sctp_prot = {
.memory_allocated = &sctp_memory_allocated,

sysctl limitation is per-netns, but the counter 'sctp_memory_allocated' is not.
It means other netns buffer allocation will affect the current netns.

>
> > Each subcomponent seems to have its own test case in other git repo,
> > can I ask what kind of tests should I put into kernel selftests in
> > the future?
>
> It would be ideal that we have a decent base set of SCTP tests and
> then when bug fixes are added, we get a unit test with the fix.
>
> I guess a good baseline would be testing basic comminucation between
> SCTP sockets in different netns, and then adding tests for setting
> the various socket options and making sure the socket option had the
> desired effect.
got your point. thanks