[PATCH net 0/3] sctp: some cleanups for sctp

[PATCH net 0/3] sctp: some cleanups for sctp

[Xin Long]

There are some unused function and redundant rcu locks should be
removed.

Xin Long (3):
  sctp: move rcu_read_lock from __sctp_lookup_association to
    sctp_lookup_association
  sctp: remove rcu_read_lock in sctp_seq_dump_remote_addrs()
  sctp: remove the unused sctp_datamsg_free()

 include/net/sctp/structs.h |  1 -
 net/sctp/chunk.c           | 13 -------------
 net/sctp/input.c           |  4 ++--
 net/sctp/proc.c            |  2 --
 4 files changed, 2 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH net 0/3] sctp: some cleanups for sctp

[Xin Long]

There are some unused function and redundant rcu locks should be
removed.

Xin Long (3):
  sctp: move rcu_read_lock from __sctp_lookup_association to
    sctp_lookup_association
  sctp: remove rcu_read_lock in sctp_seq_dump_remote_addrs()
  sctp: remove the unused sctp_datamsg_free()

 include/net/sctp/structs.h |  1 -
 net/sctp/chunk.c           | 13 -------------
 net/sctp/input.c           |  4 ++--
 net/sctp/proc.c            |  2 --
 4 files changed, 2 insertions(+), 18 deletions(-)

-- 
2.1.0

[PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_association

[Xin Long]

__sctp_lookup_association() is only invoked by sctp_v4_err() and
sctp_rcv(), both which run on the rx BH, and it has been protected
by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].

So we can move it to sctp_lookup_association, only let
sctp_lookup_association use rcu_read_lock.

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

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 49d2cc7..21a2d6b 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -937,7 +937,6 @@ static struct sctp_association *__sctp_lookup_association(
 	struct sctp_transport *t;
 	struct sctp_association *asoc = NULL;
 
-	rcu_read_lock();
 	t = sctp_addrs_lookup_transport(net, local, peer);
 	if (!t || !sctp_transport_hold(t))
 		goto out;
@@ -949,7 +948,6 @@ static struct sctp_association *__sctp_lookup_association(
 	sctp_transport_put(t);
 
 out:
-	rcu_read_unlock();
 	return asoc;
 }
 
@@ -962,7 +960,9 @@ struct sctp_association *sctp_lookup_association(struct net *net,
 {
 	struct sctp_association *asoc;
 
+	rcu_read_lock();
 	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
+	rcu_read_unlock();
 
 	return asoc;
 }
-- 
2.1.0

[PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_association

[Xin Long]

__sctp_lookup_association() is only invoked by sctp_v4_err() and
sctp_rcv(), both which run on the rx BH, and it has been protected
by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].

So we can move it to sctp_lookup_association, only let
sctp_lookup_association use rcu_read_lock.

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

diff --git a/net/sctp/input.c b/net/sctp/input.c
index 49d2cc7..21a2d6b 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -937,7 +937,6 @@ static struct sctp_association *__sctp_lookup_association(
 	struct sctp_transport *t;
 	struct sctp_association *asoc = NULL;
 
-	rcu_read_lock();
 	t = sctp_addrs_lookup_transport(net, local, peer);
 	if (!t || !sctp_transport_hold(t))
 		goto out;
@@ -949,7 +948,6 @@ static struct sctp_association *__sctp_lookup_association(
 	sctp_transport_put(t);
 
 out:
-	rcu_read_unlock();
 	return asoc;
 }
 
@@ -962,7 +960,9 @@ struct sctp_association *sctp_lookup_association(struct net *net,
 {
 	struct sctp_association *asoc;
 
+	rcu_read_lock();
 	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
+	rcu_read_unlock();
 
 	return asoc;
 }
-- 
2.1.0

[PATCH net 2/3] sctp: remove rcu_read_lock in sctp_seq_dump_remote_addrs()

[Xin Long]

sctp_seq_dump_remote_addrs is only called by sctp_assocs_seq_show()
and it has been protected by rcu_read_lock that is from
rhashtable_walk_start().

So we will remove this one.

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

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index ded7d93..cfc3c71 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -161,7 +161,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 	struct sctp_af *af;
 
 	primary = &assoc->peer.primary_addr;
-	rcu_read_lock();
 	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
 			transports) {
 		addr = &transport->ipaddr;
@@ -172,7 +171,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 		}
 		af->seq_dump_addr(seq, addr);
 	}
-	rcu_read_unlock();
 }
 
 static void *sctp_eps_seq_start(struct seq_file *seq, loff_t *pos)
-- 
2.1.0

[PATCH net 2/3] sctp: remove rcu_read_lock in sctp_seq_dump_remote_addrs()

[Xin Long]

sctp_seq_dump_remote_addrs is only called by sctp_assocs_seq_show()
and it has been protected by rcu_read_lock that is from
rhashtable_walk_start().

So we will remove this one.

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

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index ded7d93..cfc3c71 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -161,7 +161,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 	struct sctp_af *af;
 
 	primary = &assoc->peer.primary_addr;
-	rcu_read_lock();
 	list_for_each_entry_rcu(transport, &assoc->peer.transport_addr_list,
 			transports) {
 		addr = &transport->ipaddr;
@@ -172,7 +171,6 @@ static void sctp_seq_dump_remote_addrs(struct seq_file *seq, struct sctp_associa
 		}
 		af->seq_dump_addr(seq, addr);
 	}
-	rcu_read_unlock();
 }
 
 static void *sctp_eps_seq_start(struct seq_file *seq, loff_t *pos)
