sctp: some cleanups for sctp

PATCH In review Xin Long 3 participants 7 messages Feb 15, 2016 – Feb 18, 2016

Subsystems (3)

Networking Network drivers SCTP protocol

lore

Share & tools

lore

Xin Long

author maintainer

2016-02-15 06:28 UTC #0 lore raw

cover sctp: some cleanups for sctp

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(-)

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-02-15 06:28 UTC #1 lore raw

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

__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 <[email protected]>

---
 net/sctp/input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/sctp/input.c +2 -2

index 49d2cc7..21a2d6b 100644
@@ -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;
 }

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-02-15 06:28 UTC #2 lore raw

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

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 <[email protected]>

---
 net/sctp/proc.c | 2 --
 1 file changed, 2 deletions(-)

net/sctp/proc.c +0 -2

index ded7d93..cfc3c71 100644
@@ -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)

Signature · 1 line

2.1.0

Xin Long

author maintainer

2016-02-15 06:28 UTC #3 lore raw

PATCH 3/3 sctp: remove the unused sctp_datamsg_free()

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 <[email protected]>

---
 include/net/sctp/structs.h |  1 -
 net/sctp/chunk.c           | 13 -------------
 2 files changed, 14 deletions(-)

include/net/sctp/structs.h +0 -1

index 205630b..d05b566 100644
@@ -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 *);

net/sctp/chunk.c +0 ~~-13~~

index a338091..3aa4307 100644
@@ -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)
 {

Signature · 1 line

2.1.0

Neil Horman

2016-02-17 16:11 UTC #4 lore raw

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

In reply to Xin Long

Quoted 8 lines · 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 <[email protected]>

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

Quoted 42 lines

---
 net/sctp/input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/sctp/input.c +2 -2

index 49d2cc7..21a2d6b 100644
@@ -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;
 }

Signature · 6 lines

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2016-02-17 20:42 UTC #6 lore raw

In reply to Xin Long

From: Xin Long <[email protected]>
Date: Mon, 15 Feb 2016 14:28:02 +0800

Quoted 2 lines

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.

Xin Long

author maintainer

2016-02-18 13:59 UTC #5 lore raw

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

In reply to Neil Horman

Quoted 5 lines · Neil Horman

Quoted 4 lines · 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()].

Quoted 8 lines

Quoted 4 lines
So we can move it to sctp_lookup_association, only let sctp_lookup_association use rcu_read_lock.
Signed-off-by Xin Long <[email protected]>
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.

Quoted 2 lines

Neil