* Re: [PATCH 08/16] ipvs: fix ip_vs_set_timeout debug messages
From: Arnd Bergmann @ 2012-10-06 6:45 UTC (permalink / raw)
To: Julian Anastasov
Cc: linux-arm-kernel, linux-kernel, arm, David S. Miller, netdev,
Simon Horman, netfilter-devel, netfilter, coreteam
In-Reply-To: <alpine.LFD.2.00.1210052305190.1639@ja.ssi.bg>
On Friday 05 October 2012, Julian Anastasov wrote:
>
> Hello,
>
> On Fri, 5 Oct 2012, Arnd Bergmann wrote:
>
> > The ip_vs_set_timeout function sets timeouts for TCP and UDP, which
> > can be enabled independently at compile time. The debug message
> > always prints both timeouts that are passed into the function,
> > but if one is disabled, the message will show uninitialized data.
> >
> > This splits the debug message into two separte IP_VS_DBG statements
> > that are in the same #ifdef section to ensure we only print the
> > text about what is actually going on.
> >
> > Without this patch, building ARM ixp4xx_defconfig results in:
>
> Are there any CONFIG_IP_VS_PROTO_xxx options in this
> default config? It is a waste of memory if IPVS is compiled
> without any protocols.
They all appear to be turned off:
$ grep CONFIG_IP_VS obj-tmp/.config
CONFIG_IP_VS=m
CONFIG_IP_VS_DEBUG=y
CONFIG_IP_VS_TAB_BITS=12
# CONFIG_IP_VS_PROTO_TCP is not set
# CONFIG_IP_VS_PROTO_UDP is not set
# CONFIG_IP_VS_PROTO_AH_ESP is not set
# CONFIG_IP_VS_PROTO_ESP is not set
# CONFIG_IP_VS_PROTO_AH is not set
# CONFIG_IP_VS_PROTO_SCTP is not set
CONFIG_IP_VS_RR=m
CONFIG_IP_VS_WRR=m
CONFIG_IP_VS_LC=m
CONFIG_IP_VS_WLC=m
CONFIG_IP_VS_LBLC=m
CONFIG_IP_VS_LBLCR=m
CONFIG_IP_VS_DH=m
CONFIG_IP_VS_SH=m
# CONFIG_IP_VS_SED is not set
# CONFIG_IP_VS_NQ is not set
CONFIG_IP_VS_SH_TAB_BITS=8
> > net/netfilter/ipvs/ip_vs_ctl.c: In function 'ip_vs_genl_set_cmd':
> > net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.udp_timeout' may be used uninitialized in this function [-Wuninitialized]
> > net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.udp_timeout' was declared here
> > net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_fin_timeout' may be used uninitialized in this function [-Wuninitialized]
> > net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_fin_timeout' was declared here
> > net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_timeout' may be used uninitialized in this function [-Wuninitialized]
> > net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_timeout' was declared here
>
> There are many __ip_vs_get_timeouts callers but
> just one calls memset(&t, 0, sizeof(t)) before that,
> problem only for ip_vs_genl_set_config and ip_vs_set_timeout.
>
> To be safe, can we move this memset into
> __ip_vs_get_timeouts instead of playing games with defines?:
>
> memset(t, 0, sizeof(*t));
>
> This debug message will be more precise in showing the
> changed values if we replace the __ip_vs_get_timeouts
> call in ip_vs_genl_set_config with memset(&t, 0, sizeof(t)).
> Then we will see 0 for values that are not changed/supported.
8<-----
ipvs: initialize returned data in do_ip_vs_get_ctl
As reported by a gcc warning, the do_ip_vs_get_ctl does not initalize
all the members of the ip_vs_timeout_user structure it returns if
at least one of the TCP or UDP protocols is disabled for ipvs.
This makes sure that the data is always initialized, before it is
returned as a response to IPVS_CMD_GET_CONFIG or printed as a
debug message in IPVS_CMD_SET_CONFIG.
Without this patch, building ARM ixp4xx_defconfig results in:
net/netfilter/ipvs/ip_vs_ctl.c: In function 'ip_vs_genl_set_cmd':
net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.udp_timeout' may be used uninitialized in this function [-Wuninitialized]
net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.udp_timeout' was declared here
net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_fin_timeout' may be used uninitialized in this function [-Wuninitialized]
net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_fin_timeout' was declared here
net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_timeout' may be used uninitialized in this function [-Wuninitialized]
net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_timeout' was declared here
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 2770f85..3995d2e 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -2590,6 +2588,7 @@ __ip_vs_get_timeouts(struct net *net, struct ip_vs_timeout_user *u)
#if defined(CONFIG_IP_VS_PROTO_TCP) || defined(CONFIG_IP_VS_PROTO_UDP)
struct ip_vs_proto_data *pd;
#endif
+ memset(u, 0, sizeof (*u));
#ifdef CONFIG_IP_VS_PROTO_TCP
pd = ip_vs_proto_data_get(net, IPPROTO_TCP);
@@ -2768,7 +2767,6 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
{
struct ip_vs_timeout_user t;
- memset(&t, 0, sizeof(t));
__ip_vs_get_timeouts(net, &t);
if (copy_to_user(user, &t, sizeof(t)) != 0)
ret = -EFAULT;
^ permalink raw reply related
* [PATCH v2] ipv6: GRO should be ECN friendly
From: Eric Dumazet @ 2012-10-06 6:43 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, netdev
From: Eric Dumazet <edumazet@google.com>
IPv4 side of the problem was addressed in commit a9e050f4e7f9d
(net: tcp: GRO should be ECN friendly)
This patch does the same, but for IPv6 : A Traffic Class mismatch
doesnt mean flows are different, but instead should force a flush
of previous packets.
This patch removes artificial packet reordering problem.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
v2: sparse friendly version
net/ipv6/af_inet6.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index e22e6d8..f757e3b 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -880,22 +880,25 @@ static struct sk_buff **ipv6_gro_receive(struct sk_buff **head,
nlen = skb_network_header_len(skb);
for (p = *head; p; p = p->next) {
- struct ipv6hdr *iph2;
+ const struct ipv6hdr *iph2;
+ __be32 first_word; /* <Version:4><Traffic_Class:8><Flow_Label:20> */
if (!NAPI_GRO_CB(p)->same_flow)
continue;
iph2 = ipv6_hdr(p);
+ first_word = *(__be32 *)iph ^ *(__be32 *)iph2 ;
- /* All fields must match except length. */
+ /* All fields must match except length and Traffic Class. */
if (nlen != skb_network_header_len(p) ||
- memcmp(iph, iph2, offsetof(struct ipv6hdr, payload_len)) ||
+ (first_word & htonl(0xF00FFFFF)) ||
memcmp(&iph->nexthdr, &iph2->nexthdr,
nlen - offsetof(struct ipv6hdr, nexthdr))) {
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
-
+ /* flush if Traffic Class fields are different */
+ NAPI_GRO_CB(p)->flush |= !!(first_word & htonl(0x0FF00000));
NAPI_GRO_CB(p)->flush |= flush;
}
^ permalink raw reply related
* [PATCH] ipv6: GRO should be ECN friendly
From: Eric Dumazet @ 2012-10-06 6:32 UTC (permalink / raw)
To: David Miller; +Cc: Herbert Xu, netdev
From: Eric Dumazet <edumazet@google.com>
IPv4 side of the problem was addressed in commit a9e050f4e7f9d
(net: tcp: GRO should be ECN friendly)
This patch does the same, but for IPv6 : A Traffic Class mismatch
doesnt mean flows are different, but instead should force a flush
of previous packets.
This patch removes artificial packet reordering problem.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
net/ipv6/af_inet6.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index e22e6d8..7739d28 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -880,22 +880,25 @@ static struct sk_buff **ipv6_gro_receive(struct sk_buff **head,
nlen = skb_network_header_len(skb);
for (p = *head; p; p = p->next) {
- struct ipv6hdr *iph2;
+ const struct ipv6hdr *iph2;
+ u32 first_word; /* <Version:4><Traffic_Class:8><Flow_Label:20> */
if (!NAPI_GRO_CB(p)->same_flow)
continue;
iph2 = ipv6_hdr(p);
+ first_word = *(u32 *)iph ^ *(u32 *)iph2 ;
- /* All fields must match except length. */
+ /* All fields must match except length and Traffic Class. */
if (nlen != skb_network_header_len(p) ||
- memcmp(iph, iph2, offsetof(struct ipv6hdr, payload_len)) ||
+ (first_word & htonl(0xF00FFFFF)) ||
memcmp(&iph->nexthdr, &iph2->nexthdr,
nlen - offsetof(struct ipv6hdr, nexthdr))) {
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
-
+ /* flush if Traffic Class fields are different */
+ NAPI_GRO_CB(p)->flush |= (first_word & htonl(0x0FF00000));
NAPI_GRO_CB(p)->flush |= flush;
}
^ permalink raw reply related
* Re: [RFC] GRO scalability
From: Eric Dumazet @ 2012-10-06 6:22 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, Jesse Gross
In-Reply-To: <20121006051407.GA27390@gondor.apana.org.au>
On Sat, 2012-10-06 at 13:14 +0800, Herbert Xu wrote:
> On Sat, Oct 06, 2012 at 07:08:46AM +0200, Eric Dumazet wrote:
> > Le samedi 06 octobre 2012 à 12:11 +0800, Herbert Xu a écrit :
> > > On Fri, Oct 05, 2012 at 04:52:27PM +0200, Eric Dumazet wrote:
> > > > Current GRO cell is somewhat limited :
> > > >
> > > > - It uses a single list (napi->gro_list) of pending skbs
> > > >
> > > > - This list has a limit of 8 skbs (MAX_GRO_SKBS)
> > > >
> > > > - Workloads with lot of concurrent flows have small GRO hit rate but
> > > > pay high overhead (in inet_gro_receive())
> > > >
> > > > - Increasing MAX_GRO_SKBS is not an option, because GRO
> > > > overhead becomes too high.
> > >
> > > Yeah these were all meant to be addressed at some point.
> > >
> > > > - Packets can stay a long time held in GRO cell (there is
> > > > no flush if napi never completes on a stressed cpu)
> > >
> > > This should never happen though. NAPI runs must always be
> > > punctuated just to guarantee one card never hogs a CPU. Which
> > > driver causes these behaviour?
> >
> > I believe its a generic issue, not specific to a driver.
> >
> > napi_gro_flush() is only called from napi_complete()
> >
> > Some drivers (marvell/skge.c & realtek/8139cp.c) calls it only because
> > they 'inline' napi_complete()
>
> So which driver has the potential of never doing napi_gro_flush?
All drivers.
If the napi->poll() consumes all budget, we dont call napi_complete()
^ permalink raw reply
* [PATCH] RDS: Fix spinlock recursion for rds over tcp transmit
From: Jeff Liu @ 2012-10-06 5:42 UTC (permalink / raw)
To: rds-devel; +Cc: Venkat Venkatsubra, Dan Carpenter, davem, James Morris, netdev
Hello,
RDS ping/pong over TCP feature has broke for years(2.6.39 to 3.6.0) since we have to set TCP cork and
call kerenel_sendmsg() to reply a ping requirement which both need to lock "struct sock *sk".
However, this lock has already been hold before our rda_tcp_data_ready() callback is triggerred.
As a result, we always facing spinlock recursion which would resulting in system panic...
Given that RDS ping is a special kind of message, we don't need to reply it as
soon as possible, IMHO, we can schedule it to work queue as a delayed response to
make TCP transport totally works. Also, I think we can using the system default
work queue to serve it to reduce the possible impact on general TCP transmit.
With below patch, I have run rds-ping(run multiple ping between two hosts at the same time) and
rds-stress(hostA listen, hostB send packets) for half day, it works to me.
Thanks,
-Jeff
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
CC: Venkat Venkatsubra <venkat.x.venkatsubra@oracle.com>
CC: David S. Miller <davem@davemloft.net>
CC: James Morris <james.l.morris@oracle.com>
Signed-off-by: Jie Liu <jeff.liu@oracle.com>
---
net/rds/send.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
net/rds/tcp.h | 7 +++++
2 files changed, 82 insertions(+), 5 deletions(-)
diff --git a/net/rds/send.c b/net/rds/send.c
index 96531d4..011006e 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -38,8 +38,10 @@
#include <linux/list.h>
#include <linux/ratelimit.h>
#include <linux/export.h>
+#include <linux/workqueue.h>
#include "rds.h"
+#include "tcp.h"
/* When transmitting messages in rds_send_xmit, we need to emerge from
* time to time and briefly release the CPU. Otherwise the softlock watchdog
@@ -55,6 +57,12 @@ static int send_batch_count = 64;
module_param(send_batch_count, int, 0444);
MODULE_PARM_DESC(send_batch_count, " batch factor when working the send queue");
+/* RDS over TCP ping/pong */
+static void rds_tcp_pong_worker(struct work_struct *work);
+static DEFINE_SPINLOCK(rds_tcp_pong_lock);
+static LIST_HEAD(rds_tcp_pong_list);
+static DECLARE_WORK(rds_tcp_pong_work, rds_tcp_pong_worker);
+
static void rds_send_remove_from_sock(struct list_head *messages, int status);
/*
@@ -1082,11 +1090,7 @@ out:
return ret;
}
-/*
- * Reply to a ping packet.
- */
-int
-rds_send_pong(struct rds_connection *conn, __be16 dport)
+static int rds_tcp_send_pong(struct rds_connection *conn, __be16 dport)
{
struct rds_message *rm;
unsigned long flags;
@@ -1132,3 +1136,69 @@ out:
rds_message_put(rm);
return ret;
}
+
+static void rds_tcp_pong_worker(struct work_struct *work)
+{
+ struct rds_tcp_pong *tp;
+ struct rds_connection *conn;
+ __be16 dport;
+
+ spin_lock(&rds_tcp_pong_lock);
+ if (list_empty(&rds_tcp_pong_list))
+ goto out_unlock;
+
+ /*
+ * Process on tcp pong once one time to reduce the possbile impact
+ * on normal transmit.
+ */
+ tp = list_entry(rds_tcp_pong_list.next, struct rds_tcp_pong, tp_node);
+ conn = tp->tp_conn;
+ dport = tp->tp_dport;
+ list_del(&tp->tp_node);
+ spin_unlock(&rds_tcp_pong_lock);
+
+ kfree(tp);
+ rds_tcp_send_pong(conn, dport);
+ goto out;
+
+out_unlock:
+ spin_unlock(&rds_tcp_pong_lock);
+out:
+ return;
+}
+
+/*
+ * RDS over TCP transport support ping/pong message. However, it
+ * always resulting in sock spinlock recursion up to 3.7.0. To solve
+ * this issue, we can shedule it to work queue as a delayed response.
+ * Here we using the system default work queue.
+ */
+int rds_tcp_pong(struct rds_connection *conn, __be16 dport)
+{
+ struct rds_tcp_pong *tp;
+ int ret = 0;
+
+ tp = kmalloc(sizeof(*tp), GFP_ATOMIC);
+ if (!tp) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ tp->tp_conn = conn;
+ tp->tp_dport = dport;
+ spin_lock(&rds_tcp_pong_lock);
+ list_add_tail(&tp->tp_node, &rds_tcp_pong_list);
+ spin_unlock(&rds_tcp_pong_lock);
+ schedule_work(&rds_tcp_pong_work);
+
+out:
+ return ret;
+}
+
+/*
+ * Reply to a ping package, TCP only.
+ */
+int rds_send_pong(struct rds_connection *conn, __be16 dport)
+{
+ return rds_tcp_pong(conn, dport);
+}
diff --git a/net/rds/tcp.h b/net/rds/tcp.h
index 9cf2927..c4c7e01 100644
--- a/net/rds/tcp.h
+++ b/net/rds/tcp.h
@@ -3,6 +3,13 @@
#define RDS_TCP_PORT 16385
+/* RDS over TCP ping/pong message entry */
+struct rds_tcp_pong {
+ struct list_head tp_node;
+ struct rds_connection *tp_conn;
+ __be16 tp_dport;
+};
+
struct rds_tcp_incoming {
struct rds_incoming ti_inc;
struct sk_buff_head ti_skb_list;
--
1.7.9.5
^ permalink raw reply related
* Re: [RFC] GRO scalability
From: Herbert Xu @ 2012-10-06 5:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Jesse Gross
In-Reply-To: <1349500126.4883.4.camel@edumazet-laptop>
On Sat, Oct 06, 2012 at 07:08:46AM +0200, Eric Dumazet wrote:
> Le samedi 06 octobre 2012 à 12:11 +0800, Herbert Xu a écrit :
> > On Fri, Oct 05, 2012 at 04:52:27PM +0200, Eric Dumazet wrote:
> > > Current GRO cell is somewhat limited :
> > >
> > > - It uses a single list (napi->gro_list) of pending skbs
> > >
> > > - This list has a limit of 8 skbs (MAX_GRO_SKBS)
> > >
> > > - Workloads with lot of concurrent flows have small GRO hit rate but
> > > pay high overhead (in inet_gro_receive())
> > >
> > > - Increasing MAX_GRO_SKBS is not an option, because GRO
> > > overhead becomes too high.
> >
> > Yeah these were all meant to be addressed at some point.
> >
> > > - Packets can stay a long time held in GRO cell (there is
> > > no flush if napi never completes on a stressed cpu)
> >
> > This should never happen though. NAPI runs must always be
> > punctuated just to guarantee one card never hogs a CPU. Which
> > driver causes these behaviour?
>
> I believe its a generic issue, not specific to a driver.
>
> napi_gro_flush() is only called from napi_complete()
>
> Some drivers (marvell/skge.c & realtek/8139cp.c) calls it only because
> they 'inline' napi_complete()
So which driver has the potential of never doing napi_gro_flush?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH V2] net: Fix skb_under_panic oops in neigh_resolve_output
From: ramesh.nagappa @ 2012-10-06 5:10 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, ebiederm, xemul, linux-kernel, Ramesh Nagappa
The retry loop in neigh_resolve_output() and neigh_connected_output()
call dev_hard_header() with out reseting the skb to network_header.
This causes the retry to fail with skb_under_panic. The fix is to
reset the network_header within the retry loop.
Signed-off-by: Ramesh Nagappa <ramesh.nagappa@ericsson.com>
Reviewed-by: Shawn Lu <shawn.lu@ericsson.com>
Reviewed-by: Robert Coulson <robert.coulson@ericsson.com>
Reviewed-by: Billie Alsup <billie.alsup@ericsson.com>
---
net/core/neighbour.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 117afaf..058bb1e 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1301,8 +1301,6 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb)
if (!dst)
goto discard;
- __skb_pull(skb, skb_network_offset(skb));
-
if (!neigh_event_send(neigh, skb)) {
int err;
struct net_device *dev = neigh->dev;
@@ -1312,6 +1310,7 @@ int neigh_resolve_output(struct neighbour *neigh, struct sk_buff *skb)
neigh_hh_init(neigh, dst);
do {
+ __skb_pull(skb, skb_network_offset(skb));
seq = read_seqbegin(&neigh->ha_lock);
err = dev_hard_header(skb, dev, ntohs(skb->protocol),
neigh->ha, NULL, skb->len);
@@ -1342,9 +1341,8 @@ int neigh_connected_output(struct neighbour *neigh, struct sk_buff *skb)
unsigned int seq;
int err;
- __skb_pull(skb, skb_network_offset(skb));
-
do {
+ __skb_pull(skb, skb_network_offset(skb));
seq = read_seqbegin(&neigh->ha_lock);
err = dev_hard_header(skb, dev, ntohs(skb->protocol),
neigh->ha, NULL, skb->len);
--
1.7.11.4
^ permalink raw reply related
* Re: [RFC] GRO scalability
From: Eric Dumazet @ 2012-10-06 5:08 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, Jesse Gross
In-Reply-To: <20121006041155.GA27134@gondor.apana.org.au>
Le samedi 06 octobre 2012 à 12:11 +0800, Herbert Xu a écrit :
> On Fri, Oct 05, 2012 at 04:52:27PM +0200, Eric Dumazet wrote:
> > Current GRO cell is somewhat limited :
> >
> > - It uses a single list (napi->gro_list) of pending skbs
> >
> > - This list has a limit of 8 skbs (MAX_GRO_SKBS)
> >
> > - Workloads with lot of concurrent flows have small GRO hit rate but
> > pay high overhead (in inet_gro_receive())
> >
> > - Increasing MAX_GRO_SKBS is not an option, because GRO
> > overhead becomes too high.
>
> Yeah these were all meant to be addressed at some point.
>
> > - Packets can stay a long time held in GRO cell (there is
> > no flush if napi never completes on a stressed cpu)
>
> This should never happen though. NAPI runs must always be
> punctuated just to guarantee one card never hogs a CPU. Which
> driver causes these behaviour?
I believe its a generic issue, not specific to a driver.
napi_gro_flush() is only called from napi_complete()
Some drivers (marvell/skge.c & realtek/8139cp.c) calls it only because
they 'inline' napi_complete()
^ permalink raw reply
* Re: [RFC] GRO scalability
From: Herbert Xu @ 2012-10-06 4:11 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Jesse Gross
In-Reply-To: <1349448747.21172.113.camel@edumazet-glaptop>
On Fri, Oct 05, 2012 at 04:52:27PM +0200, Eric Dumazet wrote:
> Current GRO cell is somewhat limited :
>
> - It uses a single list (napi->gro_list) of pending skbs
>
> - This list has a limit of 8 skbs (MAX_GRO_SKBS)
>
> - Workloads with lot of concurrent flows have small GRO hit rate but
> pay high overhead (in inet_gro_receive())
>
> - Increasing MAX_GRO_SKBS is not an option, because GRO
> overhead becomes too high.
Yeah these were all meant to be addressed at some point.
> - Packets can stay a long time held in GRO cell (there is
> no flush if napi never completes on a stressed cpu)
This should never happen though. NAPI runs must always be
punctuated just to guarantee one card never hogs a CPU. Which
driver causes these behaviour?
> Some elephant flows can stall interactive ones (if we receive
> flood of non TCP frames, we dont flush tcp packets waiting in
> gro_list)
Again this should never be a problem given the natural limit
on backlog processing.
> What we could do :
>
> 1) Use a hash to avoid expensive gro_list management and allow
> much more concurrent flows.
>
> Use skb_get_rxhash(skb) to compute rxhash
>
> If l4_rxhash not set -> not a GRO candidate.
>
> If l4_rxhash set, use a hash lookup to immediately finds a 'same flow'
> candidates.
>
> (tcp stack could eventually use rxhash instead of its custom hash
> computation ...)
Sounds good to me.
> 2) Use a LRU list to eventually be able to 'flush' too old packets,
> even if the napi never completes. Each time we process a new packet,
> being a GRO candidate or not, we increment a napi->sequence, and we
> flush the oldest packet in gro_lru_list if its own sequence is too
> old.
>
> That would give a latency guarantee.
I don't think this should ever be necessary. IOW, if we need this
for GRO, then it means that we also need it for NAPI for the exact
same reasons.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* replace existing filter creates new filter instead
From: John A. Sullivan III @ 2012-10-06 2:43 UTC (permalink / raw)
To: netdev
Hello, all. I write most of my tc scripts using replace so I can reload
my configuration as well as create it anew. However, one particular
filter command is giving me grief. The below sequence show my listing
of the filters showing and empty list. Then I replace one and see one.
Then I merely up arrow to execute the exact same replace command and I
then see two filters when I would have expected to see one. What am I
misunderstanding? Thanks - John
# tc filter show dev bond3 parent 1:0
# tc filter replace dev bond3 parent 1:0 protocol ip prio 1 u32 match u8 0 0 flowid 1:1 action mirred egress redirect dev ifb1
Action 4 device ifb1 ifindex 26
# tc filter show dev bond3 parent 1:0
filter protocol ip pref 1 u32
filter protocol ip pref 1 u32 fh 800: ht divisor 1
filter protocol ip pref 1 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1
match 00000000/00000000 at 0
action order 1: mirred (Egress Redirect to device ifb1) stolen
index 27 ref 1 bind 1
# tc filter replace dev bond3 parent 1:0 protocol ip prio 1 u32 match u8 0 0 flowid 1:1 action mirred egress redirect dev ifb1
Action 4 device ifb1 ifindex 26
# tc filter show dev bond3 parent 1:0
filter protocol ip pref 1 u32
filter protocol ip pref 1 u32 fh 800: ht divisor 1
filter protocol ip pref 1 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:1
match 00000000/00000000 at 0
action order 1: mirred (Egress Redirect to device ifb1) stolen
index 27 ref 1 bind 1
filter protocol ip pref 1 u32 fh 800::801 order 2049 key ht 800 bkt 0 flowid 1:1
match 00000000/00000000 at 0
action order 33: mirred (Egress Redirect to device ifb1) stolen
index 28 ref 1 bind 1
^ permalink raw reply
* Re: drivers/net/cris/eth_v10.c:1715:2: error: too many arguments to function 'e100rxtx_interrupt'
From: Fengguang Wu @ 2012-10-06 1:07 UTC (permalink / raw)
To: Jesper Nilsson
Cc: David Miller, kernel-janitors@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <20121005212422.GA1390@axis.com>
On Fri, Oct 05, 2012 at 11:24:22PM +0200, Jesper Nilsson wrote:
> On Wed, Oct 03, 2012 at 10:26:57PM +0200, Jesper Nilsson wrote:
> > On Wed, Oct 03, 2012 at 08:40:40PM +0200, David Miller wrote:
> > > From: Jesper Nilsson <jesper.nilsson@axis.com>
> > > Date: Wed, 3 Oct 2012 12:46:52 +0200
> > >
> > > > On Fri, Sep 28, 2012 at 03:06:08PM +0200, Fengguang Wu wrote:
> > > >> Hi Jesper,
> > > >
> > > > Hi!
> > > >
> > > >> FYI, a rather old build bug that's introduced by
> > > >>
> > > >> bafef0a cris build fixes: update eth_v10.c ethernet driver
> > > >>
> > > >> All error/warnings:
> > > >>
> > > >> drivers/net/cris/eth_v10.c: In function 'e100_netpoll':
> > > >> drivers/net/cris/eth_v10.c:1715:2: error: too many arguments to function 'e100rxtx_interrupt'
> > > >> drivers/net/cris/eth_v10.c:1131:1: note: declared here
> > > >
> > > > Yep, I can't figure out why the followup patches never reached mainline,
> > > > but we have fixes for exactly that in our in-house tree.
> > > > I'll push some move patches after this merge window.
> > >
> > > It's a bug fix, even worse a build fix, why want until after the merge
> > > window?
> >
> > Aye, true, I'll just have to make sure I don't get any other change from
> > the inhouse tree.
>
> Turns out this was already fixed in mainline since August.
Embarrassing.. It must be testing some git tree that's based on one
upstream HEAD sometime between where the bug was introduced and where
it's fixed.
Thanks,
Fengguang
^ permalink raw reply
* Re: [PATCH] mlx4: set carrier off after register netdev
From: Min Zhang @ 2012-10-06 0:02 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, linux-kernel
In-Reply-To: <1349470415.2819.1.camel@bwh-desktop.uk.solarflarecom.com>
On Fri, 5 Oct 2012, Ben Hutchings wrote:
> On Fri, 2012-10-05 at 11:28 -0700, Min Zhang wrote:
> > ifconfig mlx4_en port reported RUNNING even though the link was down.
> >
> > mlx4_en_init_netdev didn't initialize the dev operstate properly so
> > the operstate stayed as default IF_OPER_UNKNOWN, then ifconfig treated
> > the UNKNOWN as RUNNING state for backward compatiblity per RFC2863.
> >
> > The fix calls netif_carrier_off which is supposed to set operstate
> > after register_netdev. Calling it before register_netdev has no effect
> > since the dev->state is still NETREG_UNINITIALIZED
> >
> > Tested by removing the physical link signal to the mellanox 10G port,
> > modprobe mlx4_en, then ifconfig up. Verify there is no RUNNING status.
> [...]
>
> This was supposed to be fixed by:
>
> commit 8f4cccbbd92f2ad0ddbbc498ef7cee2a1c3defe9
> Author: Ben Hutchings <bhutchings@solarflare.com>
> Date: Mon Aug 20 22:16:51 2012 +0100
>
> net: Set device operstate at registration time
>
> Does that not work for mlx4_en, for some reason?
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
Indeed, commit 8f4cccbbd92f2ad0ddbbc498ef7cee2a1c3defe9 does fix the issue
in mlx4. I didn't have the newest net/core. Therefore ignore my mlx4 fix.
^ permalink raw reply
* ethtool 3.6 released
From: Ben Hutchings @ 2012-10-05 23:15 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
ethtool version 3.6 has been released.
Home page: https://ftp.kernel.org/pub/software/network/ethtool/
Download link:
https://ftp.kernel.org/pub/software/network/ethtool/ethtool-3.6.tar.xz
Release notes:
* Feature: Allow setting MDI-X state (-s option)
* Fix: Preserve pause advertising bits when setting speed and
duplex with autoneg on (-s option)
* Fix: Don't call ioctl to set EEE parameters if they are the same
as the current parameters (--set-eee option)
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [ethtool] ethtool: --set-eee sends ETHTOOL_SEEE ioctl even if nothing changed
From: Ben Hutchings @ 2012-10-05 23:14 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: Bruce Allan, netdev, gospo, sassmann
In-Reply-To: <1348024819-29876-1-git-send-email-jeffrey.t.kirsher@intel.com>
On Tue, 2012-09-18 at 20:20 -0700, Jeff Kirsher wrote:
> From: Bruce Allan <bruce.w.allan@intel.com>
>
> When setting EEE parameters with the --set-eee command line option,
> ethtool will send the ETHTOOL_SEEE ioctl down to the driver even if
> none of the provided parameters are a change from current settings.
> Simply ignore it when that happens as done with other ethtool commands.
[...]
Applied, thanks. Sorry for the slow response.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH 20/20 V2] drivers/net/ethernet/marvell/sky2.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:40 UTC (permalink / raw)
To: mlindner
Cc: shemminger, netdev, linux-kernel, kernel-janitors,
Peter Senna Tschudin
In-Reply-To: <1349476856-16075-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function sky2_probe() return 0 for success and negative value
for most of its internal tests failures. There are two exceptions
that are error cases going to err_out*:. For this two cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/marvell/sky2.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 2b0748d..78946fe 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -4924,6 +4924,7 @@ static int __devinit sky2_probe(struct pci_dev *pdev,
if (~reg == 0) {
dev_err(&pdev->dev, "PCI configuration read error\n");
+ err = -EIO;
goto err_out;
}
@@ -4993,8 +4994,10 @@ static int __devinit sky2_probe(struct pci_dev *pdev,
hw->st_size = hw->ports * roundup_pow_of_two(3*RX_MAX_PENDING + TX_MAX_PENDING);
hw->st_le = pci_alloc_consistent(pdev, hw->st_size * sizeof(struct sky2_status_le),
&hw->st_dma);
- if (!hw->st_le)
+ if (!hw->st_le) {
+ err = -ENOMEM;
goto err_out_reset;
+ }
dev_info(&pdev->dev, "Yukon-2 %s chip revision %d\n",
sky2_name(hw->chip_id, buf1, sizeof(buf1)), hw->chip_rev);
^ permalink raw reply related
* [PATCH 19/20 V2] drivers/net/ethernet/marvell/skge.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:40 UTC (permalink / raw)
To: mlindner
Cc: shemminger, netdev, linux-kernel, kernel-janitors,
Peter Senna Tschudin
In-Reply-To: <1349476856-16075-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function skge_probe() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to err_out_led_off:. For this error case, the
function abort its success execution path, but returns non negative
value, making it difficult for a caller function to notice the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/marvell/skge.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/skge.c b/drivers/net/ethernet/marvell/skge.c
index 5a30bf8..91836b5 100644
--- a/drivers/net/ethernet/marvell/skge.c
+++ b/drivers/net/ethernet/marvell/skge.c
@@ -3945,8 +3945,10 @@ static int __devinit skge_probe(struct pci_dev *pdev,
skge_board_name(hw), hw->chip_rev);
dev = skge_devinit(hw, 0, using_dac);
- if (!dev)
+ if (!dev) {
+ err = -ENOMEM;
goto err_out_led_off;
+ }
/* Some motherboards are broken and has zero in ROM. */
if (!is_valid_ether_addr(dev->dev_addr))
^ permalink raw reply related
* [PATCH 18/20 V2] drivers/net/ethernet/sun/sungem.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:40 UTC (permalink / raw)
To: davem
Cc: jdmason, gerard.lledo, dhowells, joe, netdev, linux-kernel,
kernel-janitors, Peter Senna Tschudin
In-Reply-To: <1349476856-16075-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function gem_init_one() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to err_out_free_consistent:. For this error
case, the function abort its success execution path, but returns non
negative value, making it difficult for a caller function to notice
the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/sun/sungem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
index 9ae12d0..6c8695e 100644
--- a/drivers/net/ethernet/sun/sungem.c
+++ b/drivers/net/ethernet/sun/sungem.c
@@ -2963,7 +2963,8 @@ static int __devinit gem_init_one(struct pci_dev *pdev,
goto err_out_iounmap;
}
- if (gem_get_device_address(gp))
+ err = gem_get_device_address(gp);
+ if (err)
goto err_out_free_consistent;
dev->netdev_ops = &gem_netdev_ops;
^ permalink raw reply related
* [PATCH 17/20 V2] drivers/net/ethernet/sun/niu.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:40 UTC (permalink / raw)
To: davem
Cc: mcarlson, eric.dumazet, mchan, shuah.khan, netdev, linux-kernel,
kernel-janitors, Peter Senna Tschudin
In-Reply-To: <1349476856-16075-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function niu_pci_init_one() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to err_out_free_res:. For this error case, the
function abort its success execution path, but returns non negative
value, making it difficult for a caller function to notice the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/sun/niu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 8419bf3..275b430 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -9788,6 +9788,7 @@ static int __devinit niu_pci_init_one(struct pci_dev *pdev,
if (!pci_is_pcie(pdev)) {
dev_err(&pdev->dev, "Cannot find PCI Express capability, aborting\n");
+ err = -ENODEV;
goto err_out_free_res;
}
^ permalink raw reply related
* [PATCH 16/20 V2] drivers/net/ethernet/renesas/sh_eth.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:40 UTC (permalink / raw)
To: davem
Cc: yoshihiro.shimoda.uh, nobuhiro.iwamatsu.yj, netdev, linux-kernel,
kernel-janitors, Peter Senna Tschudin
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function sh_eth_drv_probe() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to out_release:. For this error case, the
function abort its success execution path, but returns non negative
value, making it difficult for a caller function to notice the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/renesas/sh_eth.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index bad8f2e..c8bfea0 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2438,6 +2438,7 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (!rtsu) {
dev_err(&pdev->dev, "Not found TSU resource\n");
+ ret = -ENODEV;
goto out_release;
}
mdp->tsu_addr = ioremap(rtsu->start,
^ permalink raw reply related
* [PATCH 15/20 V2] drivers/net/ethernet/natsemi/xtsonic.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:10 UTC (permalink / raw)
To: davem
Cc: mcuos.com, axel.lin, netdev, linux-kernel, kernel-janitors,
Peter Senna Tschudin
In-Reply-To: <1349475053-11464-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function sonic_probe1() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to out:. For this error case, the
function abort its success execution path, but returns non negative
value, making it difficult for a caller function to notice the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/natsemi/xtsonic.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/natsemi/xtsonic.c b/drivers/net/ethernet/natsemi/xtsonic.c
index e01c0a0..7dfe883 100644
--- a/drivers/net/ethernet/natsemi/xtsonic.c
+++ b/drivers/net/ethernet/natsemi/xtsonic.c
@@ -205,6 +205,7 @@ static int __init sonic_probe1(struct net_device *dev)
if (lp->descriptors == NULL) {
printk(KERN_ERR "%s: couldn't alloc DMA memory for "
" descriptors.\n", dev_name(lp->device));
+ err = -ENOMEM;
goto out;
}
^ permalink raw reply related
* [PATCH 14/20 V2] drivers/net/ethernet/amd/au1000_eth.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:10 UTC (permalink / raw)
To: davem
Cc: danny.kukawka, mcuos.com, joe, florian, netdev, linux-kernel,
kernel-janitors, Peter Senna Tschudin
In-Reply-To: <1349475053-11464-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function au1000_probe() return 0 for success and negative value
for most of its internal tests failures. There are exceptions
that are error cases going to err_out:. For this cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/amd/au1000_eth.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/amd/au1000_eth.c b/drivers/net/ethernet/amd/au1000_eth.c
index 397596b..f195acf 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1174,8 +1174,10 @@ static int __devinit au1000_probe(struct platform_device *pdev)
snprintf(aup->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
pdev->name, aup->mac_id);
aup->mii_bus->irq = kmalloc(sizeof(int)*PHY_MAX_ADDR, GFP_KERNEL);
- if (aup->mii_bus->irq == NULL)
+ if (aup->mii_bus->irq == NULL) {
+ err = -ENOMEM;
goto err_out;
+ }
for (i = 0; i < PHY_MAX_ADDR; ++i)
aup->mii_bus->irq[i] = PHY_POLL;
@@ -1190,7 +1192,8 @@ static int __devinit au1000_probe(struct platform_device *pdev)
goto err_mdiobus_reg;
}
- if (au1000_mii_probe(dev) != 0)
+ err = au1000_mii_probe(dev);
+ if (err != 0)
goto err_out;
pDBfree = NULL;
@@ -1205,6 +1208,7 @@ static int __devinit au1000_probe(struct platform_device *pdev)
}
aup->pDBfree = pDBfree;
+ err = -ENODEV;
for (i = 0; i < NUM_RX_DMA; i++) {
pDB = au1000_GetFreeDB(aup);
if (!pDB)
@@ -1213,6 +1217,8 @@ static int __devinit au1000_probe(struct platform_device *pdev)
aup->rx_dma_ring[i]->buff_stat = (unsigned)pDB->dma_addr;
aup->rx_db_inuse[i] = pDB;
}
+
+ err = -ENODEV;
for (i = 0; i < NUM_TX_DMA; i++) {
pDB = au1000_GetFreeDB(aup);
if (!pDB)
^ permalink raw reply related
* [PATCH 13/20 V2] drivers/net/ethernet/amd/amd8111e.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:10 UTC (permalink / raw)
To: davem
Cc: joe, dhowells, rick.jones2, netdev, netdev, linux-kernel,
kernel-janitors, Peter Senna Tschudin
In-Reply-To: <1349475053-11464-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function amd8111e_probe_one() return 0 for success and negative
value for most of its internal tests failures. There are two exceptions
that are error cases going to err_free_reg:. For this two cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/amd/amd8111e.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/amd/amd8111e.c b/drivers/net/ethernet/amd/amd8111e.c
index 64d0d9c..3491d43 100644
--- a/drivers/net/ethernet/amd/amd8111e.c
+++ b/drivers/net/ethernet/amd/amd8111e.c
@@ -1845,6 +1845,7 @@ static int __devinit amd8111e_probe_one(struct pci_dev *pdev,
if((pm_cap = pci_find_capability(pdev, PCI_CAP_ID_PM))==0){
printk(KERN_ERR "amd8111e: No Power Management capability, "
"exiting.\n");
+ err = -ENODEV;
goto err_free_reg;
}
@@ -1852,6 +1853,7 @@ static int __devinit amd8111e_probe_one(struct pci_dev *pdev,
if (pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) < 0) {
printk(KERN_ERR "amd8111e: DMA not supported,"
"exiting.\n");
+ err = -ENODEV;
goto err_free_reg;
}
^ permalink raw reply related
* [PATCH 12/20 V2] drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:10 UTC (permalink / raw)
To: jitendra.kalsaria
Cc: sony.chacko, linux-driver, netdev, linux-kernel, kernel-janitors,
Peter Senna Tschudin
In-Reply-To: <1349475053-11464-1-git-send-email-peter.senna@gmail.com>
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function qlcnic_probe() return 0 for success and negative value
for most of its internal tests failures. There is one exception
that is error case going to err_out_free_netdev:. For this error case,
the function abort its success execution path, but returns non negative
value, making it difficult for a caller function to notice the error.
This patch fixes the error case that do not return negative value.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 473ce13..24ad17e 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -1601,7 +1601,8 @@ qlcnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
adapter->netdev = netdev;
adapter->pdev = pdev;
- if (qlcnic_alloc_adapter_resources(adapter))
+ err = qlcnic_alloc_adapter_resources(adapter);
+ if (err)
goto err_out_free_netdev;
adapter->dev_rst_time = jiffies;
^ permalink raw reply related
* [PATCH 11/20 V2] drivers/net/irda/sh_sir.c: fix error return code
From: Peter Senna Tschudin @ 2012-10-05 22:10 UTC (permalink / raw)
To: samuel
Cc: irda-users, netdev, linux-kernel, kernel-janitors,
Peter Senna Tschudin
From: Peter Senna Tschudin <peter.senna@gmail.com>
The function sh_sir_probe() return 0 for success and negative value
for most of its internal tests failures. There are two exceptions
that are error cases going to err_mem_*:. For this two cases, the
function abort its success execution path, but returns non negative
value, making it dificult for a caller function to notice the error.
This patch fixes the error cases that do not return negative values.
This was found by Coccinelle, but the code change was made by hand.
This patch is not robot generated.
A simplified version of the semantic match that finds this problem is
as follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Change from V1:
Updated commit message. See:
http://www.kernelhub.org/?p=2&msg=139319
drivers/net/irda/sh_sir.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/irda/sh_sir.c b/drivers/net/irda/sh_sir.c
index 7951094..624ac19 100644
--- a/drivers/net/irda/sh_sir.c
+++ b/drivers/net/irda/sh_sir.c
@@ -741,6 +741,7 @@ static int __devinit sh_sir_probe(struct platform_device *pdev)
self->clk = clk_get(&pdev->dev, clk_name);
if (IS_ERR(self->clk)) {
dev_err(&pdev->dev, "cannot get clock \"%s\"\n", clk_name);
+ err = -ENODEV;
goto err_mem_3;
}
@@ -760,8 +761,8 @@ static int __devinit sh_sir_probe(struct platform_device *pdev)
goto err_mem_4;
platform_set_drvdata(pdev, ndev);
-
- if (request_irq(irq, sh_sir_irq, IRQF_DISABLED, "sh_sir", self)) {
+ err = request_irq(irq, sh_sir_irq, IRQF_DISABLED, "sh_sir", self);
+ if (err) {
dev_warn(&pdev->dev, "Unable to attach sh_sir interrupt\n");
goto err_mem_4;
}
^ permalink raw reply related
* Re: [PATCH 2/20 V2] drivers/net/ethernet/natsemi/natsemi.c: fix error return code
From: Francois Romieu @ 2012-10-05 21:40 UTC (permalink / raw)
To: Peter Senna Tschudin
Cc: davem, rick.jones2, netdev, netdev, linux-kernel, kernel-janitors
In-Reply-To: <1349469667-6137-2-git-send-email-peter.senna@gmail.com>
Peter Senna Tschudin <peter.senna@gmail.com> :
[...]
> The function natsemi_probe1() return 0 for success and negative value
> for most of its internal tests failures. There is one exception
> that is error case going to err_create_file:. Fore this error case the
> function abort its success execution path, but returns non negative value,
> making it difficult for a caller function to notice the error.
Ok. natsemi_probe1() forgets to return a negative status code in one of
its failure paths.
[...]
> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
Acked-by: Francois Romieu <romieu@fr.zoreil.com>
--
Ueimor
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox