Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH net-next] tcp: tcp_try_coalesce returns a boolean
From: David Miller @ 2012-04-24  3:37 UTC (permalink / raw)
  To: eric.dumazet; +Cc: ncardwell, netdev, therbert, maze, ilpo.jarvinen
In-Reply-To: <1335238476.5205.122.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 Apr 2012 05:34:36 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> This clarifies code intention, as suggested by David.
> 
> Suggested-by: David Miller <davem@davemloft.net>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next] net: speedup skb_splice_bits()
From: David Miller @ 2012-04-24  3:35 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, therbert, axboe
In-Reply-To: <1335237503.5205.113.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 Apr 2012 05:18:23 +0200

> linear could be a bool too 

Yep.

> [PATCH] net: make spd_fill_page() linear argument a bool
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks!

^ permalink raw reply

* [PATCH net-next] tcp: tcp_try_coalesce returns a boolean
From: Eric Dumazet @ 2012-04-24  3:34 UTC (permalink / raw)
  To: David Miller; +Cc: ncardwell, netdev, therbert, maze, ilpo.jarvinen
In-Reply-To: <20120423.224602.594997774992725103.davem@davemloft.net>

From: Eric Dumazet <edumazet@google.com>

This clarifies code intention, as suggested by David.

Suggested-by: David Miller <davem@davemloft.net>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_input.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bd7aef5..67f352a 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4459,23 +4459,23 @@ static inline int tcp_try_rmem_schedule(struct sock *sk, unsigned int size)
  * to reduce overall memory use and queue lengths, if cost is small.
  * Packets in ofo or receive queues can stay a long time.
  * Better try to coalesce them right now to avoid future collapses.
- * Returns > 0 value if caller should free @from instead of queueing it
+ * Returns true if caller should free @from instead of queueing it
  */
-static int tcp_try_coalesce(struct sock *sk,
-			    struct sk_buff *to,
-			    struct sk_buff *from)
+static bool tcp_try_coalesce(struct sock *sk,
+			     struct sk_buff *to,
+			     struct sk_buff *from)
 {
 	int len = from->len;
 
 	if (tcp_hdr(from)->fin)
-		return 0;
+		return false;
 	if (len <= skb_tailroom(to)) {
 		BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
 merge:
 		NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
 		TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
 		TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
-		return 1;
+		return true;
 	}
 	if (skb_headlen(from) == 0 &&
 	    !skb_has_frag_list(to) &&
@@ -4498,7 +4498,7 @@ merge:
 		to->data_len += len;
 		goto merge;
 	}
-	return 0;
+	return false;
 }
 
 static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
