[PATCH net-next 0/2] sctp: some small clean ups

[PATCH net-next 0/2] sctp: some small clean ups

[Wang Weidong]

We have the macros in sctp.h, so use them for coding accordance
in sctp.

Wang Weidong (2):
  sctp: use sctp_local_bh_{disable|enable} instead
    local_bh_{disable|enable}
  sctp: use sctp_read_[un]lock instead of read_[un]lock

 net/sctp/endpointola.c   |  4 ++--
 net/sctp/input.c         | 10 +++++-----
 net/sctp/proc.c          | 12 ++++++------
 net/sctp/sm_make_chunk.c |  8 ++++----
 net/sctp/socket.c        |  8 ++++----
 5 files changed, 21 insertions(+), 21 deletions(-)

-- 
1.7.12

[PATCH net-next 1/2] sctp: use sctp_local_bh_{disable|enable} instead local_bh_{disable|enable}

[Wang Weidong]

While we have the macros sctp_local_bh_{disable|enable}, so use them.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/sm_make_chunk.c | 8 ++++----
 net/sctp/socket.c        | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 632090b..1bbac08 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3266,12 +3266,12 @@ static void sctp_asconf_param_success(struct sctp_association *asoc,
 		/* This is always done in BH context with a socket lock
 		 * held, so the list can not change.
 		 */
-		local_bh_disable();
+		sctp_local_bh_disable();
 		list_for_each_entry(saddr, &bp->address_list, list) {
 			if (sctp_cmp_addr_exact(&saddr->a, &addr))
 				saddr->state = SCTP_ADDR_SRC;
 		}
-		local_bh_enable();
+		sctp_local_bh_enable();
 		list_for_each_entry(transport, &asoc->peer.transport_addr_list,
 				transports) {
 			dst_release(transport->dst);
@@ -3279,14 +3279,14 @@ static void sctp_asconf_param_success(struct sctp_association *asoc,
 		}
 		break;
 	case SCTP_PARAM_DEL_IP:
-		local_bh_disable();
+		sctp_local_bh_disable();
 		sctp_del_bind_addr(bp, &addr);
 		if (asoc->asconf_addr_del_pending != NULL &&
 		    sctp_cmp_addr_exact(asoc->asconf_addr_del_pending, &addr)) {
 			kfree(asoc->asconf_addr_del_pending);
 			asoc->asconf_addr_del_pending = NULL;
 		}
-		local_bh_enable();
+		sctp_local_bh_enable();
 		list_for_each_entry(transport, &asoc->peer.transport_addr_list,
 				transports) {
 			dst_release(transport->dst);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index fd7337a..f73918c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3985,7 +3985,7 @@ static int sctp_init_sock(struct sock *sk)
 
 	SCTP_DBG_OBJCNT_INC(sock);
 
-	local_bh_disable();
+	sctp_local_bh_disable();
 	percpu_counter_inc(&sctp_sockets_allocated);
 	sock_prot_inuse_add(net, sk->sk_prot, 1);
 	if (net->sctp.default_auto_asconf) {
@@ -3994,7 +3994,7 @@ static int sctp_init_sock(struct sock *sk)
 		sp->do_auto_asconf = 1;
 	} else
 		sp->do_auto_asconf = 0;
-	local_bh_enable();
+	sctp_local_bh_enable();
 
 	return 0;
 }
@@ -4019,10 +4019,10 @@ static void sctp_destroy_sock(struct sock *sk)
 		list_del(&sp->auto_asconf_list);
 	}
 	sctp_endpoint_free(sp->ep);
-	local_bh_disable();
+	sctp_local_bh_disable();
 	percpu_counter_dec(&sctp_sockets_allocated);
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
-	local_bh_enable();
+	sctp_local_bh_enable();
 }
 
 /* Triggered when there are no references on the socket anymore */
-- 
1.7.12

[PATCH net-next 2/2] sctp: use sctp_read_[un]lock instead of read_[un]lock

[Wang Weidong]

