netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/6] tipc: do some clean ups
@ 2013-12-06  6:23 Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

This patch series do some small clean up in tipc. almost of them 
just code simplification, no functional changes.

Wang Weidong (6):
  tipc: add link_kfree_skbuff helper function
  tipc: remove the unnecessary variable
  tipc: use a same goto path
  tipc: Use <linux/uaccess.h> instead of <asm/uaccess.h>
  tipc: change lock_sock order in connect()
  tipc: separate the check nseq and sseq allocate failed

 net/tipc/core.h       |  2 +-
 net/tipc/link.c       | 58 +++++++++++++++++----------------------------------
 net/tipc/name_table.c | 14 ++++++++-----
 net/tipc/port.c       |  9 +++-----
 net/tipc/socket.c     | 33 +++++++++++------------------
 5 files changed, 44 insertions(+), 72 deletions(-)

-- 
1.7.12

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  2013-12-06  6:34   ` Ying Xue
                     ` (2 more replies)
  2013-12-06  6:23 ` [PATCH net-next 2/6] tipc: remove the unnecessary variable Wang Weidong
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

replaces some chunks of code that kfree the sk_buff.
This is just code simplification, no functional changes.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
 1 file changed, 19 insertions(+), 39 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 69cd9bf..1c27d7b 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
 	return (i + 3) & ~3u;
 }
 
+static void link_kfree_skbuff(struct sk_buff *buf)
+{
+	struct sk_buff *next;
+
+	while (buf) {
+		next = buf->next;
+		kfree_skb(buf);
+		buf = next;
+	}
+}
+
 static void link_init_max_pkt(struct tipc_link *l_ptr)
 {
 	u32 max_pkt;
@@ -387,13 +398,8 @@ exit:
 static void link_release_outqueue(struct tipc_link *l_ptr)
 {
 	struct sk_buff *buf = l_ptr->first_out;
-	struct sk_buff *next;
 
-	while (buf) {
-		next = buf->next;
-		kfree_skb(buf);
-		buf = next;
-	}
+	link_kfree_skbuff(buf);
 	l_ptr->first_out = NULL;
 	l_ptr->out_queue_size = 0;
 }
@@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
 void tipc_link_stop(struct tipc_link *l_ptr)
 {
 	struct sk_buff *buf;
-	struct sk_buff *next;
 
 	buf = l_ptr->oldest_deferred_in;
-	while (buf) {
-		next = buf->next;
-		kfree_skb(buf);
-		buf = next;
-	}
+	link_kfree_skbuff(buf);
 
 	buf = l_ptr->first_out;
-	while (buf) {
-		next = buf->next;
-		kfree_skb(buf);
-		buf = next;
-	}
+	link_kfree_skbuff(buf);
 
 	tipc_link_reset_fragments(l_ptr);
 
@@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
 	kfree_skb(l_ptr->proto_msg_queue);
 	l_ptr->proto_msg_queue = NULL;
 	buf = l_ptr->oldest_deferred_in;
-	while (buf) {
-		struct sk_buff *next = buf->next;
-		kfree_skb(buf);
-		buf = next;
-	}
+	link_kfree_skbuff(buf);
 	if (!list_empty(&l_ptr->waiting_ports))
 		tipc_link_wakeup_ports(l_ptr, 1);
 
@@ -1127,10 +1120,7 @@ again:
 		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
 			res = -EFAULT;
 error:
-			for (; buf_chain; buf_chain = buf) {
-				buf = buf_chain->next;
-				kfree_skb(buf_chain);
-			}
+			link_kfree_skbuff(buf_chain);
 			return res;
 		}
 		sect_crs += sz;
@@ -1180,18 +1170,12 @@ error:
 		if (l_ptr->max_pkt < max_pkt) {
 			sender->max_pkt = l_ptr->max_pkt;
 			tipc_node_unlock(node);
-			for (; buf_chain; buf_chain = buf) {
-				buf = buf_chain->next;
-				kfree_skb(buf_chain);
-			}
+			link_kfree_skbuff(buf_chain);
 			goto again;
 		}
 	} else {
 reject:
-		for (; buf_chain; buf_chain = buf) {
-			buf = buf_chain->next;
-			kfree_skb(buf_chain);
-		}
+		link_kfree_skbuff(buf_chain);
 		return tipc_port_reject_sections(sender, hdr, msg_sect,
 						 len, TIPC_ERR_NO_NODE);
 	}
@@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
 		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
 		if (fragm == NULL) {
 			kfree_skb(buf);
-			while (buf_chain) {
-				buf = buf_chain;
-				buf_chain = buf_chain->next;
-				kfree_skb(buf);
-			}
+			link_kfree_skbuff(buf_chain);
 			return -ENOMEM;
 		}
 		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next 2/6] tipc: remove the unnecessary variable
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 3/6] tipc: use a same goto path Wang Weidong
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

the sseq variable is unnecessary in tipc_subseq_alloc, so remove it.
sk in tipc_sock_create_local is never used, so remove it.
res in __tipc_disconnect is unnecessary, so remove it.
limit in rcvbuf_limit is unnecessary, so remove it.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/name_table.c |  3 +--
 net/tipc/port.c       |  9 +++------
 net/tipc/socket.c     | 13 ++++---------
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 09dcd54..92a1533 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -148,8 +148,7 @@ static struct publication *publ_create(u32 type, u32 lower, u32 upper,
  */
 static struct sub_seq *tipc_subseq_alloc(u32 cnt)
 {
-	struct sub_seq *sseq = kcalloc(cnt, sizeof(struct sub_seq), GFP_ATOMIC);
-	return sseq;
+	return kcalloc(cnt, sizeof(struct sub_seq), GFP_ATOMIC);
 }
 
 /**
diff --git a/net/tipc/port.c b/net/tipc/port.c
index c081a76..5fd4c8c 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -832,17 +832,14 @@ exit:
  */
 int __tipc_disconnect(struct tipc_port *tp_ptr)
 {
-	int res;
-
 	if (tp_ptr->connected) {
 		tp_ptr->connected = 0;
 		/* let timer expire on it's own to avoid deadlock! */
 		tipc_nodesub_unsubscribe(&tp_ptr->subscription);
-		res = 0;
-	} else {
-		res = -ENOTCONN;
+		return 0;
 	}
-	return res;
+
+	return -ENOTCONN;
 }
 
 /*
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 3b61851..32037c5 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -239,7 +239,6 @@ static int tipc_sk_create(struct net *net, struct socket *sock, int protocol,
 int tipc_sock_create_local(int type, struct socket **res)
 {
 	int rc;
-	struct sock *sk;
 
 	rc = sock_create_lite(AF_TIPC, type, 0, res);
 	if (rc < 0) {
@@ -248,8 +247,6 @@ int tipc_sock_create_local(int type, struct socket **res)
 	}
 	tipc_sk_create(&init_net, *res, 0, 1);
 
-	sk = (*res)->sk;
-
 	return 0;
 }
 
@@ -1311,14 +1308,12 @@ static u32 filter_connect(struct tipc_sock *tsock, struct sk_buff **buf)
 static unsigned int rcvbuf_limit(struct sock *sk, struct sk_buff *buf)
 {
 	struct tipc_msg *msg = buf_msg(buf);
-	unsigned int limit;
 
 	if (msg_connected(msg))
-		limit = sysctl_tipc_rmem[2];
-	else
-		limit = sk->sk_rcvbuf >> TIPC_CRITICAL_IMPORTANCE <<
-			msg_importance(msg);
-	return limit;
+		return sysctl_tipc_rmem[2];
+
+	return sk->sk_rcvbuf >> TIPC_CRITICAL_IMPORTANCE <<
+		msg_importance(msg);
 }
 
 /**
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next 3/6] tipc: use a same goto path
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 2/6] tipc: remove the unnecessary variable Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 4/6] tipc: Use <linux/uaccess.h> instead of <asm/uaccess.h> Wang Weidong
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

It will goto exit on every pach. so use one goto.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/socket.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 32037c5..844bf34 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -751,16 +751,14 @@ static int send_stream(struct kiocb *iocb, struct socket *sock,
 
 	/* Handle special cases where there is no connection */
 	if (unlikely(sock->state != SS_CONNECTED)) {
-		if (sock->state == SS_UNCONNECTED) {
+		res = -ENOTCONN;
+
+		if (sock->state == SS_UNCONNECTED)
 			res = send_packet(NULL, sock, m, total_len);
-			goto exit;
-		} else if (sock->state == SS_DISCONNECTING) {
+		else if (sock->state == SS_DISCONNECTING)
 			res = -EPIPE;
-			goto exit;
-		} else {
-			res = -ENOTCONN;
-			goto exit;
-		}
+
+		goto exit;
 	}
 
 	if (unlikely(m->msg_name)) {
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next 4/6] tipc: Use <linux/uaccess.h> instead of <asm/uaccess.h>
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
                   ` (2 preceding siblings ...)
  2013-12-06  6:23 ` [PATCH net-next 3/6] tipc: use a same goto path Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 5/6] tipc: change lock_sock order in connect() Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 6/6] tipc: separate the check nseq and sseq allocate failed Wang Weidong
  5 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

As warned by checkpatch.pl, use #include <linux/uaccess.h>
instead of <asm/uaccess.h>

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tipc/core.h b/net/tipc/core.h
index 94895d4..1ff477b 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -47,7 +47,7 @@
 #include <linux/mm.h>
 #include <linux/timer.h>
 #include <linux/string.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include <linux/interrupt.h>
 #include <linux/atomic.h>
 #include <asm/hardirq.h>
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next 5/6] tipc: change lock_sock order in connect()
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
                   ` (3 preceding siblings ...)
  2013-12-06  6:23 ` [PATCH net-next 4/6] tipc: Use <linux/uaccess.h> instead of <asm/uaccess.h> Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  2013-12-06  6:23 ` [PATCH net-next 6/6] tipc: separate the check nseq and sseq allocate failed Wang Weidong
  5 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

when res<=0, return the res no need to add lock_sock. so remove it.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/socket.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 844bf34..83f466e 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1507,14 +1507,12 @@ static int connect(struct socket *sock, struct sockaddr *dest, int destlen,
 				sock->state != SS_CONNECTING,
 				timeout ? (long)msecs_to_jiffies(timeout)
 					: MAX_SCHEDULE_TIMEOUT);
-		lock_sock(sk);
 		if (res <= 0) {
 			if (res == 0)
 				res = -ETIMEDOUT;
-			else
-				; /* leave "res" unchanged */
-			goto exit;
+			return res;
 		}
+		lock_sock(sk);
 	}
 
 	if (unlikely(sock->state == SS_DISCONNECTING))
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH net-next 6/6] tipc: separate the check nseq and sseq allocate failed
  2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
                   ` (4 preceding siblings ...)
  2013-12-06  6:23 ` [PATCH net-next 5/6] tipc: change lock_sock order in connect() Wang Weidong
@ 2013-12-06  6:23 ` Wang Weidong
  5 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:23 UTC (permalink / raw)
  To: jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

if nseq allocate failed, sseq is no need to alloc. so separate the
check.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/tipc/name_table.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 92a1533..b91387c 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -159,12 +159,17 @@ static struct sub_seq *tipc_subseq_alloc(u32 cnt)
 static struct name_seq *tipc_nameseq_create(u32 type, struct hlist_head *seq_head)
 {
 	struct name_seq *nseq = kzalloc(sizeof(*nseq), GFP_ATOMIC);
-	struct sub_seq *sseq = tipc_subseq_alloc(1);
+	struct sub_seq *sseq;
 
-	if (!nseq || !sseq) {
+	if (!nseq) {
 		pr_warn("Name sequence creation failed, no memory\n");
+		return NULL;
+	}
+
+	sseq = tipc_subseq_alloc(1);
+	if (!sseq) {
+		pr_warn("Sub sequence creation failed, no memory\n");
 		kfree(nseq);
-		kfree(sseq);
 		return NULL;
 	}
 
-- 
1.7.12

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
@ 2013-12-06  6:34   ` Ying Xue
  2013-12-06  6:42     ` Wang Weidong
  2013-12-06  6:44   ` Eric Dumazet
  2013-12-06  9:09   ` Daniel Borkmann
  2 siblings, 1 reply; 14+ messages in thread
From: Ying Xue @ 2013-12-06  6:34 UTC (permalink / raw)
  To: Wang Weidong, jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

On 12/06/2013 02:23 PM, Wang Weidong wrote:
> replaces some chunks of code that kfree the sk_buff.
> This is just code simplification, no functional changes.
> 
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>  net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>  1 file changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 69cd9bf..1c27d7b 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>  	return (i + 3) & ~3u;
>  }
>  
> +static void link_kfree_skbuff(struct sk_buff *buf)
> +{
> +	struct sk_buff *next;
> +
> +	while (buf) {
> +		next = buf->next;
> +		kfree_skb(buf);
> +		buf = next;
> +	}
> +}
> +

Your new defined function is unnecessary, instead we already have
another patch doing the same thing with kfree_skb_list(), and the patch
will be to be sent out soon.

Please see below link:

http://article.gmane.org/gmane.network.tipc.general/5140/

And the patch cleans up more things than your patch.

Regards,
Ying

>  static void link_init_max_pkt(struct tipc_link *l_ptr)
>  {
>  	u32 max_pkt;
> @@ -387,13 +398,8 @@ exit:
>  static void link_release_outqueue(struct tipc_link *l_ptr)
>  {
>  	struct sk_buff *buf = l_ptr->first_out;
> -	struct sk_buff *next;
>  
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>  	l_ptr->first_out = NULL;
>  	l_ptr->out_queue_size = 0;
>  }
> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>  void tipc_link_stop(struct tipc_link *l_ptr)
>  {
>  	struct sk_buff *buf;
> -	struct sk_buff *next;
>  
>  	buf = l_ptr->oldest_deferred_in;
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>  
>  	buf = l_ptr->first_out;
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>  
>  	tipc_link_reset_fragments(l_ptr);
>  
> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>  	kfree_skb(l_ptr->proto_msg_queue);
>  	l_ptr->proto_msg_queue = NULL;
>  	buf = l_ptr->oldest_deferred_in;
> -	while (buf) {
> -		struct sk_buff *next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>  	if (!list_empty(&l_ptr->waiting_ports))
>  		tipc_link_wakeup_ports(l_ptr, 1);
>  
> @@ -1127,10 +1120,7 @@ again:
>  		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>  			res = -EFAULT;
>  error:
> -			for (; buf_chain; buf_chain = buf) {
> -				buf = buf_chain->next;
> -				kfree_skb(buf_chain);
> -			}
> +			link_kfree_skbuff(buf_chain);
>  			return res;
>  		}
>  		sect_crs += sz;
> @@ -1180,18 +1170,12 @@ error:
>  		if (l_ptr->max_pkt < max_pkt) {
>  			sender->max_pkt = l_ptr->max_pkt;
>  			tipc_node_unlock(node);
> -			for (; buf_chain; buf_chain = buf) {
> -				buf = buf_chain->next;
> -				kfree_skb(buf_chain);
> -			}
> +			link_kfree_skbuff(buf_chain);
>  			goto again;
>  		}
>  	} else {
>  reject:
> -		for (; buf_chain; buf_chain = buf) {
> -			buf = buf_chain->next;
> -			kfree_skb(buf_chain);
> -		}
> +		link_kfree_skbuff(buf_chain);
>  		return tipc_port_reject_sections(sender, hdr, msg_sect,
>  						 len, TIPC_ERR_NO_NODE);
>  	}
> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>  		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>  		if (fragm == NULL) {
>  			kfree_skb(buf);
> -			while (buf_chain) {
> -				buf = buf_chain;
> -				buf_chain = buf_chain->next;
> -				kfree_skb(buf);
> -			}
> +			link_kfree_skbuff(buf_chain);
>  			return -ENOMEM;
>  		}
>  		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:34   ` Ying Xue
@ 2013-12-06  6:42     ` Wang Weidong
  2013-12-06 14:13       ` Jon Maloy
  0 siblings, 1 reply; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  6:42 UTC (permalink / raw)
  To: Ying Xue, jon.maloy, allan.stephens, davem; +Cc: netdev, tipc-discussion

On 2013/12/6 14:34, Ying Xue wrote:
> On 12/06/2013 02:23 PM, Wang Weidong wrote:
>> replaces some chunks of code that kfree the sk_buff.
>> This is just code simplification, no functional changes.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>  net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>>  1 file changed, 19 insertions(+), 39 deletions(-)
>>
>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>> index 69cd9bf..1c27d7b 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>>  	return (i + 3) & ~3u;
>>  }
>>  
>> +static void link_kfree_skbuff(struct sk_buff *buf)
>> +{
>> +	struct sk_buff *next;
>> +
>> +	while (buf) {
>> +		next = buf->next;
>> +		kfree_skb(buf);
>> +		buf = next;
>> +	}
>> +}
>> +
> 
> Your new defined function is unnecessary, instead we already have
> another patch doing the same thing with kfree_skb_list(), and the patch
> will be to be sent out soon.
> 
> Please see below link:
> 
> http://article.gmane.org/gmane.network.tipc.general/5140/
> 
> And the patch cleans up more things than your patch.
> 

Yes, You are right. 
Thanks.

> Regards,
> Ying
> 
>>  static void link_init_max_pkt(struct tipc_link *l_ptr)
>>  {
>>  	u32 max_pkt;
>> @@ -387,13 +398,8 @@ exit:
>>  static void link_release_outqueue(struct tipc_link *l_ptr)
>>  {
>>  	struct sk_buff *buf = l_ptr->first_out;
>> -	struct sk_buff *next;
>>  
>> -	while (buf) {
>> -		next = buf->next;
>> -		kfree_skb(buf);
>> -		buf = next;
>> -	}
>> +	link_kfree_skbuff(buf);
>>  	l_ptr->first_out = NULL;
>>  	l_ptr->out_queue_size = 0;
>>  }
>> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>>  void tipc_link_stop(struct tipc_link *l_ptr)
>>  {
>>  	struct sk_buff *buf;
>> -	struct sk_buff *next;
>>  
>>  	buf = l_ptr->oldest_deferred_in;
>> -	while (buf) {
>> -		next = buf->next;
>> -		kfree_skb(buf);
>> -		buf = next;
>> -	}
>> +	link_kfree_skbuff(buf);
>>  
>>  	buf = l_ptr->first_out;
>> -	while (buf) {
>> -		next = buf->next;
>> -		kfree_skb(buf);
>> -		buf = next;
>> -	}
>> +	link_kfree_skbuff(buf);
>>  
>>  	tipc_link_reset_fragments(l_ptr);
>>  
>> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>>  	kfree_skb(l_ptr->proto_msg_queue);
>>  	l_ptr->proto_msg_queue = NULL;
>>  	buf = l_ptr->oldest_deferred_in;
>> -	while (buf) {
>> -		struct sk_buff *next = buf->next;
>> -		kfree_skb(buf);
>> -		buf = next;
>> -	}
>> +	link_kfree_skbuff(buf);
>>  	if (!list_empty(&l_ptr->waiting_ports))
>>  		tipc_link_wakeup_ports(l_ptr, 1);
>>  
>> @@ -1127,10 +1120,7 @@ again:
>>  		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>>  			res = -EFAULT;
>>  error:
>> -			for (; buf_chain; buf_chain = buf) {
>> -				buf = buf_chain->next;
>> -				kfree_skb(buf_chain);
>> -			}
>> +			link_kfree_skbuff(buf_chain);
>>  			return res;
>>  		}
>>  		sect_crs += sz;
>> @@ -1180,18 +1170,12 @@ error:
>>  		if (l_ptr->max_pkt < max_pkt) {
>>  			sender->max_pkt = l_ptr->max_pkt;
>>  			tipc_node_unlock(node);
>> -			for (; buf_chain; buf_chain = buf) {
>> -				buf = buf_chain->next;
>> -				kfree_skb(buf_chain);
>> -			}
>> +			link_kfree_skbuff(buf_chain);
>>  			goto again;
>>  		}
>>  	} else {
>>  reject:
>> -		for (; buf_chain; buf_chain = buf) {
>> -			buf = buf_chain->next;
>> -			kfree_skb(buf_chain);
>> -		}
>> +		link_kfree_skbuff(buf_chain);
>>  		return tipc_port_reject_sections(sender, hdr, msg_sect,
>>  						 len, TIPC_ERR_NO_NODE);
>>  	}
>> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>>  		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>>  		if (fragm == NULL) {
>>  			kfree_skb(buf);
>> -			while (buf_chain) {
>> -				buf = buf_chain;
>> -				buf_chain = buf_chain->next;
>> -				kfree_skb(buf);
>> -			}
>> +			link_kfree_skbuff(buf_chain);
>>  			return -ENOMEM;
>>  		}
>>  		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
>>
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
  2013-12-06  6:34   ` Ying Xue
@ 2013-12-06  6:44   ` Eric Dumazet
  2013-12-06  9:09   ` Daniel Borkmann
  2 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2013-12-06  6:44 UTC (permalink / raw)
  To: Wang Weidong; +Cc: jon.maloy, allan.stephens, davem, netdev, tipc-discussion

On Fri, 2013-12-06 at 14:23 +0800, Wang Weidong wrote:
> replaces some chunks of code that kfree the sk_buff.
> This is just code simplification, no functional changes.
> 
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>  net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>  1 file changed, 19 insertions(+), 39 deletions(-)
> 
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 69cd9bf..1c27d7b 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>  	return (i + 3) & ~3u;
>  }
>  
> +static void link_kfree_skbuff(struct sk_buff *buf)
> +{
> +	struct sk_buff *next;
> +
> +	while (buf) {
> +		next = buf->next;
> +		kfree_skb(buf);
> +		buf = next;
> +	}
> +}
> +

Oh well, this looks like kfree_skb_list() to me.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
  2013-12-06  6:34   ` Ying Xue
  2013-12-06  6:44   ` Eric Dumazet
@ 2013-12-06  9:09   ` Daniel Borkmann
  2013-12-06  9:16     ` Wang Weidong
  2 siblings, 1 reply; 14+ messages in thread
From: Daniel Borkmann @ 2013-12-06  9:09 UTC (permalink / raw)
  To: Wang Weidong; +Cc: jon.maloy, allan.stephens, davem, netdev, tipc-discussion

On 12/06/2013 07:23 AM, Wang Weidong wrote:
> replaces some chunks of code that kfree the sk_buff.
> This is just code simplification, no functional changes.
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>   net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>   1 file changed, 19 insertions(+), 39 deletions(-)
>
> diff --git a/net/tipc/link.c b/net/tipc/link.c
> index 69cd9bf..1c27d7b 100644
> --- a/net/tipc/link.c
> +++ b/net/tipc/link.c
> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>   	return (i + 3) & ~3u;
>   }
>
> +static void link_kfree_skbuff(struct sk_buff *buf)
> +{
> +	struct sk_buff *next;
> +
> +	while (buf) {
> +		next = buf->next;
> +		kfree_skb(buf);
> +		buf = next;
> +	}
> +}

So kfree_skb_list() does not work for you ?

Also, in case we do not have such generic functions, they should
go to skbuff.c instead ...

>   static void link_init_max_pkt(struct tipc_link *l_ptr)
>   {
>   	u32 max_pkt;
> @@ -387,13 +398,8 @@ exit:
>   static void link_release_outqueue(struct tipc_link *l_ptr)
>   {
>   	struct sk_buff *buf = l_ptr->first_out;
> -	struct sk_buff *next;
>
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>   	l_ptr->first_out = NULL;
>   	l_ptr->out_queue_size = 0;
>   }
> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>   void tipc_link_stop(struct tipc_link *l_ptr)
>   {
>   	struct sk_buff *buf;
> -	struct sk_buff *next;
>
>   	buf = l_ptr->oldest_deferred_in;
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>
>   	buf = l_ptr->first_out;
> -	while (buf) {
> -		next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>
>   	tipc_link_reset_fragments(l_ptr);
>
> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>   	kfree_skb(l_ptr->proto_msg_queue);
>   	l_ptr->proto_msg_queue = NULL;
>   	buf = l_ptr->oldest_deferred_in;
> -	while (buf) {
> -		struct sk_buff *next = buf->next;
> -		kfree_skb(buf);
> -		buf = next;
> -	}
> +	link_kfree_skbuff(buf);
>   	if (!list_empty(&l_ptr->waiting_ports))
>   		tipc_link_wakeup_ports(l_ptr, 1);
>
> @@ -1127,10 +1120,7 @@ again:
>   		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>   			res = -EFAULT;
>   error:
> -			for (; buf_chain; buf_chain = buf) {
> -				buf = buf_chain->next;
> -				kfree_skb(buf_chain);
> -			}
> +			link_kfree_skbuff(buf_chain);
>   			return res;
>   		}
>   		sect_crs += sz;
> @@ -1180,18 +1170,12 @@ error:
>   		if (l_ptr->max_pkt < max_pkt) {
>   			sender->max_pkt = l_ptr->max_pkt;
>   			tipc_node_unlock(node);
> -			for (; buf_chain; buf_chain = buf) {
> -				buf = buf_chain->next;
> -				kfree_skb(buf_chain);
> -			}
> +			link_kfree_skbuff(buf_chain);
>   			goto again;
>   		}
>   	} else {
>   reject:
> -		for (; buf_chain; buf_chain = buf) {
> -			buf = buf_chain->next;
> -			kfree_skb(buf_chain);
> -		}
> +		link_kfree_skbuff(buf_chain);
>   		return tipc_port_reject_sections(sender, hdr, msg_sect,
>   						 len, TIPC_ERR_NO_NODE);
>   	}
> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>   		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>   		if (fragm == NULL) {
>   			kfree_skb(buf);
> -			while (buf_chain) {
> -				buf = buf_chain;
> -				buf_chain = buf_chain->next;
> -				kfree_skb(buf);
> -			}
> +			link_kfree_skbuff(buf_chain);
>   			return -ENOMEM;
>   		}
>   		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  9:09   ` Daniel Borkmann
@ 2013-12-06  9:16     ` Wang Weidong
  0 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-06  9:16 UTC (permalink / raw)
  To: Daniel Borkmann; +Cc: jon.maloy, allan.stephens, davem, netdev, tipc-discussion

On 2013/12/6 17:09, Daniel Borkmann wrote:
> On 12/06/2013 07:23 AM, Wang Weidong wrote:
>> replaces some chunks of code that kfree the sk_buff.
>> This is just code simplification, no functional changes.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>>   1 file changed, 19 insertions(+), 39 deletions(-)
>>
>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>> index 69cd9bf..1c27d7b 100644
>> --- a/net/tipc/link.c
>> +++ b/net/tipc/link.c
>> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>>       return (i + 3) & ~3u;
>>   }
>>
>> +static void link_kfree_skbuff(struct sk_buff *buf)
>> +{
>> +    struct sk_buff *next;
>> +
>> +    while (buf) {
>> +        next = buf->next;
>> +        kfree_skb(buf);
>> +        buf = next;
>> +    }
>> +}
> 
> So kfree_skb_list() does not work for you ?
> 
> Also, in case we do not have such generic functions, they should
> go to skbuff.c instead ...
> 
No, It is work for me. I just din't not the kfree_skb_list() and I found
that use a helper function can make code more simplification. So I do that.

Regards.
Wang


>>   static void link_init_max_pkt(struct tipc_link *l_ptr)
>>   {
>>       u32 max_pkt;
>> @@ -387,13 +398,8 @@ exit:
>>   static void link_release_outqueue(struct tipc_link *l_ptr)
>>   {
>>       struct sk_buff *buf = l_ptr->first_out;
>> -    struct sk_buff *next;
>>
>> -    while (buf) {
>> -        next = buf->next;
>> -        kfree_skb(buf);
>> -        buf = next;
>> -    }
>> +    link_kfree_skbuff(buf);
>>       l_ptr->first_out = NULL;
>>       l_ptr->out_queue_size = 0;
>>   }
>> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>>   void tipc_link_stop(struct tipc_link *l_ptr)
>>   {
>>       struct sk_buff *buf;
>> -    struct sk_buff *next;
>>
>>       buf = l_ptr->oldest_deferred_in;
>> -    while (buf) {
>> -        next = buf->next;
>> -        kfree_skb(buf);
>> -        buf = next;
>> -    }
>> +    link_kfree_skbuff(buf);
>>
>>       buf = l_ptr->first_out;
>> -    while (buf) {
>> -        next = buf->next;
>> -        kfree_skb(buf);
>> -        buf = next;
>> -    }
>> +    link_kfree_skbuff(buf);
>>
>>       tipc_link_reset_fragments(l_ptr);
>>
>> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>>       kfree_skb(l_ptr->proto_msg_queue);
>>       l_ptr->proto_msg_queue = NULL;
>>       buf = l_ptr->oldest_deferred_in;
>> -    while (buf) {
>> -        struct sk_buff *next = buf->next;
>> -        kfree_skb(buf);
>> -        buf = next;
>> -    }
>> +    link_kfree_skbuff(buf);
>>       if (!list_empty(&l_ptr->waiting_ports))
>>           tipc_link_wakeup_ports(l_ptr, 1);
>>
>> @@ -1127,10 +1120,7 @@ again:
>>           if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>>               res = -EFAULT;
>>   error:
>> -            for (; buf_chain; buf_chain = buf) {
>> -                buf = buf_chain->next;
>> -                kfree_skb(buf_chain);
>> -            }
>> +            link_kfree_skbuff(buf_chain);
>>               return res;
>>           }
>>           sect_crs += sz;
>> @@ -1180,18 +1170,12 @@ error:
>>           if (l_ptr->max_pkt < max_pkt) {
>>               sender->max_pkt = l_ptr->max_pkt;
>>               tipc_node_unlock(node);
>> -            for (; buf_chain; buf_chain = buf) {
>> -                buf = buf_chain->next;
>> -                kfree_skb(buf_chain);
>> -            }
>> +            link_kfree_skbuff(buf_chain);
>>               goto again;
>>           }
>>       } else {
>>   reject:
>> -        for (; buf_chain; buf_chain = buf) {
>> -            buf = buf_chain->next;
>> -            kfree_skb(buf_chain);
>> -        }
>> +        link_kfree_skbuff(buf_chain);
>>           return tipc_port_reject_sections(sender, hdr, msg_sect,
>>                            len, TIPC_ERR_NO_NODE);
>>       }
>> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>>           fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>>           if (fragm == NULL) {
>>               kfree_skb(buf);
>> -            while (buf_chain) {
>> -                buf = buf_chain;
>> -                buf_chain = buf_chain->next;
>> -                kfree_skb(buf);
>> -            }
>> +            link_kfree_skbuff(buf_chain);
>>               return -ENOMEM;
>>           }
>>           msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
>>
> 
> .
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06  6:42     ` Wang Weidong
@ 2013-12-06 14:13       ` Jon Maloy
  2013-12-07  4:52         ` Wang Weidong
  0 siblings, 1 reply; 14+ messages in thread
From: Jon Maloy @ 2013-12-06 14:13 UTC (permalink / raw)
  To: Wang Weidong; +Cc: netdev, tipc-discussion, allan.stephens, davem

Wang,
I am very happy to see you posting improvements to TIPC, but please synch up
with the TIPC development team (i.e., use tipc_discussion), before posting 
it to netdev. As Ying stated,we have a patch series in the pipe that deals 
with exactly this issue, and more.

Regards
///jon




On 12/06/2013 01:42 AM, Wang Weidong wrote:
> On 2013/12/6 14:34, Ying Xue wrote:
>> On 12/06/2013 02:23 PM, Wang Weidong wrote:
>>> replaces some chunks of code that kfree the sk_buff.
>>> This is just code simplification, no functional changes.
>>>
>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>> ---
>>>  net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>>>  1 file changed, 19 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>> index 69cd9bf..1c27d7b 100644
>>> --- a/net/tipc/link.c
>>> +++ b/net/tipc/link.c
>>> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>>>  	return (i + 3) & ~3u;
>>>  }
>>>  
>>> +static void link_kfree_skbuff(struct sk_buff *buf)
>>> +{
>>> +	struct sk_buff *next;
>>> +
>>> +	while (buf) {
>>> +		next = buf->next;
>>> +		kfree_skb(buf);
>>> +		buf = next;
>>> +	}
>>> +}
>>> +
>>
>> Your new defined function is unnecessary, instead we already have
>> another patch doing the same thing with kfree_skb_list(), and the patch
>> will be to be sent out soon.
>>
>> Please see below link:
>>
>> http://article.gmane.org/gmane.network.tipc.general/5140/
>>
>> And the patch cleans up more things than your patch.
>>
> 
> Yes, You are right. 
> Thanks.
> 
>> Regards,
>> Ying
>>
>>>  static void link_init_max_pkt(struct tipc_link *l_ptr)
>>>  {
>>>  	u32 max_pkt;
>>> @@ -387,13 +398,8 @@ exit:
>>>  static void link_release_outqueue(struct tipc_link *l_ptr)
>>>  {
>>>  	struct sk_buff *buf = l_ptr->first_out;
>>> -	struct sk_buff *next;
>>>  
>>> -	while (buf) {
>>> -		next = buf->next;
>>> -		kfree_skb(buf);
>>> -		buf = next;
>>> -	}
>>> +	link_kfree_skbuff(buf);
>>>  	l_ptr->first_out = NULL;
>>>  	l_ptr->out_queue_size = 0;
>>>  }
>>> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>>>  void tipc_link_stop(struct tipc_link *l_ptr)
>>>  {
>>>  	struct sk_buff *buf;
>>> -	struct sk_buff *next;
>>>  
>>>  	buf = l_ptr->oldest_deferred_in;
>>> -	while (buf) {
>>> -		next = buf->next;
>>> -		kfree_skb(buf);
>>> -		buf = next;
>>> -	}
>>> +	link_kfree_skbuff(buf);
>>>  
>>>  	buf = l_ptr->first_out;
>>> -	while (buf) {
>>> -		next = buf->next;
>>> -		kfree_skb(buf);
>>> -		buf = next;
>>> -	}
>>> +	link_kfree_skbuff(buf);
>>>  
>>>  	tipc_link_reset_fragments(l_ptr);
>>>  
>>> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>>>  	kfree_skb(l_ptr->proto_msg_queue);
>>>  	l_ptr->proto_msg_queue = NULL;
>>>  	buf = l_ptr->oldest_deferred_in;
>>> -	while (buf) {
>>> -		struct sk_buff *next = buf->next;
>>> -		kfree_skb(buf);
>>> -		buf = next;
>>> -	}
>>> +	link_kfree_skbuff(buf);
>>>  	if (!list_empty(&l_ptr->waiting_ports))
>>>  		tipc_link_wakeup_ports(l_ptr, 1);
>>>  
>>> @@ -1127,10 +1120,7 @@ again:
>>>  		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>>>  			res = -EFAULT;
>>>  error:
>>> -			for (; buf_chain; buf_chain = buf) {
>>> -				buf = buf_chain->next;
>>> -				kfree_skb(buf_chain);
>>> -			}
>>> +			link_kfree_skbuff(buf_chain);
>>>  			return res;
>>>  		}
>>>  		sect_crs += sz;
>>> @@ -1180,18 +1170,12 @@ error:
>>>  		if (l_ptr->max_pkt < max_pkt) {
>>>  			sender->max_pkt = l_ptr->max_pkt;
>>>  			tipc_node_unlock(node);
>>> -			for (; buf_chain; buf_chain = buf) {
>>> -				buf = buf_chain->next;
>>> -				kfree_skb(buf_chain);
>>> -			}
>>> +			link_kfree_skbuff(buf_chain);
>>>  			goto again;
>>>  		}
>>>  	} else {
>>>  reject:
>>> -		for (; buf_chain; buf_chain = buf) {
>>> -			buf = buf_chain->next;
>>> -			kfree_skb(buf_chain);
>>> -		}
>>> +		link_kfree_skbuff(buf_chain);
>>>  		return tipc_port_reject_sections(sender, hdr, msg_sect,
>>>  						 len, TIPC_ERR_NO_NODE);
>>>  	}
>>> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>>>  		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>>>  		if (fragm == NULL) {
>>>  			kfree_skb(buf);
>>> -			while (buf_chain) {
>>> -				buf = buf_chain;
>>> -				buf_chain = buf_chain->next;
>>> -				kfree_skb(buf);
>>> -			}
>>> +			link_kfree_skbuff(buf_chain);
>>>  			return -ENOMEM;
>>>  		}
>>>  		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
>>>
>>
>>
>> .
>>
> 
> 


------------------------------------------------------------------------------
Sponsored by Intel(R) XDK 
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function
  2013-12-06 14:13       ` Jon Maloy
@ 2013-12-07  4:52         ` Wang Weidong
  0 siblings, 0 replies; 14+ messages in thread
From: Wang Weidong @ 2013-12-07  4:52 UTC (permalink / raw)
  To: Jon Maloy; +Cc: Ying Xue, allan.stephens, davem, netdev, tipc-discussion

On 2013/12/6 22:13, Jon Maloy wrote:
> Wang,
> I am very happy to see you posting improvements to TIPC, but please synch up
> with the TIPC development team (i.e., use tipc_discussion), before posting 
> it to netdev. As Ying stated,we have a patch series in the pipe that deals 
> with exactly this issue, and more.
> 
> Regards
> ///jon
> 

Hi Jon,

Thanks for your reply. Now I am more clearly about how to do it.

Regards.
Wang

> 
> 
> 
> On 12/06/2013 01:42 AM, Wang Weidong wrote:
>> On 2013/12/6 14:34, Ying Xue wrote:
>>> On 12/06/2013 02:23 PM, Wang Weidong wrote:
>>>> replaces some chunks of code that kfree the sk_buff.
>>>> This is just code simplification, no functional changes.
>>>>
>>>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>>>> ---
>>>>  net/tipc/link.c | 58 +++++++++++++++++++--------------------------------------
>>>>  1 file changed, 19 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/net/tipc/link.c b/net/tipc/link.c
>>>> index 69cd9bf..1c27d7b 100644
>>>> --- a/net/tipc/link.c
>>>> +++ b/net/tipc/link.c
>>>> @@ -100,6 +100,17 @@ static unsigned int align(unsigned int i)
>>>>  	return (i + 3) & ~3u;
>>>>  }
>>>>  
>>>> +static void link_kfree_skbuff(struct sk_buff *buf)
>>>> +{
>>>> +	struct sk_buff *next;
>>>> +
>>>> +	while (buf) {
>>>> +		next = buf->next;
>>>> +		kfree_skb(buf);
>>>> +		buf = next;
>>>> +	}
>>>> +}
>>>> +
>>>
>>> Your new defined function is unnecessary, instead we already have
>>> another patch doing the same thing with kfree_skb_list(), and the patch
>>> will be to be sent out soon.
>>>
>>> Please see below link:
>>>
>>> http://article.gmane.org/gmane.network.tipc.general/5140/
>>>
>>> And the patch cleans up more things than your patch.
>>>
>>
>> Yes, You are right. 
>> Thanks.
>>
>>> Regards,
>>> Ying
>>>
>>>>  static void link_init_max_pkt(struct tipc_link *l_ptr)
>>>>  {
>>>>  	u32 max_pkt;
>>>> @@ -387,13 +398,8 @@ exit:
>>>>  static void link_release_outqueue(struct tipc_link *l_ptr)
>>>>  {
>>>>  	struct sk_buff *buf = l_ptr->first_out;
>>>> -	struct sk_buff *next;
>>>>  
>>>> -	while (buf) {
>>>> -		next = buf->next;
>>>> -		kfree_skb(buf);
>>>> -		buf = next;
>>>> -	}
>>>> +	link_kfree_skbuff(buf);
>>>>  	l_ptr->first_out = NULL;
>>>>  	l_ptr->out_queue_size = 0;
>>>>  }
>>>> @@ -416,21 +422,12 @@ void tipc_link_reset_fragments(struct tipc_link *l_ptr)
>>>>  void tipc_link_stop(struct tipc_link *l_ptr)
>>>>  {
>>>>  	struct sk_buff *buf;
>>>> -	struct sk_buff *next;
>>>>  
>>>>  	buf = l_ptr->oldest_deferred_in;
>>>> -	while (buf) {
>>>> -		next = buf->next;
>>>> -		kfree_skb(buf);
>>>> -		buf = next;
>>>> -	}
>>>> +	link_kfree_skbuff(buf);
>>>>  
>>>>  	buf = l_ptr->first_out;
>>>> -	while (buf) {
>>>> -		next = buf->next;
>>>> -		kfree_skb(buf);
>>>> -		buf = next;
>>>> -	}
>>>> +	link_kfree_skbuff(buf);
>>>>  
>>>>  	tipc_link_reset_fragments(l_ptr);
>>>>  
>>>> @@ -472,11 +469,7 @@ void tipc_link_reset(struct tipc_link *l_ptr)
>>>>  	kfree_skb(l_ptr->proto_msg_queue);
>>>>  	l_ptr->proto_msg_queue = NULL;
>>>>  	buf = l_ptr->oldest_deferred_in;
>>>> -	while (buf) {
>>>> -		struct sk_buff *next = buf->next;
>>>> -		kfree_skb(buf);
>>>> -		buf = next;
>>>> -	}
>>>> +	link_kfree_skbuff(buf);
>>>>  	if (!list_empty(&l_ptr->waiting_ports))
>>>>  		tipc_link_wakeup_ports(l_ptr, 1);
>>>>  
>>>> @@ -1127,10 +1120,7 @@ again:
>>>>  		if (copy_from_user(buf->data + fragm_crs, sect_crs, sz)) {
>>>>  			res = -EFAULT;
>>>>  error:
>>>> -			for (; buf_chain; buf_chain = buf) {
>>>> -				buf = buf_chain->next;
>>>> -				kfree_skb(buf_chain);
>>>> -			}
>>>> +			link_kfree_skbuff(buf_chain);
>>>>  			return res;
>>>>  		}
>>>>  		sect_crs += sz;
>>>> @@ -1180,18 +1170,12 @@ error:
>>>>  		if (l_ptr->max_pkt < max_pkt) {
>>>>  			sender->max_pkt = l_ptr->max_pkt;
>>>>  			tipc_node_unlock(node);
>>>> -			for (; buf_chain; buf_chain = buf) {
>>>> -				buf = buf_chain->next;
>>>> -				kfree_skb(buf_chain);
>>>> -			}
>>>> +			link_kfree_skbuff(buf_chain);
>>>>  			goto again;
>>>>  		}
>>>>  	} else {
>>>>  reject:
>>>> -		for (; buf_chain; buf_chain = buf) {
>>>> -			buf = buf_chain->next;
>>>> -			kfree_skb(buf_chain);
>>>> -		}
>>>> +		link_kfree_skbuff(buf_chain);
>>>>  		return tipc_port_reject_sections(sender, hdr, msg_sect,
>>>>  						 len, TIPC_ERR_NO_NODE);
>>>>  	}
>>>> @@ -2306,11 +2290,7 @@ static int link_send_long_buf(struct tipc_link *l_ptr, struct sk_buff *buf)
>>>>  		fragm = tipc_buf_acquire(fragm_sz + INT_H_SIZE);
>>>>  		if (fragm == NULL) {
>>>>  			kfree_skb(buf);
>>>> -			while (buf_chain) {
>>>> -				buf = buf_chain;
>>>> -				buf_chain = buf_chain->next;
>>>> -				kfree_skb(buf);
>>>> -			}
>>>> +			link_kfree_skbuff(buf_chain);
>>>>  			return -ENOMEM;
>>>>  		}
>>>>  		msg_set_size(&fragm_hdr, fragm_sz + INT_H_SIZE);
>>>>
>>>
>>>
>>> .
>>>
>>
>>
> 
> 
> .
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2013-12-07  4:56 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-06  6:23 [PATCH net-next 0/6] tipc: do some clean ups Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 1/6] tipc: add link_kfree_skbuff helper function Wang Weidong
2013-12-06  6:34   ` Ying Xue
2013-12-06  6:42     ` Wang Weidong
2013-12-06 14:13       ` Jon Maloy
2013-12-07  4:52         ` Wang Weidong
2013-12-06  6:44   ` Eric Dumazet
2013-12-06  9:09   ` Daniel Borkmann
2013-12-06  9:16     ` Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 2/6] tipc: remove the unnecessary variable Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 3/6] tipc: use a same goto path Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 4/6] tipc: Use <linux/uaccess.h> instead of <asm/uaccess.h> Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 5/6] tipc: change lock_sock order in connect() Wang Weidong
2013-12-06  6:23 ` [PATCH net-next 6/6] tipc: separate the check nseq and sseq allocate failed Wang Weidong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).