-- 
2.1.0

[PATCH net 3/3] sctp: remove the unused sctp_datamsg_free()

[Xin Long]

Since commit 8b570dc9f7b6 ("sctp: only drop the reference on the datamsg
after sending a msg") used sctp_datamsg_put in sctp_sendmsg, instead of
sctp_datamsg_free, this function has no use in sctp.

So we will remove it.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  1 -
 net/sctp/chunk.c           | 13 -------------
 2 files changed, 14 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 205630b..d05b566 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -535,7 +535,6 @@ struct sctp_datamsg {
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
 					    struct sctp_sndrcvinfo *,
 					    struct iov_iter *);
-void sctp_datamsg_free(struct sctp_datamsg *);
 void sctp_datamsg_put(struct sctp_datamsg *);
 void sctp_chunk_fail(struct sctp_chunk *, int error);
 int sctp_chunk_abandoned(struct sctp_chunk *);
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index a338091..3aa4307 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -70,19 +70,6 @@ static struct sctp_datamsg *sctp_datamsg_new(gfp_t gfp)
 	return msg;
 }
 
-void sctp_datamsg_free(struct sctp_datamsg *msg)
-{
-	struct sctp_chunk *chunk;
-
-	/* This doesn't have to be a _safe vairant because
-	 * sctp_chunk_free() only drops the refs.
-	 */
-	list_for_each_entry(chunk, &msg->chunks, frag_list)
-		sctp_chunk_free(chunk);
-
-	sctp_datamsg_put(msg);
-}
-
 /* Final destructruction of datamsg memory. */
 static void sctp_datamsg_destroy(struct sctp_datamsg *msg)
 {
-- 
2.1.0

[PATCH net 3/3] sctp: remove the unused sctp_datamsg_free()

[Xin Long]

Since commit 8b570dc9f7b6 ("sctp: only drop the reference on the datamsg
after sending a msg") used sctp_datamsg_put in sctp_sendmsg, instead of
sctp_datamsg_free, this function has no use in sctp.

So we will remove it.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/net/sctp/structs.h |  1 -
 net/sctp/chunk.c           | 13 -------------
 2 files changed, 14 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 205630b..d05b566 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -535,7 +535,6 @@ struct sctp_datamsg {
 struct sctp_datamsg *sctp_datamsg_from_user(struct sctp_association *,
 					    struct sctp_sndrcvinfo *,
 					    struct iov_iter *);
-void sctp_datamsg_free(struct sctp_datamsg *);
 void sctp_datamsg_put(struct sctp_datamsg *);
 void sctp_chunk_fail(struct sctp_chunk *, int error);
 int sctp_chunk_abandoned(struct sctp_chunk *);
diff --git a/net/sctp/chunk.c b/net/sctp/chunk.c
index a338091..3aa4307 100644
--- a/net/sctp/chunk.c
+++ b/net/sctp/chunk.c
@@ -70,19 +70,6 @@ static struct sctp_datamsg *sctp_datamsg_new(gfp_t gfp)
 	return msg;
 }
 
-void sctp_datamsg_free(struct sctp_datamsg *msg)
-{
-	struct sctp_chunk *chunk;
-
-	/* This doesn't have to be a _safe vairant because
-	 * sctp_chunk_free() only drops the refs.
-	 */
-	list_for_each_entry(chunk, &msg->chunks, frag_list)
-		sctp_chunk_free(chunk);
-
-	sctp_datamsg_put(msg);
-}
-
 /* Final destructruction of datamsg memory. */
 static void sctp_datamsg_destroy(struct sctp_datamsg *msg)
 {
-- 
2.1.0

Re: [PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_associati

[Neil Horman]

On Mon, Feb 15, 2016 at 02:28:03PM +0800, Xin Long wrote:
> __sctp_lookup_association() is only invoked by sctp_v4_err() and
> sctp_rcv(), both which run on the rx BH, and it has been protected
> by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].
> 
> So we can move it to sctp_lookup_association, only let
> sctp_lookup_association use rcu_read_lock.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Not that it was right before if this is true, but if we're using rcu in a bottom
half path, shouldn't rcu_read_lock_bh be used instead here?

Neil

> ---
>  net/sctp/input.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 49d2cc7..21a2d6b 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -937,7 +937,6 @@ static struct sctp_association *__sctp_lookup_association(
>  	struct sctp_transport *t;
>  	struct sctp_association *asoc = NULL;
>  
> -	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
>  	if (!t || !sctp_transport_hold(t))
>  		goto out;
> @@ -949,7 +948,6 @@ static struct sctp_association *__sctp_lookup_association(
>  	sctp_transport_put(t);
>  
>  out:
> -	rcu_read_unlock();
>  	return asoc;
>  }
>  
> @@ -962,7 +960,9 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> +	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> +	rcu_read_unlock();
>  
>  	return asoc;
>  }
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_association

[Neil Horman]

On Mon, Feb 15, 2016 at 02:28:03PM +0800, Xin Long wrote:
> __sctp_lookup_association() is only invoked by sctp_v4_err() and
> sctp_rcv(), both which run on the rx BH, and it has been protected
> by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].
> 
> So we can move it to sctp_lookup_association, only let
> sctp_lookup_association use rcu_read_lock.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Not that it was right before if this is true, but if we're using rcu in a bottom
half path, shouldn't rcu_read_lock_bh be used instead here?

Neil

> ---
>  net/sctp/input.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 49d2cc7..21a2d6b 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -937,7 +937,6 @@ static struct sctp_association *__sctp_lookup_association(
>  	struct sctp_transport *t;
>  	struct sctp_association *asoc = NULL;
>  
> -	rcu_read_lock();
>  	t = sctp_addrs_lookup_transport(net, local, peer);
>  	if (!t || !sctp_transport_hold(t))
>  		goto out;
> @@ -949,7 +948,6 @@ static struct sctp_association *__sctp_lookup_association(
>  	sctp_transport_put(t);
>  
>  out:
> -	rcu_read_unlock();
>  	return asoc;
>  }
>  
> @@ -962,7 +960,9 @@ struct sctp_association *sctp_lookup_association(struct net *net,
>  {
>  	struct sctp_association *asoc;
>  
> +	rcu_read_lock();
>  	asoc = __sctp_lookup_association(net, laddr, paddr, transportp);
> +	rcu_read_unlock();
>  
>  	return asoc;
>  }
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_associati

[Xin Long]

On Thu, Feb 18, 2016 at 12:11 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Mon, Feb 15, 2016 at 02:28:03PM +0800, Xin Long wrote:
>> __sctp_lookup_association() is only invoked by sctp_v4_err() and
>> sctp_rcv(), both which run on the rx BH, and it has been protected
>> by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].
>>
>> So we can move it to sctp_lookup_association, only let
>> sctp_lookup_association use rcu_read_lock.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> Not that it was right before if this is true, but if we're using rcu in a bottom
> half path, shouldn't rcu_read_lock_bh be used instead here?
>
the resource is also accessed in threads, not only in BH, so we cannot
use rcu_read_lock_bh here.

> Neil
>

Re: [PATCH net 1/3] sctp: move rcu_read_lock from __sctp_lookup_association to sctp_lookup_association

[Xin Long]

On Thu, Feb 18, 2016 at 12:11 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Mon, Feb 15, 2016 at 02:28:03PM +0800, Xin Long wrote:
>> __sctp_lookup_association() is only invoked by sctp_v4_err() and
>> sctp_rcv(), both which run on the rx BH, and it has been protected
>> by rcu_read_lock [see ip_local_deliver_finish() / ipv6_rcv()].
>>
>> So we can move it to sctp_lookup_association, only let
>> sctp_lookup_association use rcu_read_lock.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>
> Not that it was right before if this is true, but if we're using rcu in a bottom
> half path, shouldn't rcu_read_lock_bh be used instead here?
>
the resource is also accessed in threads, not only in BH, so we cannot
use rcu_read_lock_bh here.

> Neil
>

Re: [PATCH net 0/3] sctp: some cleanups for sctp

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 15 Feb 2016 14:28:02 +0800

> There are some unused function and redundant rcu locks should be
> removed.

Series applied to net-next, please target cleanups to the appropriate
tree in the future.

Also, please respond to Neil Horman's query about the use of BH
RCU locking wrt. patch #1.

Thanks.

Re: [PATCH net 0/3] sctp: some cleanups for sctp

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 15 Feb 2016 14:28:02 +0800

> There are some unused function and redundant rcu locks should be
> removed.

Series applied to net-next, please target cleanups to the appropriate
tree in the future.

Also, please respond to Neil Horman's query about the use of BH
RCU locking wrt. patch #1.

Thanks.