Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH v1 6/8] dmaengine: enhance network subsystem to support DMA device hotplug
From: Dan Williams @ 2012-04-23 18:30 UTC (permalink / raw)
  To: Jiang Liu
  Cc: Vinod Koul, Jiang Liu, Keping Chen, David S. Miller,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, netdev, linux-pci, linux-kernel
In-Reply-To: <1335189109-4871-7-git-send-email-jiang.liu@huawei.com>

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.

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.

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?

--
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: [RFC v4] Add TCP encap_rcv hook (repost)
From: David Miller @ 2012-04-23 19:15 UTC (permalink / raw)
  To: horms-/R6kz+dDXgpPR4JQBCEnsQ
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
	jhs-jkUAjuhPggJWk0Htik3J/w, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20120423083007.GB22556-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>

From: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
Date: Mon, 23 Apr 2012 17:30:08 +0900

> I'm pretty sure the patch I posted added encap_rcv to tcp_sock.
> Am I missing the point?

It did, my eyes are failing me :-)

> Currently I am setting up a listening socket. The Open vSwtich tunneling
> code transmits skbs and using either dev_queue_xmit() or ip_local_out().
> I'm not sure that I have exercised the ip_local_out() case yet.

I don't see where on transmit you're going to realize the primary
stated benefit of STT, that being TSO/GSO.

You'll probably want to gather as many packets as possible into a
larger STT frame for this purpose.  And when switching between STT
tunnels, leave the packet alone since a GRO STT frame on receive will
transparently become a STT GSO frame on transmit.

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Stephen Hemminger @ 2012-04-23 19:19 UTC (permalink / raw)
  To: David Miller
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA
In-Reply-To: <20120423.151533.694306336485319759.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

On Mon, 23 Apr 2012 15:15:33 -0400 (EDT)
David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:

> From: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
> Date: Mon, 23 Apr 2012 17:30:08 +0900
> 
> > I'm pretty sure the patch I posted added encap_rcv to tcp_sock.
> > Am I missing the point?
> 
> It did, my eyes are failing me :-)
> 
> > Currently I am setting up a listening socket. The Open vSwtich tunneling
> > code transmits skbs and using either dev_queue_xmit() or ip_local_out().
> > I'm not sure that I have exercised the ip_local_out() case yet.
> 
> I don't see where on transmit you're going to realize the primary
> stated benefit of STT, that being TSO/GSO.
> 
> You'll probably want to gather as many packets as possible into a
> larger STT frame for this purpose.  And when switching between STT
> tunnels, leave the packet alone since a GRO STT frame on receive will
> transparently become a STT GSO frame on transmit.
> 

I think the point of the TSO hack is to get around the MTU problem when tunneling.
The added header of the tunnel eats into the the possible MTU. The use of TSO
in STT is designed to deal with the fact that hardware can't do IP fragmentation
of IP (or UDP).

^ permalink raw reply

* Re: [PATCH 00/28] MISDN patchset for net-next
From: David Miller @ 2012-04-23 19:19 UTC (permalink / raw)
  To: isdn; +Cc: netdev, isdn4linux
In-Reply-To: <20120423170549.33CDA9FB80@pingi6.linux-pingi.de>

From: Karsten Keil <isdn@linux-pingi.de>
Date: Mon, 23 Apr 2012 18:03:51 +0200

> please include thes patches into netnext.

Please submit smaller, reasonable, amounts of patches at one time.
Say 10 or so.

Coming out of the blue with a 28 entry patch bomb is not acceptable.

Also, please do not let whatever you're using to submit these patches
take the email date from the commit timestamp in your local tree.

This makes no sense when your submitting patches for someone to apply,
and utterly screws up the order in which the patches show up in
patchwork:

http://patchwork.ozlabs.org/project/netdev/list/

^ permalink raw reply

* [PATCH v2 0/5] Fix problem with static_key decrement
From: Glauber Costa @ 2012-04-23 19:37 UTC (permalink / raw)
  To: Tejun Heo
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Li Zefan, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A

This is my proposed fix for the sock memcg static_key
problem raised by Kamezawa. In a summary, the problem is
as follows:

We are decrementing the jump label when the socket limit is set back to
unlimited. The problem is that the sockets outlive the memcg, so we can
only do that when the last reference count is dropped. It is worth
mentioning that kmem controller for memcg will have the exact same problem.