While we have the macros sctp_read_[un]lock, so use them.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/endpointola.c |  4 ++--
 net/sctp/input.c       | 10 +++++-----
 net/sctp/proc.c        | 12 ++++++------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 6ffb6c1..68e1f91 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -342,7 +342,7 @@ static struct sctp_association *__sctp_endpoint_lookup_assoc(
 	hash = sctp_assoc_hashfn(sock_net(ep->base.sk), ep->base.bind_addr.port,
 				 rport);
 	head = &sctp_assoc_hashtable[hash];
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	sctp_for_each_hentry(epb, &head->chain) {
 		tmp = sctp_assoc(epb);
 		if (tmp->ep != ep || rport != tmp->peer.port)
@@ -355,7 +355,7 @@ static struct sctp_association *__sctp_endpoint_lookup_assoc(
 			break;
 		}
 	}
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 out:
 	return asoc;
 }
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 1f4eeb4..f22669c 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -768,7 +768,7 @@ static struct sctp_endpoint *__sctp_rcv_lookup_endpoint(struct net *net,
 
 	hash = sctp_ep_hashfn(net, ntohs(laddr->v4.sin_port));
 	head = &sctp_ep_hashtable[hash];
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	sctp_for_each_hentry(epb, &head->chain) {
 		ep = sctp_ep(epb);
 		if (sctp_endpoint_is_match(ep, net, laddr))
@@ -779,7 +779,7 @@ static struct sctp_endpoint *__sctp_rcv_lookup_endpoint(struct net *net,
 
 hit:
 	sctp_endpoint_hold(ep);
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 	return ep;
 }
 
@@ -863,7 +863,7 @@ static struct sctp_association *__sctp_lookup_association(
 	hash = sctp_assoc_hashfn(net, ntohs(local->v4.sin_port),
 				 ntohs(peer->v4.sin_port));
 	head = &sctp_assoc_hashtable[hash];
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	sctp_for_each_hentry(epb, &head->chain) {
 		asoc = sctp_assoc(epb);
 		transport = sctp_assoc_is_match(asoc, net, local, peer);
@@ -871,14 +871,14 @@ static struct sctp_association *__sctp_lookup_association(
 			goto hit;
 	}
 
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 
 	return NULL;
 
 hit:
 	*pt = transport;
 	sctp_association_hold(asoc);
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 	return asoc;
 }
 
diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 63ba0bd..bbba718 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -219,7 +219,7 @@ static int sctp_eps_seq_show(struct seq_file *seq, void *v)
 
 	head = &sctp_ep_hashtable[hash];
 	sctp_local_bh_disable();
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	sctp_for_each_hentry(epb, &head->chain) {
 		ep = sctp_ep(epb);
 		sk = epb->sk;
@@ -234,7 +234,7 @@ static int sctp_eps_seq_show(struct seq_file *seq, void *v)
 		sctp_seq_dump_local_addrs(seq, epb);
 		seq_printf(seq, "\n");
 	}
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 	sctp_local_bh_enable();
 
 	return 0;
@@ -327,7 +327,7 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 
 	head = &sctp_assoc_hashtable[hash];
 	sctp_local_bh_disable();
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	sctp_for_each_hentry(epb, &head->chain) {
 		assoc = sctp_assoc(epb);
 		sk = epb->sk;
@@ -361,7 +361,7 @@ static int sctp_assocs_seq_show(struct seq_file *seq, void *v)
 			sk->sk_rcvbuf);
 		seq_printf(seq, "\n");
 	}
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 	sctp_local_bh_enable();
 
 	return 0;
@@ -447,7 +447,7 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 
 	head = &sctp_assoc_hashtable[hash];
 	sctp_local_bh_disable();
-	read_lock(&head->lock);
+	sctp_read_lock(&head->lock);
 	rcu_read_lock();
 	sctp_for_each_hentry(epb, &head->chain) {
 		if (!net_eq(sock_net(epb->sk), seq_file_net(seq)))
@@ -504,7 +504,7 @@ static int sctp_remaddr_seq_show(struct seq_file *seq, void *v)
 	}
 
 	rcu_read_unlock();
-	read_unlock(&head->lock);
+	sctp_read_unlock(&head->lock);
 	sctp_local_bh_enable();
 
 	return 0;
-- 
1.7.12

Re: [PATCH net-next 0/2] sctp: some small clean ups

[Daniel Borkmann]

On 01/20/2014 12:27 PM, Wang Weidong wrote:
> We have the macros in sctp.h, so use them for coding accordance
> in sctp.

Thanks for doing this Wang.

I am actually wondering why we have these macro locking wrappers
and not use these functions directly? Hm, any reasons? Maybe we
should rather go in the other direction with this?

> Wang Weidong (2):
>    sctp: use sctp_local_bh_{disable|enable} instead
>      local_bh_{disable|enable}
>    sctp: use sctp_read_[un]lock instead of read_[un]lock
>
>   net/sctp/endpointola.c   |  4 ++--
>   net/sctp/input.c         | 10 +++++-----
>   net/sctp/proc.c          | 12 ++++++------
>   net/sctp/sm_make_chunk.c |  8 ++++----
>   net/sctp/socket.c        |  8 ++++----
>   5 files changed, 21 insertions(+), 21 deletions(-)
>

Re: [PATCH net-next 0/2] sctp: some small clean ups

[Wang Weidong]

On 2014/1/20 19:37, Daniel Borkmann wrote:
> On 01/20/2014 12:27 PM, Wang Weidong wrote:
>> We have the macros in sctp.h, so use them for coding accordance
>> in sctp.
> 
> Thanks for doing this Wang.
> 
> I am actually wondering why we have these macro locking wrappers

Here, I didn't found any description about it. In v2.6.12-rc2, the sctp
use these macros.

> and not use these functions directly? Hm, any reasons? Maybe we
> should rather go in the other direction with this?
> 

Yeah. 
Should I do the other direction for clean ups now?

Regards,
Wang

>> Wang Weidong (2):
>>    sctp: use sctp_local_bh_{disable|enable} instead
>>      local_bh_{disable|enable}
>>    sctp: use sctp_read_[un]lock instead of read_[un]lock
>>
>>   net/sctp/endpointola.c   |  4 ++--
>>   net/sctp/input.c         | 10 +++++-----
>>   net/sctp/proc.c          | 12 ++++++------
>>   net/sctp/sm_make_chunk.c |  8 ++++----
>>   net/sctp/socket.c        |  8 ++++----
>>   5 files changed, 21 insertions(+), 21 deletions(-)
>>
> 
> 

Re: [PATCH net-next 0/2] sctp: some small clean ups

[Neil Horman]

On Mon, Jan 20, 2014 at 12:37:06PM +0100, Daniel Borkmann wrote:
> On 01/20/2014 12:27 PM, Wang Weidong wrote:
> >We have the macros in sctp.h, so use them for coding accordance
> >in sctp.
> 
> Thanks for doing this Wang.
> 
> I am actually wondering why we have these macro locking wrappers
> and not use these functions directly? Hm, any reasons? Maybe we
> should rather go in the other direction with this?
> 
Its because in the origional implementation of the sctp protocol, there was a
user space test harness which built the kernel module for userspace execution to
cary our some unit testing on the code.  It did so by redefining some of those
locking macros to user space friendly code.  IIRC we haven't use those unit
tests in years, and so should be removing them, not adding them to other
locations.

Neil

> >Wang Weidong (2):
> >   sctp: use sctp_local_bh_{disable|enable} instead
> >     local_bh_{disable|enable}
> >   sctp: use sctp_read_[un]lock instead of read_[un]lock
> >
> >  net/sctp/endpointola.c   |  4 ++--
> >  net/sctp/input.c         | 10 +++++-----
> >  net/sctp/proc.c          | 12 ++++++------
> >  net/sctp/sm_make_chunk.c |  8 ++++----
> >  net/sctp/socket.c        |  8 ++++----
> >  5 files changed, 21 insertions(+), 21 deletions(-)
> >
> 

Re: [PATCH net-next 0/2] sctp: some small clean ups

[Wang Weidong]

On 2014/1/20 20:20, Neil Horman wrote:
> On Mon, Jan 20, 2014 at 12:37:06PM +0100, Daniel Borkmann wrote:
>> On 01/20/2014 12:27 PM, Wang Weidong wrote:
>>> We have the macros in sctp.h, so use them for coding accordance
>>> in sctp.
>>
>> Thanks for doing this Wang.
>>
>> I am actually wondering why we have these macro locking wrappers
>> and not use these functions directly? Hm, any reasons? Maybe we
>> should rather go in the other direction with this?
>>
> Its because in the origional implementation of the sctp protocol, there was a
> user space test harness which built the kernel module for userspace execution to
> cary our some unit testing on the code.  It did so by redefining some of those
> locking macros to user space friendly code.  IIRC we haven't use those unit
> tests in years, and so should be removing them, not adding them to other
> locations.
> 

Thanks for your answers.

I will send the patches with removing these macros soon.

Regards,
Wang

> Neil
> 
>>> Wang Weidong (2):
>>>   sctp: use sctp_local_bh_{disable|enable} instead
>>>     local_bh_{disable|enable}
>>>   sctp: use sctp_read_[un]lock instead of read_[un]lock
>>>
>>>  net/sctp/endpointola.c   |  4 ++--
>>>  net/sctp/input.c         | 10 +++++-----
>>>  net/sctp/proc.c          | 12 ++++++------
>>>  net/sctp/sm_make_chunk.c |  8 ++++----
>>>  net/sctp/socket.c        |  8 ++++----
>>>  5 files changed, 21 insertions(+), 21 deletions(-)
>>>
>>
> 
> .
>