| Message ID | 201006302159.o5ULx8ow025348@d01av03.pok.ibm.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Brian King <brking@linux.vnet.ibm.com> Date: Wed, 30 Jun 2010 16:59:12 -0500 > > Since ehea_get_stats calls ehea_h_query_ehea_port, which > can sleep, we can also sleep when allocating a page in > this function. This fixes some memory allocation failure > warnings seen under low memory conditions. > > Signed-off-by: Brian King <brking@linux.vnet.ibm.com> Applied to net-next-2.6 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le jeudi 01 juillet 2010 à 22:48 -0700, David Miller a écrit : > From: Brian King <brking@linux.vnet.ibm.com> > Date: Wed, 30 Jun 2010 16:59:12 -0500 > > > > > Since ehea_get_stats calls ehea_h_query_ehea_port, which > > can sleep, we can also sleep when allocating a page in > > this function. This fixes some memory allocation failure > > warnings seen under low memory conditions. > > > > Signed-off-by: Brian King <brking@linux.vnet.ibm.com> > > Applied to net-next-2.6 > -- I believe there is a problem with this patch and/or bonding. If we say ndo_get_stats() methods are allowed to sleep, then bond_get_stats() should be updated, because it currently calls dev_get_stats() from a read_lock_bh(&bond->lock); section. Are we allowed to sleep inside a read_lock_bh() ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Dumazet <eric.dumazet@gmail.com> wrote: >Le jeudi 01 juillet 2010 à 22:48 -0700, David Miller a écrit : >> From: Brian King <brking@linux.vnet.ibm.com> >> Date: Wed, 30 Jun 2010 16:59:12 -0500 >> >> > >> > Since ehea_get_stats calls ehea_h_query_ehea_port, which >> > can sleep, we can also sleep when allocating a page in >> > this function. This fixes some memory allocation failure >> > warnings seen under low memory conditions. >> > >> > Signed-off-by: Brian King <brking@linux.vnet.ibm.com> >> >> Applied to net-next-2.6 >> -- > >I believe there is a problem with this patch and/or bonding. > >If we say ndo_get_stats() methods are allowed to sleep, then >bond_get_stats() should be updated, because it currently calls >dev_get_stats() from a read_lock_bh(&bond->lock); section. > >Are we allowed to sleep inside a read_lock_bh() ? Nope. And bonding's not the only call site that holds a lock over the call to ndo_get_stats / dev_get_stats; net/core/net-sysfs.c:netstat_show does it as well. I presume that bonding and netstat_show are holding a lock to keep a list of interfaces from changing, since there's no other locking that's guaranteed to be held for a call to dev_get_stats. In any event, ehea is doing an hcall to the hypervisor, which may return "long busy," after which ehea sleeps for however long the hypervisor told it to wait before trying again. So, the real question is whether the ndo_get_stats* functions are permitted to sleep. If they are, then bonding and netstat_show both need to change. If not, then ehea needs to change. Ehea is probably not alone in this; I poked around a bit, and it looks like mlx4 may also sleep in ndo_get_stats. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jay Vosburgh <fubar@us.ibm.com> Date: Wed, 18 Aug 2010 10:49:41 -0700 > So, the real question is whether the ndo_get_stats* functions > are permitted to sleep. If they are, then bonding and netstat_show both > need to change. If not, then ehea needs to change. Ehea is probably > not alone in this; I poked around a bit, and it looks like mlx4 may also > sleep in ndo_get_stats. They really can't currently.... so EHEA will need to be changed such that it can do stats fetching atomically. In the long term, we could allow this. The netstat_stat function should be RCU'able. And something similar, I suppose, should be possible on the bonding side. But as it stands EHEA has to be fixed and can't stay as-is. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -puN drivers/net/ehea/ehea_main.c~ehea_get_stats_gfp drivers/net/ehea/ehea_main.c --- linux-2.6/drivers/net/ehea/ehea_main.c~ehea_get_stats_gfp 2010-06-28 09:46:51.000000000 -0500 +++ linux-2.6-bjking1/drivers/net/ehea/ehea_main.c 2010-06-28 09:46:51.000000000 -0500 @@ -335,7 +335,7 @@ static struct net_device_stats *ehea_get memset(stats, 0, sizeof(*stats)); - cb2 = (void *)get_zeroed_page(GFP_ATOMIC); + cb2 = (void *)get_zeroed_page(GFP_KERNEL); if (!cb2) { ehea_error("no mem for cb2"); goto out;
Since ehea_get_stats calls ehea_h_query_ehea_port, which can sleep, we can also sleep when allocating a page in this function. This fixes some memory allocation failure warnings seen under low memory conditions. Signed-off-by: Brian King <brking@linux.vnet.ibm.com> --- drivers/net/ehea/ehea_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)