If, however, there are no sockets in flight, mem_cgroup_put() during
->destroy() will be the last one, and the decrementing will happen there.

But static_key updates cannot happen with the cgroup_mutex held. This is
because cpusets hold it from within the cpu_hotplug.lock - that static_keys
take through get_online_cpus() in its cpu hotplug handler.

Removing the cgroup_lock() dependency from cpusets is a lot harder, since the
code for generate_sched_domain() rely on that lock to be held, and it interact
with the cgroup core code by quite a bit.

The aim of this series is to make ->destroy() a stable point for jump label
updating, by calling it without the cgroup_mutex held. I believe it to be a
good thing in itself, since it removes a bit the reach of the almighty
cgroup_mutex.

I am ready to make any further modifications on this that you guys
deem necessary.

Thanks


Glauber Costa (5):
  don't attach a task to a dead cgroup
  blkcg: protect blkcg->policy_list
  change number_of_cpusets to an atomic
  don't take cgroup_mutex in destroy()
  decrement static keys on real destroy time

 block/blk-cgroup.c        |    2 +
 include/linux/cpuset.h    |    6 ++--
 include/net/sock.h        |    9 ++++++++
 kernel/cgroup.c           |   12 ++++++----
 kernel/cpuset.c           |   10 ++++----
 mm/memcontrol.c           |   20 ++++++++++++++++-
 net/ipv4/tcp_memcontrol.c |   50 ++++++++++++++++++++++++++++++++++++++------
 7 files changed, 87 insertions(+), 22 deletions(-)

-- 
1.7.7.6

^ permalink raw reply

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

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>
---
 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;
-- 
1.7.7.6

^ permalink raw reply related

* [PATCH v2 2/5] blkcg: protect blkcg->policy_list
From: Glauber Costa @ 2012-04-23 19:37 UTC (permalink / raw)
  To: Tejun Heo
  Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
	Li Zefan, kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A, David Miller,
	devel-GEFAQzZX7r8dnm+yROfE0A, Glauber Costa
In-Reply-To: <1335209867-1831-1-git-send-email-glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

policy_list walks are protected with blkcg->lock everywhere else in the code.
In destroy(), they are not. Because destroy is usually protected with the
cgroup_mutex(), this is usually not a problem. But it would be a lot better
not to assume this.

Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
---
 block/blk-cgroup.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index 126c341..35fd701 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1557,10 +1557,12 @@ static void blkiocg_destroy(struct cgroup *cgroup)
 		spin_unlock(&blkio_list_lock);
 	} while (1);
 
+	spin_lock_irqsave(&blkcg->lock, flags);
 	list_for_each_entry_safe(pn, pntmp, &blkcg->policy_list, node) {
 		blkio_policy_delete_node(pn);
 		kfree(pn);
 	}
+	spin_unlock_irqrestore(&blkcg->lock, flags);
 
 	free_css_id(&blkio_subsys, &blkcg->css);
 	rcu_read_unlock();
-- 
1.7.7.6

^ permalink raw reply related

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

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>
---
 include/linux/cpuset.h |    6 +++---
 kernel/cpuset.c        |   10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/cpuset.h b/include/linux/cpuset.h
index 668f66b..9b3d468 100644
--- a/include/linux/cpuset.h
+++ b/include/linux/cpuset.h
@@ -16,7 +16,7 @@
 
 #ifdef CONFIG_CPUSETS
 
-extern int number_of_cpusets;	/* How many cpusets are defined in system? */
+extern atomic_t number_of_cpusets;	/* How many cpusets are defined in system? */
 
 extern int cpuset_init(void);
 extern void cpuset_init_smp(void);
@@ -33,13 +33,13 @@ extern int __cpuset_node_allowed_hardwall(int node, gfp_t gfp_mask);
 
 static inline int cpuset_node_allowed_softwall(int node, gfp_t gfp_mask)
 {
-	return number_of_cpusets <= 1 ||
+	return atomic_read(&number_of_cpusets) <= 1 ||
 		__cpuset_node_allowed_softwall(node, gfp_mask);
 }
 
 static inline int cpuset_node_allowed_hardwall(int node, gfp_t gfp_mask)
 {
-	return number_of_cpusets <= 1 ||
+	return atomic_read(&number_of_cpusets) <= 1 ||
 		__cpuset_node_allowed_hardwall(node, gfp_mask);
 }
 
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 8c8bd65..65bfd6d 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -73,7 +73,7 @@ static struct workqueue_struct *cpuset_wq;
  * When there is only one cpuset (the root cpuset) we can
  * short circuit some hooks.
  */
