All of lore.kernel.org
 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 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.