* [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it to check for writable space @ 2018-10-16 19:07 ` Xin Long 0 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem sctp doesn't count and use asoc sndbuf_used, sk sk_wmem_alloc and sk_wmem_queued properly, which also causes some problem. This patchset is to improve it. Xin Long (2): sctp: count both sk and asoc sndbuf with skb truesize and sctp_chunk size sctp: use sk_wmem_queued to check for writable space include/net/sctp/constants.h | 5 ---- net/sctp/outqueue.c | 8 ++---- net/sctp/socket.c | 59 +++++++++++--------------------------------- 3 files changed, 17 insertions(+), 55 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it to check for writable space @ 2018-10-16 19:07 ` Xin Long 0 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem sctp doesn't count and use asoc sndbuf_used, sk sk_wmem_alloc and sk_wmem_queued properly, which also causes some problem. This patchset is to improve it. Xin Long (2): sctp: count both sk and asoc sndbuf with skb truesize and sctp_chunk size sctp: use sk_wmem_queued to check for writable space include/net/sctp/constants.h | 5 ---- net/sctp/outqueue.c | 8 ++---- net/sctp/socket.c | 59 +++++++++++--------------------------------- 3 files changed, 17 insertions(+), 55 deletions(-) -- 2.1.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH net-next 1/2] sctp: count both sk and asoc sndbuf with skb truesize and sctp_chunk size 2018-10-16 19:07 ` Xin Long @ 2018-10-16 19:07 ` Xin Long -1 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem Now it's confusing that asoc sndbuf_used is doing memory accounting with SCTP_DATA_SNDSIZE(chunk) + sizeof(sk_buff) + sizeof(sctp_chunk) while sk sk_wmem_alloc is doing that with skb->truesize + sizeof(sctp_chunk). It also causes sctp_prsctp_prune to count with a wrong freed memory when sndbuf_policy is not set. To make this right and also keep consistent between asoc sndbuf_used, sk sk_wmem_alloc and sk_wmem_queued, use skb->truesize + sizeof(sctp_chunk) for them. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/sctp/constants.h | 5 ----- net/sctp/outqueue.c | 8 ++------ net/sctp/socket.c | 21 ++++++--------------- 3 files changed, 8 insertions(+), 26 deletions(-) diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h index 86f034b..8dadc74 100644 --- a/include/net/sctp/constants.h +++ b/include/net/sctp/constants.h @@ -148,11 +148,6 @@ SCTP_SUBTYPE_CONSTRUCTOR(PRIMITIVE, enum sctp_event_primitive, primitive) #define sctp_chunk_is_data(a) (a->chunk_hdr->type == SCTP_CID_DATA || \ a->chunk_hdr->type == SCTP_CID_I_DATA) -/* Calculate the actual data size in a data chunk */ -#define SCTP_DATA_SNDSIZE(c) ((int)((unsigned long)(c->chunk_end) - \ - (unsigned long)(c->chunk_hdr) - \ - sctp_datachk_len(&c->asoc->stream))) - /* Internal error codes */ enum sctp_ierror { SCTP_IERROR_NO_ERROR = 0, diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index 42191ed..9cb854b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -385,9 +385,7 @@ static int sctp_prsctp_prune_sent(struct sctp_association *asoc, asoc->outqueue.outstanding_bytes -= sctp_data_size(chk); } - msg_len -= SCTP_DATA_SNDSIZE(chk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); + msg_len -= chk->skb->truesize + sizeof(struct sctp_chunk); if (msg_len <= 0) break; } @@ -421,9 +419,7 @@ static int sctp_prsctp_prune_unsent(struct sctp_association *asoc, streamout->ext->abandoned_unsent[SCTP_PR_INDEX(PRIO)]++; } - msg_len -= SCTP_DATA_SNDSIZE(chk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); + msg_len -= chk->skb->truesize + sizeof(struct sctp_chunk); sctp_chunk_free(chk); if (msg_len <= 0) break; diff --git a/net/sctp/socket.c b/net/sctp/socket.c index f73e9d3..c6f2950 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -166,12 +166,9 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk) /* Save the chunk pointer in skb for sctp_wfree to use later. */ skb_shinfo(chunk->skb)->destructor_arg = chunk; - asoc->sndbuf_used += SCTP_DATA_SNDSIZE(chunk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); - refcount_add(sizeof(struct sctp_chunk), &sk->sk_wmem_alloc); - sk->sk_wmem_queued += chunk->skb->truesize; + asoc->sndbuf_used += chunk->skb->truesize + sizeof(struct sctp_chunk); + sk->sk_wmem_queued += chunk->skb->truesize + sizeof(struct sctp_chunk); sk_mem_charge(sk, chunk->skb->truesize); } @@ -8460,17 +8457,11 @@ static void sctp_wfree(struct sk_buff *skb) struct sctp_association *asoc = chunk->asoc; struct sock *sk = asoc->base.sk; - asoc->sndbuf_used -= SCTP_DATA_SNDSIZE(chunk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); - - WARN_ON(refcount_sub_and_test(sizeof(struct sctp_chunk), &sk->sk_wmem_alloc)); - - /* - * This undoes what is done via sctp_set_owner_w and sk_mem_charge - */ - sk->sk_wmem_queued -= skb->truesize; sk_mem_uncharge(sk, skb->truesize); + sk->sk_wmem_queued -= skb->truesize + sizeof(struct sctp_chunk); + asoc->sndbuf_used -= skb->truesize + sizeof(struct sctp_chunk); + WARN_ON(refcount_sub_and_test(sizeof(struct sctp_chunk), + &sk->sk_wmem_alloc)); if (chunk->shkey) { struct sctp_shared_key *shkey = chunk->shkey; -- 2.1.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next 1/2] sctp: count both sk and asoc sndbuf with skb truesize and sctp_chunk size @ 2018-10-16 19:07 ` Xin Long 0 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem Now it's confusing that asoc sndbuf_used is doing memory accounting with SCTP_DATA_SNDSIZE(chunk) + sizeof(sk_buff) + sizeof(sctp_chunk) while sk sk_wmem_alloc is doing that with skb->truesize + sizeof(sctp_chunk). It also causes sctp_prsctp_prune to count with a wrong freed memory when sndbuf_policy is not set. To make this right and also keep consistent between asoc sndbuf_used, sk sk_wmem_alloc and sk_wmem_queued, use skb->truesize + sizeof(sctp_chunk) for them. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- include/net/sctp/constants.h | 5 ----- net/sctp/outqueue.c | 8 ++------ net/sctp/socket.c | 21 ++++++--------------- 3 files changed, 8 insertions(+), 26 deletions(-) diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h index 86f034b..8dadc74 100644 --- a/include/net/sctp/constants.h +++ b/include/net/sctp/constants.h @@ -148,11 +148,6 @@ SCTP_SUBTYPE_CONSTRUCTOR(PRIMITIVE, enum sctp_event_primitive, primitive) #define sctp_chunk_is_data(a) (a->chunk_hdr->type = SCTP_CID_DATA || \ a->chunk_hdr->type = SCTP_CID_I_DATA) -/* Calculate the actual data size in a data chunk */ -#define SCTP_DATA_SNDSIZE(c) ((int)((unsigned long)(c->chunk_end) - \ - (unsigned long)(c->chunk_hdr) - \ - sctp_datachk_len(&c->asoc->stream))) - /* Internal error codes */ enum sctp_ierror { SCTP_IERROR_NO_ERROR = 0, diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c index 42191ed..9cb854b 100644 --- a/net/sctp/outqueue.c +++ b/net/sctp/outqueue.c @@ -385,9 +385,7 @@ static int sctp_prsctp_prune_sent(struct sctp_association *asoc, asoc->outqueue.outstanding_bytes -= sctp_data_size(chk); } - msg_len -= SCTP_DATA_SNDSIZE(chk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); + msg_len -= chk->skb->truesize + sizeof(struct sctp_chunk); if (msg_len <= 0) break; } @@ -421,9 +419,7 @@ static int sctp_prsctp_prune_unsent(struct sctp_association *asoc, streamout->ext->abandoned_unsent[SCTP_PR_INDEX(PRIO)]++; } - msg_len -= SCTP_DATA_SNDSIZE(chk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); + msg_len -= chk->skb->truesize + sizeof(struct sctp_chunk); sctp_chunk_free(chk); if (msg_len <= 0) break; diff --git a/net/sctp/socket.c b/net/sctp/socket.c index f73e9d3..c6f2950 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -166,12 +166,9 @@ static inline void sctp_set_owner_w(struct sctp_chunk *chunk) /* Save the chunk pointer in skb for sctp_wfree to use later. */ skb_shinfo(chunk->skb)->destructor_arg = chunk; - asoc->sndbuf_used += SCTP_DATA_SNDSIZE(chunk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); - refcount_add(sizeof(struct sctp_chunk), &sk->sk_wmem_alloc); - sk->sk_wmem_queued += chunk->skb->truesize; + asoc->sndbuf_used += chunk->skb->truesize + sizeof(struct sctp_chunk); + sk->sk_wmem_queued += chunk->skb->truesize + sizeof(struct sctp_chunk); sk_mem_charge(sk, chunk->skb->truesize); } @@ -8460,17 +8457,11 @@ static void sctp_wfree(struct sk_buff *skb) struct sctp_association *asoc = chunk->asoc; struct sock *sk = asoc->base.sk; - asoc->sndbuf_used -= SCTP_DATA_SNDSIZE(chunk) + - sizeof(struct sk_buff) + - sizeof(struct sctp_chunk); - - WARN_ON(refcount_sub_and_test(sizeof(struct sctp_chunk), &sk->sk_wmem_alloc)); - - /* - * This undoes what is done via sctp_set_owner_w and sk_mem_charge - */ - sk->sk_wmem_queued -= skb->truesize; sk_mem_uncharge(sk, skb->truesize); + sk->sk_wmem_queued -= skb->truesize + sizeof(struct sctp_chunk); + asoc->sndbuf_used -= skb->truesize + sizeof(struct sctp_chunk); + WARN_ON(refcount_sub_and_test(sizeof(struct sctp_chunk), + &sk->sk_wmem_alloc)); if (chunk->shkey) { struct sctp_shared_key *shkey = chunk->shkey; -- 2.1.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] sctp: use sk_wmem_queued to check for writable space 2018-10-16 19:07 ` Xin Long @ 2018-10-16 19:07 ` Xin Long -1 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem sk->sk_wmem_queued is used to count the size of chunks in out queue while sk->sk_wmem_alloc is for counting the size of chunks has been sent. sctp is increasing both of them before enqueuing the chunks, and using sk->sk_wmem_alloc to check for writable space. However, sk_wmem_alloc is also increased by 1 for the skb allocked for sending in sctp_packet_transmit() but it will not wake up the waiters when sk_wmem_alloc is decreased in this skb's destructor. If msg size is equal to sk_sndbuf and sendmsg is waiting for sndbuf, the check 'msg_len <= sctp_wspace(asoc)' in sctp_wait_for_sndbuf() will keep waiting if there's a skb allocked in sctp_packet_transmit, and later even if this skb got freed, the waiting thread will never get waked up. This issue has been there since very beginning, so we change to use sk->sk_wmem_queued to check for writable space as sk_wmem_queued is not increased for the skb allocked for sending, also as TCP does. SOCK_SNDBUF_LOCK check is also removed here as it's for tx buf auto tuning which I will add in another patch. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/socket.c | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index c6f2950..111ebd8 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -83,7 +83,7 @@ #include <net/sctp/stream_sched.h> /* Forward declarations for internal helper functions. */ -static int sctp_writeable(struct sock *sk); +static bool sctp_writeable(struct sock *sk); static void sctp_wfree(struct sk_buff *skb); static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, size_t msg_len); @@ -119,25 +119,10 @@ static void sctp_enter_memory_pressure(struct sock *sk) /* Get the sndbuf space available at the time on the association. */ static inline int sctp_wspace(struct sctp_association *asoc) { - int amt; + struct sock *sk = asoc->base.sk; - if (asoc->ep->sndbuf_policy) - amt = asoc->sndbuf_used; - else - amt = sk_wmem_alloc_get(asoc->base.sk); - - if (amt >= asoc->base.sk->sk_sndbuf) { - if (asoc->base.sk->sk_userlocks & SOCK_SNDBUF_LOCK) - amt = 0; - else { - amt = sk_stream_wspace(asoc->base.sk); - if (amt < 0) - amt = 0; - } - } else { - amt = asoc->base.sk->sk_sndbuf - amt; - } - return amt; + return asoc->ep->sndbuf_policy ? sk->sk_sndbuf - asoc->sndbuf_used + : sk_stream_wspace(sk); } /* Increment the used sndbuf space count of the corresponding association by @@ -1925,10 +1910,10 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc, asoc->pmtu_pending = 0; } - if (sctp_wspace(asoc) < msg_len) + if (sctp_wspace(asoc) < (int)msg_len) sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc)); - if (!sctp_wspace(asoc)) { + if (sctp_wspace(asoc) <= 0) { timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT); err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len); if (err) @@ -8535,7 +8520,7 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, goto do_error; if (signal_pending(current)) goto do_interrupted; - if (msg_len <= sctp_wspace(asoc)) + if ((int)msg_len <= sctp_wspace(asoc)) break; /* Let another process have a go. Since we are going @@ -8610,14 +8595,9 @@ void sctp_write_space(struct sock *sk) * UDP-style sockets or TCP-style sockets, this code should work. * - Daisy */ -static int sctp_writeable(struct sock *sk) +static bool sctp_writeable(struct sock *sk) { - int amt = 0; - - amt = sk->sk_sndbuf - sk_wmem_alloc_get(sk); - if (amt < 0) - amt = 0; - return amt; + return sk->sk_sndbuf > sk->sk_wmem_queued; } /* Wait for an association to go into ESTABLISHED state. If timeout is 0, -- 2.1.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH net-next 2/2] sctp: use sk_wmem_queued to check for writable space @ 2018-10-16 19:07 ` Xin Long 0 siblings, 0 replies; 8+ messages in thread From: Xin Long @ 2018-10-16 19:07 UTC (permalink / raw) To: network dev, linux-sctp; +Cc: Marcelo Ricardo Leitner, Neil Horman, davem sk->sk_wmem_queued is used to count the size of chunks in out queue while sk->sk_wmem_alloc is for counting the size of chunks has been sent. sctp is increasing both of them before enqueuing the chunks, and using sk->sk_wmem_alloc to check for writable space. However, sk_wmem_alloc is also increased by 1 for the skb allocked for sending in sctp_packet_transmit() but it will not wake up the waiters when sk_wmem_alloc is decreased in this skb's destructor. If msg size is equal to sk_sndbuf and sendmsg is waiting for sndbuf, the check 'msg_len <= sctp_wspace(asoc)' in sctp_wait_for_sndbuf() will keep waiting if there's a skb allocked in sctp_packet_transmit, and later even if this skb got freed, the waiting thread will never get waked up. This issue has been there since very beginning, so we change to use sk->sk_wmem_queued to check for writable space as sk_wmem_queued is not increased for the skb allocked for sending, also as TCP does. SOCK_SNDBUF_LOCK check is also removed here as it's for tx buf auto tuning which I will add in another patch. Signed-off-by: Xin Long <lucien.xin@gmail.com> --- net/sctp/socket.c | 38 +++++++++----------------------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/net/sctp/socket.c b/net/sctp/socket.c index c6f2950..111ebd8 100644 --- a/net/sctp/socket.c +++ b/net/sctp/socket.c @@ -83,7 +83,7 @@ #include <net/sctp/stream_sched.h> /* Forward declarations for internal helper functions. */ -static int sctp_writeable(struct sock *sk); +static bool sctp_writeable(struct sock *sk); static void sctp_wfree(struct sk_buff *skb); static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, size_t msg_len); @@ -119,25 +119,10 @@ static void sctp_enter_memory_pressure(struct sock *sk) /* Get the sndbuf space available at the time on the association. */ static inline int sctp_wspace(struct sctp_association *asoc) { - int amt; + struct sock *sk = asoc->base.sk; - if (asoc->ep->sndbuf_policy) - amt = asoc->sndbuf_used; - else - amt = sk_wmem_alloc_get(asoc->base.sk); - - if (amt >= asoc->base.sk->sk_sndbuf) { - if (asoc->base.sk->sk_userlocks & SOCK_SNDBUF_LOCK) - amt = 0; - else { - amt = sk_stream_wspace(asoc->base.sk); - if (amt < 0) - amt = 0; - } - } else { - amt = asoc->base.sk->sk_sndbuf - amt; - } - return amt; + return asoc->ep->sndbuf_policy ? sk->sk_sndbuf - asoc->sndbuf_used + : sk_stream_wspace(sk); } /* Increment the used sndbuf space count of the corresponding association by @@ -1925,10 +1910,10 @@ static int sctp_sendmsg_to_asoc(struct sctp_association *asoc, asoc->pmtu_pending = 0; } - if (sctp_wspace(asoc) < msg_len) + if (sctp_wspace(asoc) < (int)msg_len) sctp_prsctp_prune(asoc, sinfo, msg_len - sctp_wspace(asoc)); - if (!sctp_wspace(asoc)) { + if (sctp_wspace(asoc) <= 0) { timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT); err = sctp_wait_for_sndbuf(asoc, &timeo, msg_len); if (err) @@ -8535,7 +8520,7 @@ static int sctp_wait_for_sndbuf(struct sctp_association *asoc, long *timeo_p, goto do_error; if (signal_pending(current)) goto do_interrupted; - if (msg_len <= sctp_wspace(asoc)) + if ((int)msg_len <= sctp_wspace(asoc)) break; /* Let another process have a go. Since we are going @@ -8610,14 +8595,9 @@ void sctp_write_space(struct sock *sk) * UDP-style sockets or TCP-style sockets, this code should work. * - Daisy */ -static int sctp_writeable(struct sock *sk) +static bool sctp_writeable(struct sock *sk) { - int amt = 0; - - amt = sk->sk_sndbuf - sk_wmem_alloc_get(sk); - if (amt < 0) - amt = 0; - return amt; + return sk->sk_sndbuf > sk->sk_wmem_queued; } /* Wait for an association to go into ESTABLISHED state. If timeout is 0, -- 2.1.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it to check for writable space 2018-10-16 19:07 ` Xin Long @ 2018-10-18 18:24 ` David Miller -1 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2018-10-18 18:24 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman From: Xin Long <lucien.xin@gmail.com> Date: Wed, 17 Oct 2018 03:07:49 +0800 > sctp doesn't count and use asoc sndbuf_used, sk sk_wmem_alloc and > sk_wmem_queued properly, which also causes some problem. > > This patchset is to improve it. Series applied, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it to check for writable space @ 2018-10-18 18:24 ` David Miller 0 siblings, 0 replies; 8+ messages in thread From: David Miller @ 2018-10-18 18:24 UTC (permalink / raw) To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman From: Xin Long <lucien.xin@gmail.com> Date: Wed, 17 Oct 2018 03:07:49 +0800 > sctp doesn't count and use asoc sndbuf_used, sk sk_wmem_alloc and > sk_wmem_queued properly, which also causes some problem. > > This patchset is to improve it. Series applied, thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-19 2:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-16 19:07 [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it to check for writable space Xin Long 2018-10-16 19:07 ` Xin Long 2018-10-16 19:07 ` [PATCH net-next 1/2] sctp: count both sk and asoc sndbuf with skb truesize and sctp_chunk size Xin Long 2018-10-16 19:07 ` Xin Long 2018-10-16 19:07 ` [PATCH net-next 2/2] sctp: use sk_wmem_queued to check for writable space Xin Long 2018-10-16 19:07 ` Xin Long 2018-10-18 18:24 ` [PATCH net-next 0/2] sctp: fix sk_wmem_queued and use it " David Miller 2018-10-18 18:24 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.