-int number_of_cpusets __read_mostly;
+atomic_t number_of_cpusets __read_mostly;
 
 /* Forward declare cgroup structures */
 struct cgroup_subsys cpuset_subsys;
@@ -583,7 +583,7 @@ static int generate_sched_domains(cpumask_var_t **domains,
 		goto done;
 	}
 
-	csa = kmalloc(number_of_cpusets * sizeof(cp), GFP_KERNEL);
+	csa = kmalloc(atomic_read(&number_of_cpusets) * sizeof(cp), GFP_KERNEL);
 	if (!csa)
 		goto done;
 	csn = 0;
@@ -1848,7 +1848,7 @@ static struct cgroup_subsys_state *cpuset_create(struct cgroup *cont)
 	cs->relax_domain_level = -1;
 
 	cs->parent = parent;
-	number_of_cpusets++;
+	atomic_inc(&number_of_cpusets);
 	return &cs->css ;
 }
 
@@ -1865,7 +1865,7 @@ static void cpuset_destroy(struct cgroup *cont)
 	if (is_sched_load_balance(cs))
 		update_flag(CS_SCHED_LOAD_BALANCE, cs, 0);
 
-	number_of_cpusets--;
+	atomic_dec(&number_of_cpusets);
 	free_cpumask_var(cs->cpus_allowed);
 	kfree(cs);
 }
@@ -1909,7 +1909,7 @@ int __init cpuset_init(void)
 	if (!alloc_cpumask_var(&cpus_attach, GFP_KERNEL))
 		BUG();
 
-	number_of_cpusets = 1;
+	atomic_set(&number_of_cpusets, 1);
 	return 0;
 }
 
-- 
1.7.7.6

^ permalink raw reply related

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

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;
 
-	mutex_unlock(&cgroup_mutex);
 }
 EXPORT_SYMBOL_GPL(cgroup_unload_subsys);
 
-- 
1.7.7.6

^ permalink raw reply related

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

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>
---
 include/net/sock.h        |    9 ++++++++
 mm/memcontrol.c           |   20 ++++++++++++++++-
 net/ipv4/tcp_memcontrol.c |   50 ++++++++++++++++++++++++++++++++++++++------
 3 files changed, 70 insertions(+), 9 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index b3ebe6b..c5a2010 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -914,6 +914,15 @@ struct cg_proto {
 	int			*memory_pressure;
 	long			*sysctl_mem;
 	/*
+	 * active means it is currently active, and new sockets should
+	 * be assigned to cgroups.
+	 *
+	 * activated means it was ever activated, and we need to
+	 * disarm the static keys on destruction
+	 */
+	bool			activated;
+	bool			active; 
+	/*
 	 * memcg field is used to find which memcg we belong directly
 	 * Each memcg struct can hold more than one cg_proto, so container_of
 	 * won't really cut.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 7832b4d..01d25a0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -404,6 +404,7 @@ void sock_update_memcg(struct sock *sk)
 {
 	if (mem_cgroup_sockets_enabled) {
 		struct mem_cgroup *memcg;
+		struct cg_proto *cg_proto;
 
 		BUG_ON(!sk->sk_prot->proto_cgroup);
 
@@ -423,9 +424,10 @@ void sock_update_memcg(struct sock *sk)
 
 		rcu_read_lock();
 		memcg = mem_cgroup_from_task(current);
-		if (!mem_cgroup_is_root(memcg)) {
+		cg_proto = sk->sk_prot->proto_cgroup(memcg);
+		if (!mem_cgroup_is_root(memcg) && cg_proto->active) {
 			mem_cgroup_get(memcg);
-			sk->sk_cgrp = sk->sk_prot->proto_cgroup(memcg);
+			sk->sk_cgrp = cg_proto;
 		}
 		rcu_read_unlock();
 	}
@@ -442,6 +444,14 @@ void sock_release_memcg(struct sock *sk)
 	}
 }
 
+static void disarm_static_keys(struct mem_cgroup *memcg)
+{
+#ifdef CONFIG_INET
+	if (memcg->tcp_mem.cg_proto.activated)
+		static_key_slow_dec(&memcg_socket_limit_enabled);
+#endif
+}
+
 #ifdef CONFIG_INET
 struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
 {
@@ -452,6 +462,11 @@ struct cg_proto *tcp_proto_cgroup(struct mem_cgroup *memcg)
 }
 EXPORT_SYMBOL(tcp_proto_cgroup);
 #endif /* CONFIG_INET */
+#else
+static inline void disarm_static_keys(struct mem_cgroup *memcg)
+{
+}
+
 #endif /* CONFIG_CGROUP_MEM_RES_CTLR_KMEM */
 
 static void drain_all_stock_async(struct mem_cgroup *memcg);
@@ -4883,6 +4898,7 @@ static void __mem_cgroup_put(struct mem_cgroup *memcg, int count)
 {
 	if (atomic_sub_and_test(count, &memcg->refcnt)) {
 		struct mem_cgroup *parent = parent_mem_cgroup(memcg);
+		disarm_static_keys(memcg);
 		__mem_cgroup_free(memcg);
 		if (parent)
 			mem_cgroup_put(parent);
diff --git a/net/ipv4/tcp_memcontrol.c b/net/ipv4/tcp_memcontrol.c
index 1517037..e9c2710 100644
--- a/net/ipv4/tcp_memcontrol.c
+++ b/net/ipv4/tcp_memcontrol.c
@@ -54,6 +54,8 @@ int tcp_init_cgroup(struct mem_cgroup *memcg, struct cgroup_subsys *ss)
 	cg_proto->sysctl_mem = tcp->tcp_prot_mem;
 	cg_proto->memory_allocated = &tcp->tcp_memory_allocated;
 	cg_proto->sockets_allocated = &tcp->tcp_sockets_allocated;
+	cg_proto->active = false;
+	cg_proto->activated = false;
 	cg_proto->memcg = memcg;
 
 	return 0;
@@ -74,12 +76,23 @@ void tcp_destroy_cgroup(struct mem_cgroup *memcg)
 	percpu_counter_destroy(&tcp->tcp_sockets_allocated);
 
 	val = res_counter_read_u64(&tcp->tcp_memory_allocated, RES_LIMIT);
-
-	if (val != RESOURCE_MAX)
-		static_key_slow_dec(&memcg_socket_limit_enabled);
 }
 EXPORT_SYMBOL(tcp_destroy_cgroup);
 
+/*
+ * This is to prevent two writes arriving at the same time
+ * at kmem.tcp.limit_in_bytes.
+ *
+ * There is a race at the first time we write to this file:
+ *
+ * - cg_proto->activated == false for all writers.
+ * - They all do a static_key_slow_inc().
+ * - When we are finally read to decrement the static_keys,
+ *   we'll do it only once per activated cgroup. So we won't
+ *   be able to disable it.
+ */
+static DEFINE_MUTEX(tcp_set_limit_mutex);
+
 static int tcp_update_limit(struct mem_cgroup *memcg, u64 val)
 {
 	struct net *net = current->nsproxy->net_ns;
@@ -107,10 +120,33 @@ static int tcp_update_limit(struct mem_cgroup *memcg, u64 val)
 		tcp->tcp_prot_mem[i] = min_t(long, val >> PAGE_SHIFT,
 					     net->ipv4.sysctl_tcp_mem[i]);
 
-	if (val == RESOURCE_MAX && old_lim != RESOURCE_MAX)
-		static_key_slow_dec(&memcg_socket_limit_enabled);
-	else if (old_lim == RESOURCE_MAX && val != RESOURCE_MAX)
-		static_key_slow_inc(&memcg_socket_limit_enabled);
+	if (val == RESOURCE_MAX)
+		cg_proto->active = false;
+	else if (val != RESOURCE_MAX) {
+		/*
+		 * ->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);
+		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;
 }
-- 
1.7.7.6

^ permalink raw reply related

* Re: [PATCH v2] smsc95xx: mark link down on startup and let PHY interrupt deal with carrier changes
From: Stephane Fillod @ 2012-04-23 19:43 UTC (permalink / raw)
  To: Paolo Pisati; +Cc: Steve Glendinning, netdev
In-Reply-To: <1335189920-31062-1-git-send-email-paolo.pisati@canonical.com>

lun, Apr 23, 2012, Paolo Pisati skribis:
> Without this patch sysfs reports the cable as present
> 
> flag@flag-desktop:~$ cat /sys/class/net/eth0/carrier
> 1
> 
> while it's not:
> 
> flag@flag-desktop:~$ sudo mii-tool eth0
> eth0: no link
> 
> Tested on my Beagle XM.
> 
> v2: added mantainer to the list of recipient

His e-mail address has changed, but somehow it has not been updated yet
in MAINTAINERS directory:  steve.glendinning () shawell.net

> Signed-off-by: Paolo Pisati <paolo.pisati@canonical.com>
> ---
>  drivers/net/usb/smsc95xx.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 3444514..93d3252 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -1192,7 +1192,7 @@ static const struct driver_info smsc95xx_info = {
>  	.rx_fixup	= smsc95xx_rx_fixup,
>  	.tx_fixup	= smsc95xx_tx_fixup,
>  	.status		= smsc95xx_status,
> -	.flags		= FLAG_ETHER | FLAG_SEND_ZLP,
> +	.flags		= FLAG_ETHER | FLAG_SEND_ZLP | FLAG_LINK_INTR,
>  };
>  
>  static const struct usb_device_id products[] = {

Tested this fix successfuly on elder brother smsc75xx, on top of
another patch yet-to-submit to fix smsc75xx's MDIO access.

^ permalink raw reply

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

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

> On Mon, 2012-04-23 at 10:14 -0700, Rick Jones wrote:
> 
>> 
>> This will increase what can be queued for arriving segments in general 
>> and not for ACKs specifically yes?  (A possible issue that would have 
>> come-up with my previous wondering about just increasing SO_RCVBUF as 
>> SO_SNDBUF was increasing).  Perhaps only add sk->sk_sndbuf to the limit 
>> if the arriving segment contains no data?
> 
> Thats the backlog limit that we tweak here.
> 
> Its not a big deal if we allow a bit more packets to come and later drop
> them if we hit the real rcvbuf limit. (ACKS wont consume space, since
> they are freed as soon as processed)

Hmmm... why don't we just acknowledge reality and special case ACKs?

If a TCP packet is dataless we should just let it go through no matter
what and with no limits.  It is by definition transient and will not
get queued up into the socket past this backlog stage.

This proposed patch allows non-dataless packets to eat more space in
the backlog, thus the concern and slight pushback.  And from another
perspective, having the stack process data packets which will just
get dropped when we try to attach it to the receive queue is just
wasted work.

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Jesse Gross @ 2012-04-23 20:08 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA, David Miller
In-Reply-To: <20120423121934.195e898c-We1ePj4FEcvRI77zikRAJc56i+j3xesD0e7PPNI6Mm0@public.gmane.org>

On Mon, Apr 23, 2012 at 12:19 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> On Mon, 23 Apr 2012 15:15:33 -0400 (EDT)
> David Miller <davem@davemloft.net> wrote:
>
>> From: Simon Horman <horms@verge.net.au>
>> Date: Mon, 23 Apr 2012 17:30:08 +0900
>>
>> > I'm pretty sure the patch I posted added encap_rcv to tcp_sock.
>> > Am I missing the point?
>>
>> It did, my eyes are failing me :-)
>>
>> > Currently I am setting up a listening socket. The Open vSwtich tunneling
>> > code transmits skbs and using either dev_queue_xmit() or ip_local_out().
>> > I'm not sure that I have exercised the ip_local_out() case yet.
>>
>> I don't see where on transmit you're going to realize the primary
>> stated benefit of STT, that being TSO/GSO.
>>
>> You'll probably want to gather as many packets as possible into a
>> larger STT frame for this purpose.  And when switching between STT
>> tunnels, leave the packet alone since a GRO STT frame on receive will
>> transparently become a STT GSO frame on transmit.
>>
>
> I think the point of the TSO hack is to get around the MTU problem when tunneling.
> The added header of the tunnel eats into the the possible MTU. The use of TSO
> in STT is designed to deal with the fact that hardware can't do IP fragmentation
> of IP (or UDP).

That is a beneficial side effect, although the main goal is to just to
get back all of the offloads that are lost because hardware can't see
inside of encapsulated packets, with TSO, LRO, and RSS being the main
examples.

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.  Similarly, on the other side the NIC will be
able to perform its normal offloading operations as well.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: David Miller @ 2012-04-23 20:13 UTC (permalink / raw)
  To: jesse-l0M0P4e3n4LQT0dZR+AlfA
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
	shemminger-ZtmgI6mnKB3QT0dZR+AlfA
In-Reply-To: <CAEP_g=_3om5aR=P0ffa9421KhvYYrMEeE33TNcCC9UV6+XVWAQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
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

^ permalink raw reply

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

On Mon, 2012-04-23 at 16:01 -0400, David Miller wrote:

> Hmmm... why don't we just acknowledge reality and special case ACKs?
> 

Yes why not.


> If a TCP packet is dataless we should just let it go through no matter
> what and with no limits.  It is by definition transient and will not
> get queued up into the socket past this backlog stage.
> 

Even being transient we need a limit. Without copybreak, an ACK can cost
2048+256 bytes.

In my 10Gbit tests (standard netperf using 16K buffers), I've seen
backlogs of 300 ACK packets...

> This proposed patch allows non-dataless packets to eat more space in
> the backlog, thus the concern and slight pushback.  And from another
> perspective, having the stack process data packets which will just
> get dropped when we try to attach it to the receive queue is just
> wasted work.

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

Thanks

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: Jesse Gross @ 2012-04-23 20:53 UTC (permalink / raw)
  To: David Miller
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
	shemminger-ZtmgI6mnKB3QT0dZR+AlfA
In-Reply-To: <20120423.161313.1582195533832554777.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>

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?  Since the originating stack already
generates TSO frames today, it's just a few lines of code to adjust
for the addition of the STT header as the skb is encapsulated.
Otherwise, the transmit path is the same as something like GRE.  L2TP
follows a fairly similar path - on receive it binds to a listening UDP
socket and on transmit it prepends a header, setups up checksum
offloading, and outputs directly via ip_queue_xmit().
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

^ permalink raw reply

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

On 04/23/2012 01:37 PM, Eric Dumazet wrote:
> In my 10Gbit tests (standard netperf using 16K buffers), I've seen
> backlogs of 300 ACK packets...

Probably better to call that something other than 16K buffers - the send 
size was probably 16K, which reflected SO_SNDBUF at the time the data 
socket was created, but clearly SO_SNDBUF grew in that timeframe.

And those values are "standard" for netperf only in the context of 
(default) Linux - on other platforms the defaults in the stack and so in 
netperf are probably different.

The classic/migrated classic tests report only the initial socket buffer 
sizes, not what they become by the end of the test:

raj@tardy:~/netperf2_trunk/src$ ./netperf -H 192.168.1.3
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 
192.168.1.3 () port 0 AF_INET
Recv   Send    Send
Socket Socket  Message  Elapsed
Size   Size    Size     Time     Throughput
bytes  bytes   bytes    secs.    10^6bits/sec

  87380  16384  16384    10.00     941.06

To see what they are at the end of the test requires more direct use of 
the omni path.  Either by way of test type:

raj@tardy:~/netperf2_trunk/src$ ./netperf -H 192.168.1.3 -t omni
OMNI Send TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.1.3 () 
port 0 AF_INET
Local       Remote      Local  Elapsed Throughput Throughput
Send Socket Recv Socket Send   Time               Units
Size        Size        Size   (sec)
Final       Final
266640      87380       16384  10.00   940.92     10^6bits/s

or omni output selection:

raj@tardy:~/netperf2_trunk/src$ ./netperf -H 192.168.1.3 -- -k 
lss_size_req,lss_size,lss_size_end,rsr_size_req,rsr_size,rsr_size_end
MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 
192.168.1.3 () port 0 AF_INET
LSS_SIZE_REQ=-1
LSS_SIZE=16384
LSS_SIZE_END=266640
RSR_SIZE_REQ=-1
RSR_SIZE=87380
RSR_SIZE_END=87380

BTW, does it make sense that the SO_SNDBUF size on the netperf side 
(lss_size_end - 2.6.38-14-generic kernel) grew larger than the SO_RCVBUF 
on the netserver side? (3.2.0-rc4+)

rick jones

PS - here is data flowing the other way:
raj@tardy:~/netperf2_trunk/src$ ./netperf -H 192.168.1.3 -t TCP_MAERTS 
-- -k lsr_size_req,lsr_size,lsr_size_end,rss_size_req,rss_size,rss_size_end
MIGRATED TCP MAERTS TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 
192.168.1.3 () port 0 AF_INET
LSR_SIZE_REQ=-1
LSR_SIZE=87380
LSR_SIZE_END=4194304
RSS_SIZE_REQ=-1
RSS_SIZE=16384
RSS_SIZE_END=65536

^ permalink raw reply

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

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

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

Great idea, although I wonder about the effect this could have on RTT
measurements.  Instead of having N RTT measurements, we'd have just
one.

Granted, what happens right now wrt. RTT measurements with such huge
ACK backlogs isn't all that nice either.

Ideally, perhaps, we'd do a timestamp diff at the time we insert the
packet into the backlog.  That way we wouldn't gain the RTT inaccuracy
introduced by such queueing delays and ACK backlogs.

Another way to look at it is that the coalesced scheme would actually
improve RTT measurements, since the most accurate (and least
"delayed") of the timestamps would be the only one processed :-)

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: David Miller @ 2012-04-23 21:08 UTC (permalink / raw)
  To: jesse-l0M0P4e3n4LQT0dZR+AlfA
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
	netdev-u79uwXL29TY76Z2rM5mHXA, jhs-jkUAjuhPggJWk0Htik3J/w,
	stephen.hemminger-ZtmgI6mnKB3QT0dZR+AlfA,
	shemminger-ZtmgI6mnKB3QT0dZR+AlfA
In-Reply-To: <CAEP_g=8EVOVgDaWnu3sd+qHxNZ7+ogjzBkuWvfVHNAqX2DRf=g@mail.gmail.com>

From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Date: Mon, 23 Apr 2012 13:53:42 -0700

> On Mon, Apr 23, 2012 at 1:13 PM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>> From: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
>> 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.

^ permalink raw reply

* Re: [PATCH net-next] be2net: fix ethtool get settings
From: David Miller @ 2012-04-23 21:24 UTC (permalink / raw)
  To: ajit.khaparde, ajitkhaparde; +Cc: netdev
In-Reply-To: <20120422045322.GA31559@akhaparde-VBox>

From: Ajit Khaparde <ajitkhaparde@gmail.com>
Date: Sat, 21 Apr 2012 23:53:22 -0500

> ethtool get settings was not displaying all the settings correctly.
> use the get_phy_info to get more information about the PHY to fix this.
> 
> Signed-off-by: Ajit Khaparde <ajit.khaparde@emulex.com>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next 1/3] Add capability to retrieve plug-in module EEPROM
From: David Miller @ 2012-04-23 21:28 UTC (permalink / raw)
  To: smhodgson
  Cc: netdev, bhutchings, bruce.w.allan, decot, alexander.h.duyck,
	linux-kernel
In-Reply-To: <4F9031BF.10106@solarflare.com>


You can't just submit three seperate patches each with the same exact
Subject line.

Otherwise someone scanning the commit headers can't figure out what
is different in each of these changes.

There also is no signoff from Ben for patches #2 or #3, did he review
them?  If so, why didn't he ACK or sign off on it?  If not, why not?

^ permalink raw reply

* [PATCH RFC 0/2] e1000e: 82574 also needs ASPM L1 completely disabled
From: Chris Boot @ 2012-04-23 21:29 UTC (permalink / raw)
  To: e1000-devel, netdev; +Cc: linux-kernel, nix, carolyn.wyborny, Chris Boot
In-Reply-To: <9BBC4E0CF881AA4299206E2E1412B6260E512E0C@ORSMSX102.amr.corp.intel.com>

After much toing and froing on LKML, netdev and the e1000 mailing lists
over the past few months we've determined that the 82574L needs to have
both ASPM L0s and L1 disabled or else it's likely to lock up. This
little series does just that, also cleaning up some now-unnecessary code
that disables L1 on the 82573 and 82574 if the MTU is greater than 1500
bytes.

Please note I haven't as-yet tested this code at all, but I do know that
disabling ASPM L1 on these NICs (using setpci) fixes the hangs that I
have been seeing on my Supermicro servers with X9SCL-F boards. I hope to
get the chance to install an updated kernel on my two afftected servers
later this week.

Chris Boot (2):
  e1000e: Disable ASPM L1 on 82574
  e1000e: Remove special case for 82573/82574 ASPM L1 disablement

 drivers/net/ethernet/intel/e1000e/82571.c  |    3 ++-
 drivers/net/ethernet/intel/e1000e/netdev.c |    8 --------
 2 files changed, 2 insertions(+), 9 deletions(-)

-- 
1.7.10

^ permalink raw reply

* [PATCH 1/2] e1000e: Disable ASPM L1 on 82574
From: Chris Boot @ 2012-04-23 21:29 UTC (permalink / raw)
  To: e1000-devel, netdev; +Cc: linux-kernel, nix, carolyn.wyborny, Chris Boot
In-Reply-To: <1335216578-21542-1-git-send-email-bootc@bootc.net>

ASPM on the 82574 causes trouble. Currently the driver disables L0s for
this NIC but only disables L1 if the MTU is >1500. This patch simply
causes L1 to be disabled regardless of the MTU setting.

Signed-off-by: Chris Boot <bootc@bootc.net>
Cc: "Wyborny, Carolyn" <carolyn.wyborny@intel.com>
Cc: Nix <nix@esperi.org.uk>
Link: https://lkml.org/lkml/2012/3/19/362
---
 drivers/net/ethernet/intel/e1000e/82571.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/82571.c b/drivers/net/ethernet/intel/e1000e/82571.c
index b3fdc69..c6d95f2 100644
--- a/drivers/net/ethernet/intel/e1000e/82571.c
+++ b/drivers/net/ethernet/intel/e1000e/82571.c
@@ -2061,8 +2061,9 @@ const struct e1000_info e1000_82574_info = {
 				  | FLAG_HAS_SMART_POWER_DOWN
 				  | FLAG_HAS_AMT
 				  | FLAG_HAS_CTRLEXT_ON_LOAD,
-	.flags2			  = FLAG2_CHECK_PHY_HANG
+	.flags2			= FLAG2_CHECK_PHY_HANG
 				  | FLAG2_DISABLE_ASPM_L0S
+				  | FLAG2_DISABLE_ASPM_L1
 				  | FLAG2_NO_DISABLE_RX,
 	.pba			= 32,
 	.max_hw_frame_size	= DEFAULT_JUMBO,
-- 
1.7.10

^ permalink raw reply related

* [PATCH 2/2] e1000e: Remove special case for 82573/82574 ASPM L1 disablement
From: Chris Boot @ 2012-04-23 21:29 UTC (permalink / raw)
  To: e1000-devel, netdev; +Cc: nix, Chris Boot, linux-kernel
In-Reply-To: <1335216578-21542-1-git-send-email-bootc@bootc.net>

For the 82573, ASPM L1 gets disabled wholesale so this special-case code
is not required. For the 82574 the previous patch does the same as for
the 82573, disabling L1 on the adapter. Thus, this code is no longer
required and can be removed.

Signed-off-by: Chris Boot <bootc@bootc.net>
---
 drivers/net/ethernet/intel/e1000e/netdev.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 19ab215..ea96cfd 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5293,14 +5293,6 @@ static int e1000_change_mtu(struct net_device *netdev, int new_mtu)
 		return -EINVAL;
 	}
 
-	/* 82573 Errata 17 */
-	if (((adapter->hw.mac.type == e1000_82573) ||
-	     (adapter->hw.mac.type == e1000_82574)) &&
-	    (max_frame > ETH_FRAME_LEN + ETH_FCS_LEN)) {
-		adapter->flags2 |= FLAG2_DISABLE_ASPM_L1;
-		e1000e_disable_aspm(adapter->pdev, PCIE_LINK_STATE_L1);
-	}
-
 	while (test_and_set_bit(__E1000_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
 	/* e1000e_down -> e1000e_reset dependent on max_frame_size & mtu */
-- 
1.7.10


------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply related

* Re: [PATCH 2/2 net-next] tcp: sk_add_backlog() is too agressive for TCP
From: Eric Dumazet @ 2012-04-23 21:30 UTC (permalink / raw)
  To: Rick Jones
  Cc: David Miller, netdev, therbert, ncardwell, maze, ycheng,
	ilpo.jarvinen
In-Reply-To: <4F95C22D.3010908@hp.com>

On Mon, 2012-04-23 at 13:57 -0700, Rick Jones wrote:
> On 04/23/2012 01:37 PM, Eric Dumazet wrote:
> > In my 10Gbit tests (standard netperf using 16K buffers), I've seen
> > backlogs of 300 ACK packets...
> 
> Probably better to call that something other than 16K buffers - the send 
> size was probably 16K, which reflected SO_SNDBUF at the time the data 
> socket was created, but clearly SO_SNDBUF grew in that timeframe.
> 


Maybe I was not clear : Application does sendmsg() of 16KB buffers.

Yet, in the small time it takes to perform this operation, softirq can
queue up to 300 packets coming from the other side.

^ 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