@@ -4539,7 +4539,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
 	end_seq = TCP_SKB_CB(skb)->end_seq;
 
 	if (seq == TCP_SKB_CB(skb1)->end_seq) {
-		if (tcp_try_coalesce(sk, skb1, skb) <= 0) {
+		if (!tcp_try_coalesce(sk, skb1, skb)) {
 			__skb_queue_after(&tp->out_of_order_queue, skb1, skb);
 		} else {
 			__kfree_skb(skb);
@@ -4671,7 +4671,7 @@ queue_and_out:
 				goto drop;
 
 			tail = skb_peek_tail(&sk->sk_receive_queue);
-			eaten = tail ? tcp_try_coalesce(sk, tail, skb) : -1;
+			eaten = (tail && tcp_try_coalesce(sk, tail, skb)) ? 1 : 0;
 			if (eaten <= 0) {
 				skb_set_owner_r(skb, sk);
 				__skb_queue_tail(&sk->sk_receive_queue, skb);

^ permalink raw reply related

* Re: [PATCH net-next] net: speedup skb_splice_bits()
From: Eric Dumazet @ 2012-04-24  3:18 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, therbert, axboe
In-Reply-To: <20120423.230641.256329442020745539.davem@davemloft.net>

On Mon, 2012-04-23 at 23:06 -0400, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Mon, 23 Apr 2012 23:02:05 -0400 (EDT)
> 
> > I'm going to cleanup spd_fill_page() to return a bool.
> 
> Done, thusly:
> 
> --------------------
> net: Use bool and remove inline in skb_splice_bits() code.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/core/skbuff.c |   58 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index dfb3040..aaf4abc 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1547,9 +1547,9 @@ static void sock_spd_release(struct splice_pipe_desc *spd, unsigned int i)
>  	put_page(spd->pages[i]);
>  }
>  
> -static inline struct page *linear_to_page(struct page *page, unsigned int *len,
> -					  unsigned int *offset,
> -					  struct sk_buff *skb, struct sock *sk)
> +static struct page *linear_to_page(struct page *page, unsigned int *len,
> +				   unsigned int *offset,
> +				   struct sk_buff *skb, struct sock *sk)
>  {
>  	struct page *p = sk->sk_sndmsg_page;
>  	unsigned int off;
> @@ -1598,23 +1598,23 @@ static bool spd_can_coalesce(const struct splice_pipe_desc *spd,
>  /*
>   * Fill page/offset/length into spd, if it can hold more pages.
>   */
> -static inline int spd_fill_page(struct splice_pipe_desc *spd,
> -				struct pipe_inode_info *pipe, struct page *page,
> -				unsigned int *len, unsigned int offset,
> -				struct sk_buff *skb, int linear,
> -				struct sock *sk)
> +static bool spd_fill_page(struct splice_pipe_desc *spd,
> +			  struct pipe_inode_info *pipe, struct page *page,
> +			  unsigned int *len, unsigned int offset,
> +			  struct sk_buff *skb, int linear,

linear could be a bool too 

[PATCH] net: make spd_fill_page() linear argument a bool

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/skbuff.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index aaf4abc..2342a72 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1601,7 +1601,7 @@ static bool spd_can_coalesce(const struct splice_pipe_desc *spd,
 static bool spd_fill_page(struct splice_pipe_desc *spd,
 			  struct pipe_inode_info *pipe, struct page *page,
 			  unsigned int *len, unsigned int offset,
-			  struct sk_buff *skb, int linear,
+			  struct sk_buff *skb, bool linear,
 			  struct sock *sk)
 {
 	if (unlikely(spd->nr_pages == MAX_SKB_FRAGS))
@@ -1642,7 +1642,7 @@ static inline void __segment_seek(struct page **page, unsigned int *poff,
 static bool __splice_segment(struct page *page, unsigned int poff,
 			     unsigned int plen, unsigned int *off,
 			     unsigned int *len, struct sk_buff *skb,
-			     struct splice_pipe_desc *spd, int linear,
+			     struct splice_pipe_desc *spd, bool linear,
 			     struct sock *sk,
 			     struct pipe_inode_info *pipe)
 {
@@ -1694,7 +1694,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 	if (__splice_segment(virt_to_page(skb->data),
 			     (unsigned long) skb->data & (PAGE_SIZE - 1),
 			     skb_headlen(skb),
-			     offset, len, skb, spd, 1, sk, pipe))
+			     offset, len, skb, spd, true, sk, pipe))
 		return true;
 
 	/*
@@ -1705,7 +1705,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 
 		if (__splice_segment(skb_frag_page(f),
 				     f->page_offset, skb_frag_size(f),
-				     offset, len, skb, spd, 0, sk, pipe))
+				     offset, len, skb, spd, false, sk, pipe))
 			return true;
 	}
 

^ permalink raw reply related

* Re: [PATCH v1 6/8] dmaengine: enhance network subsystem to support DMA device hotplug
From: Dan Williams @ 2012-04-24  3:09 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Jiang Liu, Vinod Koul, Keping Chen, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, linux-pci, linux-kernel
In-Reply-To: <4F961041.7010106@huawei.com>

On Mon, Apr 23, 2012 at 7:30 PM, Jiang Liu <jiang.liu@huawei.com> wrote:
>> If you are going to hotplug the entire IOH, then you are probably ok
>> with network links going down, so could you just down the links and
>> remove the driver with the existing code?
>
> I feel it's a little risky to shut down/restart all network interfaces
> for hot-removal of IOH, that may disturb the applications.

I guess I'm confused... wouldn't the removal of an entire domain of
pci devices disturb userspace applications?

> And there
> are also other kinds of clients, such as ASYNC_TX, seems we can't
> adopt this method to reclaim DMA channels from ASYNC_TX subsystem.

I say handle this like block device hotplug.  I.e. the driver stays
loaded but the channel is put into an 'offline' state.  So the driver
hides the fact that the hardware went away.  Similar to how you can
remove a disk but /dev/sda sticks around until the last reference is
gone (and the driver 'sd' sticks around until all block devices are
gone).

I expect the work will be in making sure existing clients are prepared
to handle NULL returns from ->device_prep_dma_*.  In some cases the
channel is treated more like a cpu, so a NULL return from
->device_prep_dma_memcpy() has been interpreted as "device is
temporarily busy, it is safe to try again".  We would need to change
that to a permanent indication that the device is gone and not attempt
retry.

--
Dan

^ permalink raw reply

* Re: [PATCH net-next] net: speedup skb_splice_bits()
From: David Miller @ 2012-04-24  3:06 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, therbert, axboe
In-Reply-To: <20120423.230205.418261556142199877.davem@davemloft.net>

From: David Miller <davem@davemloft.net>
Date: Mon, 23 Apr 2012 23:02:05 -0400 (EDT)

> I'm going to cleanup spd_fill_page() to return a bool.

Done, thusly:

--------------------
net: Use bool and remove inline in skb_splice_bits() code.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/core/skbuff.c |   58 ++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index dfb3040..aaf4abc 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1547,9 +1547,9 @@ static void sock_spd_release(struct splice_pipe_desc *spd, unsigned int i)
 	put_page(spd->pages[i]);
 }
 
-static inline struct page *linear_to_page(struct page *page, unsigned int *len,
-					  unsigned int *offset,
-					  struct sk_buff *skb, struct sock *sk)
+static struct page *linear_to_page(struct page *page, unsigned int *len,
+				   unsigned int *offset,
+				   struct sk_buff *skb, struct sock *sk)
 {
 	struct page *p = sk->sk_sndmsg_page;
 	unsigned int off;
@@ -1598,23 +1598,23 @@ static bool spd_can_coalesce(const struct splice_pipe_desc *spd,
 /*
  * Fill page/offset/length into spd, if it can hold more pages.
  */
-static inline int spd_fill_page(struct splice_pipe_desc *spd,
-				struct pipe_inode_info *pipe, struct page *page,
-				unsigned int *len, unsigned int offset,
-				struct sk_buff *skb, int linear,
-				struct sock *sk)
+static bool spd_fill_page(struct splice_pipe_desc *spd,
+			  struct pipe_inode_info *pipe, struct page *page,
+			  unsigned int *len, unsigned int offset,
+			  struct sk_buff *skb, int linear,
+			  struct sock *sk)
 {
 	if (unlikely(spd->nr_pages == MAX_SKB_FRAGS))
-		return 1;
+		return true;
 
 	if (linear) {
 		page = linear_to_page(page, len, &offset, skb, sk);
 		if (!page)
-			return 1;
+			return true;
 	}
 	if (spd_can_coalesce(spd, page, offset)) {
 		spd->partial[spd->nr_pages - 1].len += *len;
-		return 0;
+		return false;
 	}
 	get_page(page);
 	spd->pages[spd->nr_pages] = page;
@@ -1622,7 +1622,7 @@ static inline int spd_fill_page(struct splice_pipe_desc *spd,
 	spd->partial[spd->nr_pages].offset = offset;
 	spd->nr_pages++;
 
-	return 0;
+	return false;
 }
 
 static inline void __segment_seek(struct page **page, unsigned int *poff,
@@ -1639,20 +1639,20 @@ static inline void __segment_seek(struct page **page, unsigned int *poff,
 	*plen -= off;
 }
 
-static inline int __splice_segment(struct page *page, unsigned int poff,
-				   unsigned int plen, unsigned int *off,
-				   unsigned int *len, struct sk_buff *skb,
-				   struct splice_pipe_desc *spd, int linear,
-				   struct sock *sk,
-				   struct pipe_inode_info *pipe)
+static bool __splice_segment(struct page *page, unsigned int poff,
+			     unsigned int plen, unsigned int *off,
+			     unsigned int *len, struct sk_buff *skb,
+			     struct splice_pipe_desc *spd, int linear,
+			     struct sock *sk,
+			     struct pipe_inode_info *pipe)
 {
 	if (!*len)
-		return 1;
+		return true;
 
 	/* skip this segment if already processed */
 	if (*off >= plen) {
 		*off -= plen;
-		return 0;
+		return false;
 	}
 
 	/* ignore any bits we already processed */
@@ -1668,23 +1668,23 @@ static inline int __splice_segment(struct page *page, unsigned int poff,
 		flen = min_t(unsigned int, flen, PAGE_SIZE - poff);
 
 		if (spd_fill_page(spd, pipe, page, &flen, poff, skb, linear, sk))
-			return 1;
+			return true;
 
 		__segment_seek(&page, &poff, &plen, flen);
 		*len -= flen;
 
 	} while (*len && plen);
 
-	return 0;
+	return false;
 }
 
 /*
- * Map linear and fragment data from the skb to spd. It reports failure if the
+ * Map linear and fragment data from the skb to spd. It reports true if the
  * pipe is full or if we already spliced the requested length.
  */
-static int __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
-			     unsigned int *offset, unsigned int *len,
-			     struct splice_pipe_desc *spd, struct sock *sk)
+static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
+			      unsigned int *offset, unsigned int *len,
+			      struct splice_pipe_desc *spd, struct sock *sk)
 {
 	int seg;
 
@@ -1695,7 +1695,7 @@ static int __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 			     (unsigned long) skb->data & (PAGE_SIZE - 1),
 			     skb_headlen(skb),
 			     offset, len, skb, spd, 1, sk, pipe))
-		return 1;
+		return true;
 
 	/*
 	 * then map the fragments
@@ -1706,10 +1706,10 @@ static int __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
 		if (__splice_segment(skb_frag_page(f),
 				     f->page_offset, skb_frag_size(f),
 				     offset, len, skb, spd, 0, sk, pipe))
-			return 1;
+			return true;
 	}
 
-	return 0;
+	return false;
 }
 
 /*
-- 
1.7.10

^ permalink raw reply related

* Re: [PATCH net-next] net: speedup skb_splice_bits()
From: David Miller @ 2012-04-24  3:02 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, therbert, axboe
In-Reply-To: <1335133576.2395.409.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 23 Apr 2012 00:26:16 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> Commit 35f3d14db (pipe: add support for shrinking and growing pipes)
> added a slowdown for splice(socket -> pipe), as we might grow the spd
> used in skb_splice_bits() for each skb we process in splice() syscall.
> 
> Its not needed since skb lengths are capped. The default on-stack arrays
> are more than enough.
> 
> Use MAX_SKB_FRAGS instead of PIPE_DEF_BUFFERS to describe the reasonable
> limit per skb.
> 
> Add coalescing support to help splicing of GRO skbs built from linear
> skbs (linked into frag_list)
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Tom Herbert <therbert@google.com>

Applied thanks.

I'm going to cleanup spd_fill_page() to return a bool.

^ permalink raw reply

* Re: [PATCH net-next] tcp: introduce tcp_try_coalesce
From: Eric Dumazet @ 2012-04-24  2:59 UTC (permalink / raw)
  To: David Miller; +Cc: ncardwell, netdev, therbert, maze, ilpo.jarvinen
In-Reply-To: <20120423.224602.594997774992725103.davem@davemloft.net>

On Mon, 2012-04-23 at 22:46 -0400, David Miller wrote:

> Applied, thanks Eric.
> 
> Although I'd like to ask you to clean up tcp_try_coalesce() a bit.
> 
> It effectively returns a boolean, but you've clouded this up by
> returning an int and defining it in the comment to return "> 0" or
> not.
> 
> Just make it return a real bool.
> 
> I know why you did this, it makes the "eaten" code somewhat simpler in
> tcp_data_queue(), but overall it's more confusing how it is now.
> 
> People look at how the tcp_try_coalesce() return value is interpreted
> and say "in what cases can it return a negative value?"  We both know
> it can't, but you have to read the entire function to figure that out.
> 
> And that's by definition not intuitive.
> 
> Thanks.

Sure I'll do the cleanup. You guessed correctly why I did that ;)

In the beginning I did a "return len;" instead of "return 1;" and felt a
bit uncomfortable in case we merged a zero length message.

Then I added the !th->fin test inside tcp_try_coalesce()

(my initial patch allowed the fin being set for the tcp_data_queue()
case since tcp_fin() was called anyway)

Thanks

^ permalink raw reply

* linux-next: manual merge of the wireless-next tree with the net-next tree
From: Stephen Rothwell @ 2012-04-24  2:48 UTC (permalink / raw)
  To: John W. Linville
  Cc: linux-next, linux-kernel, Emmanuel Grumbach, Wey-Yi Guy,
	David Miller, netdev

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

Hi John,

Today's linux-next merge of the wireless-next tree got a conflict in
drivers/net/wireless/iwlwifi/iwl-testmode.c between commit d33e152e1edd
("iwlwifi: Stop using NLA_PUT*()") from the net-next tree and commit
2152268ff911 ("iwlwifi: op_mode holds its pointer to the config") from
the wireless-next tree.

I was hoping that we were done with these ... :-(

I fixed it up (see below) and can carry the fix as necessary.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc drivers/net/wireless/iwlwifi/iwl-testmode.c
index bb27509,a6b16aa..0000000
--- a/drivers/net/wireless/iwlwifi/iwl-testmode.c
+++ b/drivers/net/wireless/iwlwifi/iwl-testmode.c
@@@ -543,12 -539,11 +543,12 @@@ static int iwl_testmode_driver(struct i
  				IWL_ERR(priv, "Memory allocation fail\n");
  				return -ENOMEM;
  			}
 -			NLA_PUT_U32(skb, IWL_TM_ATTR_COMMAND,
 -				IWL_TM_CMD_DEV2APP_EEPROM_RSP);
 -			NLA_PUT(skb, IWL_TM_ATTR_EEPROM,
 -				priv->cfg->base_params->eeprom_size,
 -				priv->eeprom);
 +			if (nla_put_u32(skb, IWL_TM_ATTR_COMMAND,
 +					IWL_TM_CMD_DEV2APP_EEPROM_RSP) ||
 +			    nla_put(skb, IWL_TM_ATTR_EEPROM,
- 				    cfg(priv)->base_params->eeprom_size,
++				    priv->cfg->base_params->eeprom_size,
 +				    priv->eeprom))
 +				goto nla_put_failure;
  			status = cfg80211_testmode_reply(skb);
  			if (status < 0)
  				IWL_ERR(priv, "Error sending msg : %d\n",

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* [PATCH] b44: properly use pr_fmt()
From: Luis R. Rodriguez @ 2012-04-24  2:46 UTC (permalink / raw)
  To: zambrano; +Cc: netdev, Luis R. Rodriguez

From: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>

pr_fmt() is either defined or we redefine it. Typically
drivers define it prior to including printk.h but this
is done under the assumption that no other subsystem
it uses has already defined pr_fmt(). In such cases
pr_fmt() should be undefined and redefined.

Doing this properly shaves down compilation time quite
considerably.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 drivers/net/ethernet/broadcom/b44.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 46b8b7d..656f323 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -10,9 +10,11 @@
  * Distribute under GPL.
  */
 
+#undef pr_fmt
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/kernel.h>
+#include <linux/printk.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/types.h>
-- 
1.7.10.rc1.22.gf5241

^ permalink raw reply related

* Re: [PATCH net-next] tcp: introduce tcp_try_coalesce
From: David Miller @ 2012-04-24  2:46 UTC (permalink / raw)
  To: ncardwell; +Cc: eric.dumazet, netdev, therbert, maze, ilpo.jarvinen
In-Reply-To: <CADVnQy=igJdbcG4J2HU7nEJUNM84qVcLa08h6g6MQQ8-fvgxHg@mail.gmail.com>

From: Neal Cardwell <ncardwell@google.com>
Date: Mon, 23 Apr 2012 22:39:10 -0400

> On Mon, Apr 23, 2012 at 1:11 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> commit c8628155ece3 (tcp: reduce out_of_order memory use) took care of
>> coalescing tcp segments provided by legacy devices (linear skbs)
>>
>> We extend this idea to fragged skbs, as their truesize can be heavy.
>>
>> ixgbe for example uses 256+1024+PAGE_SIZE/2 = 3328 bytes per segment.
>>
>> Use this coalescing strategy for receive queue too.
>>
>> This contributes to reduce number of tcp collapses, at minimal cost, and
>> reduces memory overhead and packets drops.
> 
> Acked-by: Neal Cardwell <ncardwell@google.com>
> 
> Thanks for the background info, Eric.

Applied, thanks Eric.

Although I'd like to ask you to clean up tcp_try_coalesce() a bit.

It effectively returns a boolean, but you've clouded this up by
returning an int and defining it in the comment to return "> 0" or
not.

Just make it return a real bool.

I know why you did this, it makes the "eaten" code somewhat simpler in
tcp_data_queue(), but overall it's more confusing how it is now.

People look at how the tcp_try_coalesce() return value is interpreted
and say "in what cases can it return a negative value?"  We both know
it can't, but you have to read the entire function to figure that out.

And that's by definition not intuitive.

Thanks.

^ permalink raw reply

* Re: [PATCH v2 5/5] decrement static keys on real destroy time
From: KAMEZAWA Hiroyuki @ 2012-04-24  2:40 UTC (permalink / raw)
  To: Glauber Costa
  Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <1335209867-1831-6-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

(2012/04/24 4:37), Glauber Costa wrote:

> We call the destroy function when a cgroup starts to be removed,
> such as by a rmdir event.
> 
> However, because of our reference counters, some objects are still
> inflight. Right now, we are decrementing the static_keys at destroy()
> time, meaning that if we get rid of the last static_key reference,
> some objects will still have charges, but the code to properly
> uncharge them won't be run.
> 
> This becomes a problem specially if it is ever enabled again, because
> now new charges will be added to the staled charges making keeping
> it pretty much impossible.
> 
> We just need to be careful with the static branch activation:
> since there is no particular preferred order of their activation,
> we need to make sure that we only start using it after all
> call sites are active. This is achieved by having a per-memcg
> flag that is only updated after static_key_slow_inc() returns.
> At this time, we are sure all sites are active.
> 
> This is made per-memcg, not global, for a reason:
> it also has the effect of making socket accounting more
> consistent. The first memcg to be limited will trigger static_key()
> activation, therefore, accounting. But all the others will then be
> accounted no matter what. After this patch, only limited memcgs
> will have its sockets accounted.
> 
> [v2: changed a tcp limited flag for a generic proto limited flag ]
> [v3: update the current active flag only after the static_key update ]
> 
> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>


Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

A small request below.

<snip>


> +		 * ->activated needs to be written after the static_key update.
> +		 *  This is what guarantees that the socket activation function
> +		 *  is the last one to run. See sock_update_memcg() for details,
> +		 *  and note that we don't mark any socket as belonging to this
> +		 *  memcg until that flag is up.
> +		 *
> +		 *  We need to do this, because static_keys will span multiple
> +		 *  sites, but we can't control their order. If we mark a socket
> +		 *  as accounted, but the accounting functions are not patched in
> +		 *  yet, we'll lose accounting.
> +		 *
> +		 *  We never race with the readers in sock_update_memcg(), because
> +		 *  when this value change, the code to process it is not patched in
> +		 *  yet.
> +		 */
> +		mutex_lock(&tcp_set_limit_mutex);


Could you explain for what this mutex is in above comment ?

Thanks,
-Kame

> +		if (!cg_proto->activated) {
> +			static_key_slow_inc(&memcg_socket_limit_enabled);
> +			cg_proto->activated = true;
> +		}
> +		mutex_unlock(&tcp_set_limit_mutex);
> +		cg_proto->active = true;
> +	}
>  
>  	return 0;
>  }

^ permalink raw reply

* Re: [PATCH net-next] tcp: introduce tcp_try_coalesce
From: Neal Cardwell @ 2012-04-24  2:39 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski,
	Ilpo Järvinen
In-Reply-To: <1335201102.5205.28.camel@edumazet-glaptop>

On Mon, Apr 23, 2012 at 1:11 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> commit c8628155ece3 (tcp: reduce out_of_order memory use) took care of
> coalescing tcp segments provided by legacy devices (linear skbs)
>
> We extend this idea to fragged skbs, as their truesize can be heavy.
>
> ixgbe for example uses 256+1024+PAGE_SIZE/2 = 3328 bytes per segment.
>
> Use this coalescing strategy for receive queue too.
>
> This contributes to reduce number of tcp collapses, at minimal cost, and
> reduces memory overhead and packets drops.

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks for the background info, Eric.

neal

^ permalink raw reply

* Re: [PATCH net-next 0/3] bnx2x: use FW 7.2.51 and add afex support
From: David Miller @ 2012-04-24  2:34 UTC (permalink / raw)
  To: barak; +Cc: netdev, eilong
In-Reply-To: <1335186273.14229.25.camel@lb-tlvb-barak.il.broadcom.com>

From: "Barak Witkowski" <barak@broadcom.com>
Date: Mon, 23 Apr 2012 16:04:33 +0300

> This patch utilize FW 7.2.51 that was recently added to the linux-firmware tree.
> 
> The main feature of this new FW is support of the AFEX multi-function protocol.
> Supporting the afex protocol requires many changes in the driver, but all of
> them are required together in order to compile and make the feature operational.
> Breaking the afex patch into smaller chunks did not seem to increase readability
> to me.

All applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next 0/3] mlx4_en: Adding BQL support
From: David Miller @ 2012-04-24  2:34 UTC (permalink / raw)
  To: yevgenyp; +Cc: netdev
In-Reply-To: <4F954893.3040401@mellanox.co.il>

From: Yevgeny Petrilin <yevgenyp@mellanox.co.il>
Date: Mon, 23 Apr 2012 15:18:27 +0300

> This patchset adds Byte Queue Limit support to the mlx4_en driver.
> As a precondition to this I had to change the mode of TX completion handling
> Till now we have handled completions by polling from the xmit context,
> once in every 16 packets.
> Now moving to interrupts, and also added the set/get coalescing option for TX interrupts.

All applied to net-next, thanks.

^ permalink raw reply

* Re: [PATCH v2 4/5] don't take cgroup_mutex in destroy()
From: KAMEZAWA Hiroyuki @ 2012-04-24  2:31 UTC (permalink / raw)
  To: Glauber Costa
  Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A, Vivek Goyal
In-Reply-To: <1335209867-1831-5-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

(2012/04/24 4:37), Glauber Costa wrote:

> Most of the destroy functions are only doing very simple things
> like freeing memory.
> 
> The ones who goes through lists and such, already use its own
> locking for those.
> 
> * The cgroup itself won't go away until we free it, (after destroy)
> * The parent won't go away because we hold a reference count
> * There are no more tasks in the cgroup, and the cgroup is declared
>   dead (cgroup_is_removed() == true)
> 
> [v2: don't cgroup_lock the freezer and blkcg ]
> 
> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> CC: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
> CC: Vivek Goyal <vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  kernel/cgroup.c |    9 ++++-----
>  1 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 932c318..976d332 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -869,13 +869,13 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)
>  		 * agent */
>  		synchronize_rcu();
>  
> -		mutex_lock(&cgroup_mutex);
>  		/*
>  		 * Release the subsystem state objects.
>  		 */
>  		for_each_subsys(cgrp->root, ss)
>  			ss->destroy(cgrp);
>  
> +		mutex_lock(&cgroup_mutex);
>  		cgrp->root->number_of_cgroups--;
>  		mutex_unlock(&cgroup_mutex);
>  
> @@ -3994,13 +3994,12 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
>  
>   err_destroy:
>  
> +	mutex_unlock(&cgroup_mutex);
>  	for_each_subsys(root, ss) {
>  		if (cgrp->subsys[ss->subsys_id])
>  			ss->destroy(cgrp);
>  	}
>  
> -	mutex_unlock(&cgroup_mutex);
> -
>  	/* Release the reference count that we took on the superblock */
>  	deactivate_super(sb);
>  
> @@ -4349,9 +4348,9 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>  		int ret = cgroup_init_idr(ss, css);
>  		if (ret) {
>  			dummytop->subsys[ss->subsys_id] = NULL;
> +			mutex_unlock(&cgroup_mutex);
>  			ss->destroy(dummytop);
>  			subsys[i] = NULL;
> -			mutex_unlock(&cgroup_mutex);
>  			return ret;
>  		}
>  	}
> @@ -4447,10 +4446,10 @@ void cgroup_unload_subsys(struct cgroup_subsys *ss)
>  	 * pointer to find their state. note that this also takes care of
>  	 * freeing the css_id.
>  	 */
> +	mutex_unlock(&cgroup_mutex);
>  	ss->destroy(dummytop);
>  	dummytop->subsys[ss->subsys_id] = NULL;
>  

I'm not fully sure but...dummytop->subsys[] update can be done without locking ?

Thanks,
-Kame

^ permalink raw reply

* Re: [PATCH v1 6/8] dmaengine: enhance network subsystem to support DMA device hotplug
From: Jiang Liu @ 2012-04-24  2:30 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jiang Liu, Vinod Koul, Keping Chen, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, linux-pci, linux-kernel
In-Reply-To: <CABE8wwvi_Yz8AyALr1-uDW9xSgrSqfrYWhB_22NQ2ydTa81+mg@mail.gmail.com>

Hi Dan,
	Thanks for your great comments!
	gerry
On 2012-4-24 2:30, Dan Williams wrote:
> On Mon, Apr 23, 2012 at 6:51 AM, Jiang Liu<liuj97@gmail.com>  wrote:
>> Enhance network subsystem to correctly update DMA channel reference counts,
>> so it won't break DMA device hotplug logic.
>>
>> Signed-off-by: Jiang Liu<liuj97@gmail.com>
>
> This introduces an atomic action on every channel touch, which is more
> expensive than what we had previously.  There has always been a
> concern about the overhead of offload that sometimes makes ineffective
> or a loss compared to cpu copies.  In the cases where net_dma shows
> improvement this will eat into / maybe eliminate that advantage.
Good point, we should avoid pollute a shared cacheline here, otherwise
it may eat the benefits of IOAT acceleration.

>
> Take a look at where dmaengine started [1].  It was from the beginning
> going through contortions to avoid something like this.  We made it
> simpler here [2], but still kept the principle of not dirtying a
> shared cacheline on every channel touch, and certainly not locking it.
Thanks for the great background information, especially the second one.
The check-in log message as below.
 >Why?, beyond reducing complication:
 >1/ Tracking reference counts per-transaction in an efficient manner, as
 >   is currently done, requires a complicated scheme to avoid cache-line
 >   bouncing effects.
The really issue here is polluting shared cachelines here, right?
Will it help to use percpu counter instead of atomic operations here?
I will have a try to use percpu counter for reference count.
BTW, do you have any DMAEngine benchmarks so we could use them to
compare the performance difference?

 >2/ Per-transaction ref-counting gives the false impression that a
 >   dma-driver can be gracefully removed ahead of its user (net, md, or
 >   dma-slave)
 >3/ None of the in-tree dma-drivers talk to hot pluggable hardware, but
Seems the situation has changed now:)
Intel 7500 (Boxboro) chipset supports hotplug. And we are working on
a system, which adopts Boxboro chipset and supports node hotplug.
So we try to enhance the DMAEngine to support IOAT hotplug.

On the other hand, Intel next generation processor Ivybridge has
embedded IOH, so we need to support IOH/IOAT hotplug when supporting
processor hotplug.

 >   if such an engine were built one day we still would not need to >notify
 >   clients of remove events.  The driver can simply return NULL to a
 >   ->prep() request, something that is much easier for a client to 
 >handle.
Could you please help to give more explanations about "The driver can
simply return NULL to a ->prep() request", I have gotten the idea yet.

>
> If you are going to hotplug the entire IOH, then you are probably ok
> with network links going down, so could you just down the links and
> remove the driver with the existing code?
I feel it's a little risky to shut down/restart all network interfaces
for hot-removal of IOH, that may disturb the applications. And there
are also other kinds of clients, such as ASYNC_TX, seems we can't
adopt this method to reclaim DMA channels from ASYNC_TX subsystem.

>
> --
> Dan
>
> [1]: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=c13c826
> [2]: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=6f49a57a
>
> .
>

^ permalink raw reply

* Re: [PATCH net-next] tcp: introduce tcp_try_coalesce
From: Eric Dumazet @ 2012-04-24  2:29 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski,
	Ilpo Järvinen
In-Reply-To: <1335233611.5205.90.camel@edumazet-glaptop>

On Tue, 2012-04-24 at 04:13 +0200, Eric Dumazet wrote:

> This never happens on connections where performance matters : skb head
> can only contains one full mss segment.

By the way, top end hardware uses fragged skbs.

Incidentally this gives better splice() performance (avoids copy of skb
head to pages)

^ permalink raw reply

* Re: [PATCH 2/2 net-next] tcp: sk_add_backlog() is too agressive for TCP
From: David Miller @ 2012-04-24  2:27 UTC (permalink / raw)
  To: eric.dumazet
  Cc: rick.jones2, netdev, therbert, ncardwell, maze, ycheng,
	ilpo.jarvinen
In-Reply-To: <1335234012.5205.97.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 24 Apr 2012 04:20:12 +0200

> On Mon, 2012-04-23 at 22:37 +0200, Eric Dumazet wrote:
> 
>> We could try to coalesce ACKs before backlogging them. I'll work on
>> this.
>> 
> 
> I did an experiment, and found a basic coalescing was not working in
> case of packet loss and SACK storm.
> 
> Doing a smart coalescing in this case sounds really complex.
> 
> Should we really continue this way ? 

We can consider it later, but for now let's defer.

I'll apply your sk_add_backlog patches for the time being.

Thanks Eric.

^ permalink raw reply

* Re: [PATCH v2 3/5] change number_of_cpusets to an atomic
From: KAMEZAWA Hiroyuki @ 2012-04-24  2:25 UTC (permalink / raw)
  To: Glauber Costa
  Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <1335209867-1831-4-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

(2012/04/24 4:37), Glauber Costa wrote:

> This will allow us to call destroy() without holding the
> cgroup_mutex(). Other important updates inside update_flags()
> are protected by the callback_mutex.
> 
> We could protect this variable with the callback_mutex as well,
> as suggested by Li Zefan, but we need to make sure we are protected
> by that mutex at all times, and some of its updates happen inside the
> cgroup_mutex - which means we would deadlock.
> 
> An atomic variable is not expensive, since it is seldom updated,
> and protect us well.
> 
> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>


Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Simon Horman @ 2012-04-24  2:25 UTC (permalink / raw)
  To: Jesse Gross
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
	shemminger-ZtmgI6mnKB3QT0dZR+AlfA, David Miller
In-Reply-To: <CAEP_g=9p0TE59JbrS8QzHj4mEzc-5_hUDzmLRsRxLyUaFX+Z5Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Mon, Apr 23, 2012 at 03:59:24PM -0700, Jesse Gross wrote:
> On Mon, Apr 23, 2012 at 3:32 PM, Simon Horman <horms@verge.net.au> wrote:
> > On Mon, Apr 23, 2012 at 02:38:07PM -0700, Jesse Gross wrote:
> >> On Mon, Apr 23, 2012 at 2:08 PM, David Miller <davem@davemloft.net> wrote:
> >> > From: Jesse Gross <jesse@nicira.com>
> >> > Date: Mon, 23 Apr 2012 13:53:42 -0700
> >> >
> >> >> On Mon, Apr 23, 2012 at 1:13 PM, David Miller <davem@davemloft.net> wrote:
> >> >>> From: Jesse Gross <jesse@nicira.com>
> >> >>> Date: Mon, 23 Apr 2012 13:08:49 -0700
> >> >>>
> >> >>>> Assuming that the TCP stack generates large TSO frames on transmit
> >> >>>> (which could be the local stack; something sent by a VM; or packets
> >> >>>> received, coalesced by GRO and then encapsulated by STT) then you can
> >> >>>> just prepend the STT header (possibly slightly adjusting things like
> >> >>>> requested MSS, number of segments, etc. slightly).  After that it's
> >> >>>> possible to just output the resulting frame through the IP stack like
> >> >>>> all tunnels do today.
> >> >>>
> >> >>> Which seems to potentially suggest a stronger intergration of the STT
> >> >>> tunnel transmit path into our IP stack rather than the approach Simon
> >> >>> is taking
> >> >>
> >> >> Did you have something in mind?
> >> >
> >> > A normal bonafide tunnel netdevice driver like GRE instead of the
> >> > openvswitch approach Simon is using.
> >>
> >> Ahh, yes, that I agree with.  Independent of this, there's work being
> >> done to make it so that OVS can use the normal in-tree tunneling code
> >> and not need its own.  Once that's done I expect that STT will follow
> >> the same model.
> >
> > Hi Jesse,
> >
> > I am wondering how firm the plans to on allowing OVS to use in-tree tunnel
> > code are. I'm happy to move my efforts over to an in-tree STT implementation
> > but ultimately I would like to get STT running in conjunction with OVS.
> 
> I would say that it's a firm goal but the implementation probably
> still has a ways to go.  Kyle Mestery (CC'ed) has volunteered to work
> on this in support of adding VXLAN, which needs some additional
> flexibility that this approach would also provide.  You might want to
> talk to him to see if there are ways that you guys can work together
> on it if you are interested.  Having better integration with upstream
> tunneling is definitely a step that OVS needs to make and sooner would
> be better than later.

Hi Jesse, Hi Kyle,

that sounds like an excellent plan.

Kyle, do you have any thoughts on how we might best work together on this?
Perhaps there are some patches floating around that I could take a look at?

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply

* Re: [PATCH v2 1/5] don't attach a task to a dead cgroup
From: KAMEZAWA Hiroyuki @ 2012-04-24  2:20 UTC (permalink / raw)
  To: Glauber Costa
  Cc: Tejun Heo, netdev-u79uwXL29TY76Z2rM5mHXA,
	cgroups-u79uwXL29TY76Z2rM5mHXA, Li Zefan, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A
In-Reply-To: <1335209867-1831-2-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

(2012/04/24 4:37), Glauber Costa wrote:

> Not all external callers of cgroup_attach_task() test to
> see if the cgroup is still live - the internal callers at
> cgroup.c does.
> 
> With this test in cgroup_attach_task, we can assure that
> no tasks are ever moved to a cgroup that is past its
> destruction point and was already marked as dead.
> 
> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> CC: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>


Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>


> ---
>  kernel/cgroup.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index b61b938..932c318 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -1927,6 +1927,9 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>  	struct cgroup_taskset tset = { };
>  	struct css_set *newcg;
>  
> +	if (cgroup_is_removed(cgrp))
> +		return -ENODEV;
> +
>  	/* @tsk either already exited or can't exit until the end */
>  	if (tsk->flags & PF_EXITING)
>  		return -ESRCH;

^ permalink raw reply

* Re: [PATCH 2/2 net-next] tcp: sk_add_backlog() is too agressive for TCP
From: Eric Dumazet @ 2012-04-24  2:20 UTC (permalink / raw)
  To: David Miller
  Cc: rick.jones2, netdev, therbert, ncardwell, maze, ycheng,
	ilpo.jarvinen
In-Reply-To: <1335213446.5205.65.camel@edumazet-glaptop>

On Mon, 2012-04-23 at 22:37 +0200, Eric Dumazet wrote:

> We could try to coalesce ACKs before backlogging them. I'll work on
> this.
> 

I did an experiment, and found a basic coalescing was not working in
case of packet loss and SACK storm.

Doing a smart coalescing in this case sounds really complex.

Should we really continue this way ? 


 include/net/tcp.h   |    1 +
 net/ipv4/tcp_ipv4.c |   32 +++++++++++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index fc880e9..de8d847 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1418,6 +1418,7 @@ static inline unsigned int tcp_stream_is_thin(struct tcp_sock *tp)
 	return tp->packets_out < 4 && !tcp_in_initial_slowstart(tp);
 }
 
+extern bool tcp_ack_coalesce(struct sock *sk, struct sk_buff *skb);
 /* /proc */
 enum tcp_seq_states {
 	TCP_SEQ_STATE_LISTENING,
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 0883921..b5a3bac 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1670,6 +1670,36 @@ csum_err:
 }
 EXPORT_SYMBOL(tcp_v4_do_rcv);
 
+/* socket is owned by user.
+ * Before queuing this skb into backlog, try to coalesce it to previous skb.
+ * We only take care of pure ACKS.
+ */
+bool tcp_ack_coalesce(struct sock *sk, struct sk_buff *skb)
+{
+	struct sk_buff *prev = sk->sk_backlog.tail;
+	const struct tcphdr *th, *thp;
+	unsigned int i, thlen;
+
+	if (TCP_SKB_CB(skb)->seq != TCP_SKB_CB(skb)->end_seq ||
+	    !prev ||
+	    TCP_SKB_CB(skb)->seq != TCP_SKB_CB(prev)->end_seq)
+		return false;
+	th = tcp_hdr(skb);
+	thp = tcp_hdr(prev);
+	thlen = th->doff * 4;
+	i = sizeof(th->source) + sizeof(th->dest) +
+	    sizeof(th->seq) + sizeof(th->ack_seq);
+	for (; i < thlen; i += 4) {
+		if (*(u32 *)((u8 *)th + i) != *(u32 *)((u8 *)thp + i))
+			return false;
+	}
+	if (after(TCP_SKB_CB(skb)->ack_seq, TCP_SKB_CB(prev)->ack_seq))
+		TCP_SKB_CB(prev)->ack_seq = TCP_SKB_CB(skb)->ack_seq;
+	consume_skb(skb);
+	return true;
+}
+EXPORT_SYMBOL(tcp_ack_coalesce);
+
 /*
  *	From tcp_input.c
  */
@@ -1752,7 +1782,7 @@ process:
 			if (!tcp_prequeue(sk, skb))
 				ret = tcp_v4_do_rcv(sk, skb);
 		}
-	} else if (unlikely(sk_add_backlog(sk, skb))) {
+	} else if (!tcp_ack_coalesce(sk, skb) && sk_add_backlog(sk, skb)) {
 		bh_unlock_sock(sk);
 		NET_INC_STATS_BH(net, LINUX_MIB_TCPBACKLOGDROP);
 		goto discard_and_relse;

^ permalink raw reply related

* Re: [PATCH] net ax25: Fix the build when sysctl support is disabled.
From: David Miller @ 2012-04-24  2:15 UTC (permalink / raw)
  To: rdunlap; +Cc: ebiederm, sfr, linux-next, linux-kernel, netdev, linux-hams, ralf
In-Reply-To: <4F9601CD.2070000@xenotime.net>

From: Randy Dunlap <rdunlap@xenotime.net>
Date: Mon, 23 Apr 2012 18:28:45 -0700

> On 04/23/2012 05:25 PM, Eric W. Biederman wrote:
> 
>> 
>> Randy Dunlap <rdunlap@xenotime.net> reported:
>> 
>>> On 04/23/2012 12:07 AM, Stephen Rothwell wrote:
>>>
>>>> Hi all,
>>>>
>>>> Changes since 20120420:
>>>
>>>
>>> include/net/ax25.h:447:75: error: expected ';' before '}' token
>>>
>>> static inline int ax25_register_dev_sysctl(ax25_dev *ax25_dev) { return 0 };
>>> static inline void ax25_unregister_dev_sysctl(ax25_dev *ax25_dev) {};
>>>
>>> First function:  move ';' inside braces.
>>> Second function:  drop the ';'.
>> 
>> Put the semicolons where it makes sense.
>> 
>> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> 
> Acked-by: Randy Dunlap <rdunlap@xenotime.net>

Applied, thanks guys.

^ permalink raw reply

* Re: [PATCH net-next] tcp: introduce tcp_try_coalesce
From: Eric Dumazet @ 2012-04-24  2:13 UTC (permalink / raw)
  To: Neal Cardwell
  Cc: David Miller, netdev, Tom Herbert, Maciej Żenczykowski,
	Ilpo Järvinen
In-Reply-To: <CADVnQyngwym7z_zSi8h2U_1e_wVCwGMPZScywWFWM2AYvxyqjw@mail.gmail.com>

On Mon, 2012-04-23 at 21:13 -0400, Neal Cardwell wrote:
> On Mon, Apr 23, 2012 at 1:11 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > commit c8628155ece3 (tcp: reduce out_of_order memory use) took care of
> > coalescing tcp segments provided by legacy devices (linear skbs)
> >
> > We extend this idea to fragged skbs, as their truesize can be heavy.
> >
> > ixgbe for example uses 256+1024+PAGE_SIZE/2 = 3328 bytes per segment.
> >
> > Use this coalescing strategy for receive queue too.
> >
> > This contributes to reduce number of tcp collapses, at minimal cost, and
> > reduces memory overhead and packets drops.
> 
> The mechanics look solid, but I'm a little concerned about the
> potential added overhead for the new case where tcp_try_coalesce()
> does a skb_copy_bits() for in-order data that it is coalescing at the
> end of the sk_receive_queue. Do you have any performance numbers for
> this case to help suggest whether this added copy is a concern?
> 
> neal

This never happens on connections where performance matters : skb head
can only contains one full mss segment.

This part is only used on wifi devices, where skb head is really fat.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox