* [PATCH 1/2] sctp: HEARTBEAT negotiation after ASCONF
From: Michio Honda @ 2011-08-22 15:44 UTC (permalink / raw)
To: netdev; +Cc: Wei Yongjun
>From c2f008caef8deb7dbb4ce629ec6b3c096bceddd9 Mon Sep 17 00:00:00 2001
From: Michio Honda <micchie@sfc.wide.ad.jp>
Date: Thu, 16 Jun 2011 10:54:23 +0900
Subject: [PATCH 1/2] sctp: HEARTBEAT negotiation after ASCONF
This patch fixes BUG that the ASCONF receiver transmits DATA chunks
to the newly added UNCONFIRMED destination.
Signed-off-by: Michio Honda <micchie@sfc.wide.ad.jp>
---
net/sctp/outqueue.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index a6d27bf..bcecdba 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -917,6 +917,8 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
* current cwnd).
*/
if (!list_empty(&q->retransmit)) {
+ if (asoc->peer.retran_path->state == SCTP_UNCONFIRMED)
+ goto sctp_flush_out;
if (transport == asoc->peer.retran_path)
goto retran;
@@ -989,6 +991,8 @@ static int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)
((new_transport->state == SCTP_INACTIVE) ||
(new_transport->state == SCTP_UNCONFIRMED)))
new_transport = asoc->peer.active_path;
+ if (new_transport->state == SCTP_UNCONFIRMED)
+ continue;
/* Change packets if necessary. */
if (new_transport != transport) {
--
1.7.3.2
^ permalink raw reply related
* [PATCH 2/2] sctp: Bundle HEAERTBEAT into ASCONF_ACK
From: Michio Honda @ 2011-08-22 15:44 UTC (permalink / raw)
To: netdev; +Cc: Wei Yongjun
>From e7cbeb4d796fec8351ede4affbd4269346639db9 Mon Sep 17 00:00:00 2001
From: Michio Honda <micchie@sfc.wide.ad.jp>
Date: Thu, 16 Jun 2011 17:14:34 +0900
Subject: [PATCH 2/2] sctp: Bundle HEAERTBEAT into ASCONF_ACK
With this patch a HEARTBEAT chunk is bundled into the ASCONF-ACK
for ADD IP ADDRESS, confirming the new destination as quickly as
possible.
Signed-off-by: Michio Honda <micchie@sfc.wide.ad.jp>
---
include/net/sctp/structs.h | 1 +
net/sctp/associola.c | 1 +
net/sctp/sm_make_chunk.c | 1 +
net/sctp/sm_statefuns.c | 5 +++++
4 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index f7d9c3f..e90e7a9 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1915,6 +1915,7 @@ struct sctp_association {
__u32 addip_serial;
union sctp_addr *asconf_addr_del_pending;
int src_out_of_asoc_ok;
+ struct sctp_transport *new_transport;
/* SCTP AUTH: list of the endpoint shared keys. These
* keys are provided out of band by the user applicaton
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index dc16b90..152b5b3 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -282,6 +282,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
asoc->peer.asconf_capable = 1;
asoc->asconf_addr_del_pending = NULL;
asoc->src_out_of_asoc_ok = 0;
+ asoc->new_transport = NULL;
/* Create an input queue. */
sctp_inq_init(&asoc->base.inqueue);
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 81db4e3..0121e0a 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3015,6 +3015,7 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
/* Start the heartbeat timer. */
if (!mod_timer(&peer->hb_timer, sctp_transport_timeout(peer)))
sctp_transport_hold(peer);
+ asoc->new_transport = peer;
break;
case SCTP_PARAM_DEL_IP:
/* ADDIP 4.3 D7) If a request is received to delete the
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 49b847b..73d14fc 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -3612,6 +3612,11 @@ sctp_disposition_t sctp_sf_do_asconf(const struct sctp_endpoint *ep,
*/
asconf_ack->dest = chunk->source;
sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(asconf_ack));
+ if (asoc->new_transport) {
+ sctp_sf_heartbeat(ep, asoc, type, asoc->new_transport,
+ commands);
+ ((struct sctp_association *)asoc)->new_transport = NULL;
+ }
return SCTP_DISPOSITION_CONSUME;
}
--
1.7.3.2
^ permalink raw reply related
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
From: Stephen Hemminger @ 2011-08-22 15:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jan Beulich, davem, netdev
In-Reply-To: <1314017401.2307.37.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
On Mon, 22 Aug 2011 14:50:01 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 22 août 2011 à 12:34 +0100, Jan Beulich a écrit :
> > Stephen,
> >
> > directly returning the result of register_netdev() from br_add_bridge()
> > would - to me - appear to be leaking "dev" in case of failure. Or am I
> > overlooking some implicit mechanism by which this would get freed?
> >
> > Thanks, Jan
>
> You're 100 % right, there is a leak here.
>
> (But "ip" or "brctl" commands first check link already exist before
> trying create it, so you need to tweak them to actually hit this bug)
>
> We have to add a free_netdev() call to free the net_device.
>
>
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 2cdf007..e738154 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
> int br_add_bridge(struct net *net, const char *name)
> {
> struct net_device *dev;
> + int res;
>
> dev = alloc_netdev(sizeof(struct net_bridge), name,
> br_dev_setup);
> @@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
>
> dev_net_set(dev, net);
>
> - return register_netdev(dev);
> + res = register_netdev(dev);
> + if (res)
> + free_netdev(dev);
> + return res;
> }
>
> int br_del_bridge(struct net *net, const char *name)
Missing Signed-off-by:?
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
^ permalink raw reply
* Re: Bridge stays down until a port is added
From: Stephen Hemminger @ 2011-08-22 15:57 UTC (permalink / raw)
To: Marc Haber; +Cc: netdev, Sven-Haegar Koch
In-Reply-To: <20110821121305.GA22232@torres.zugschlus.de>
On Sun, 21 Aug 2011 14:13:05 +0200
Marc Haber <mh+netdev@zugschlus.de> wrote:
> On Sat, Aug 20, 2011 at 09:30:59AM -0700, Stephen Hemminger wrote:
> > The problem is that IPv6 Duplicate Address Detection needs to
> > work. This is not a simple problem. If the bridge asserted
> > carrier with no ports then:
> >
> > 1. IPv6 address assigned and IPv6 decides it is okay.
> > 2. Port added later
> > 3. Another system has the same address.
> > *broke*
>
> Same situation when a system-to-system-link is added after bringing up
> an interface. I agree that the issue is not an issue if a real switch
> is being used.
>
> > If you want to avoid DAD, then you can configure disable DAD
> > by setting /proc/sys/net/ipv6/conf/br0/accept_dad to 0
>
> I'd like to avoid that.
>
> 2001:db8::1
>
> Would it acceptable (and clean!) to have:
>
> eth0: 2001:db8:1::100/64 default gw to the internet is 2001:db8:1::1
> lo: 127.0.0.1/8, ::1/128, 2001:db8:2::100/64
> br0: 2001:db8:2::1/64 (being default gw for the VMs connected to br0,
> routing 2001:db8:2::/64 to the Internet)
>
> Note 2001:db8:2::/64 being used both on lo and br0, with 2001:db8:2::100 meant
> to be reachable from the Internet even if no VM is already up. my
> hostname will be A-Recorded to 2001:db8:2::100 with proper reverse
> DNS. The background for doing so is that I cannot control the reverse
> DNS for the IP addresses inside 200a:db8:1::/64 in a lot of my setups
> (for example, if my IPv6 comes in via Sixxs).
>
No, DAD config is per interface and addrconf doesn't know anything
about your topology.
^ permalink raw reply
* Re: net_device leak after "bridge: allow creating bridge devices with netlink"?
From: Eric Dumazet @ 2011-08-22 16:05 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Jan Beulich, davem, netdev
In-Reply-To: <20110822085532.2ff4479c@nehalam.ftrdhcpuser.net>
Le lundi 22 août 2011 à 08:55 -0700, Stephen Hemminger a écrit :
> On Mon, 22 Aug 2011 14:50:01 +0200
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> > Le lundi 22 août 2011 à 12:34 +0100, Jan Beulich a écrit :
> > > Stephen,
> > >
> > > directly returning the result of register_netdev() from br_add_bridge()
> > > would - to me - appear to be leaking "dev" in case of failure. Or am I
> > > overlooking some implicit mechanism by which this would get freed?
> > >
> > > Thanks, Jan
> >
> > You're 100 % right, there is a leak here.
> >
> > (But "ip" or "brctl" commands first check link already exist before
> > trying create it, so you need to tweak them to actually hit this bug)
> >
> > We have to add a free_netdev() call to free the net_device.
> >
> >
> >
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index 2cdf007..e738154 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
> > int br_add_bridge(struct net *net, const char *name)
> > {
> > struct net_device *dev;
> > + int res;
> >
> > dev = alloc_netdev(sizeof(struct net_bridge), name,
> > br_dev_setup);
> > @@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
> >
> > dev_net_set(dev, net);
> >
> > - return register_netdev(dev);
> > + res = register_netdev(dev);
> > + if (res)
> > + free_netdev(dev);
> > + return res;
> > }
> >
> > int br_del_bridge(struct net *net, const char *name)
>
> Missing Signed-off-by:?
>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
Well, its only a direct coding of Jan report ;)
Thanks
[PATCH] bridge: fix a possible net_device leak
Jan Beulich reported a possible net_device leak in bridge code after
commit bb900b27a2f4 (bridge: allow creating bridge devices with netlink)
Reported-by: Jan Beulich <JBeulich@novell.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
---
net/bridge/br_if.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 2cdf007..e738154 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -231,6 +231,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
int br_add_bridge(struct net *net, const char *name)
{
struct net_device *dev;
+ int res;
dev = alloc_netdev(sizeof(struct net_bridge), name,
br_dev_setup);
@@ -240,7 +241,10 @@ int br_add_bridge(struct net *net, const char *name)
dev_net_set(dev, net);
- return register_netdev(dev);
+ res = register_netdev(dev);
+ if (res)
+ free_netdev(dev);
+ return res;
}
int br_del_bridge(struct net *net, const char *name)
^ permalink raw reply related
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3)
From: Ben Hutchings @ 2011-08-22 16:17 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev
In-Reply-To: <20110822002641.GA2550@neilslaptop.think-freely.org>
On Sun, 2011-08-21 at 20:27 -0400, Neil Horman wrote:
> On Wed, Aug 17, 2011 at 04:07:17PM +0100, Ben Hutchings wrote:
> > On Tue, 2011-07-26 at 12:05 -0400, Neil Horman wrote:
> > > Ok, after considering all your comments, Dave suggested this as an alternate
> > > approach:
> > >
> > > 1) We create a new priv_flag, IFF_SKB_TX_SHARED, to identify drivers capable of
> > > handling shared skbs. Default is to not set this flag
> > >
> > > 2) Modify ether_setup to enable this flag, under the assumption that any driver
> > > calling this function is initalizing a real ethernet device and as such can
> > > handle shared skbs since they don't tend to store state in the skb struct.
> > > Pktgen can then query this flag when a user script attempts to issue the
> > > clone_skb command and decide if it is to be alowed or not.
> > [...]
> >
> > A bunch of Ethernet drivers do skb_pad() or skb_padto() in their
> > ndo_start_xmit implementations, either to avoid hardware bugs or because
> > the MAC doesn't automatically pad to the minimum frame length. This
> > presumably means they can't generally handle shared skbs, though in the
> > specific case of pktgen it should be safe as long as a single skb is not
> > submitted by multiple threads at once.
> >
> Agreed, given that pktgen is doing skb sharing in a serialized manner (i.e. one
> thread of execution increasing skb->users rather than in multiple threads), the
> skb_pad[to] cases are safe. Are there cases in which shared skbs are
> transmitted in parallel threads that we need to check for?
Not that I'm aware of. However, you have defined the flag to mean 'safe
to send shared skbs', and this is not generally true for those drivers.
So please decide whether:
a. The flag means 'safe to send shared skbs' (i.e. the TX path does not
modify the skb) and drivers which pad must clear it on their devices.
b. The flag means 'safe to send shared skbs in the way ptkgen does', and
the restrictions this places on the user and driver should be specified.
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
* Re: [net-next 03/10] ixgbe: Drop the TX work limit and instead just leave it to budget
From: Alexander Duyck @ 2011-08-22 16:30 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Jeff Kirsher, davem, netdev, gospo
In-Reply-To: <1313935304.3142.22.camel@deadeye>
On 08/21/2011 07:01 AM, Ben Hutchings wrote:
> On Sun, 2011-08-21 at 00:29 -0700, Jeff Kirsher wrote:
>> From: Alexander Duyck<alexander.h.duyck@intel.com>
>>
>> This change makes it so that the TX work limit is now obsolete. Instead of
>> using it we can instead rely on the NAPI budget for the number of packets
>> we should clean per interrupt. The advantage to this approach is that it
>> results in a much more balanced work flow since the same number of RX and
>> TX packets should be cleaned per interrupts.
> [...]
>
> This seems kind of sensible, but it's not how Dave has been recommending
> people to account for TX work in NAPI.
>
> Ben.
>
I wasn't aware there was a recommended approach. Could you tell me more
about it?
As I stated in the patch description this approach works very well for
me, especially in routing workloads since it typically keeps the TX
clean-up in polling as long as the RX is in polling.
Thanks,
Alex
^ permalink raw reply
* Re: Miscalculated TCP ACK ?
From: Eric Dumazet @ 2011-08-22 16:38 UTC (permalink / raw)
To: Gabriel Beddingfield; +Cc: netdev
In-Reply-To: <CAPbw_hwQSXtmFSxK49jDDgF2LuG8=cu5t=ZjQ_aO7=M9fm9xeA@mail.gmail.com>
Le lundi 22 août 2011 à 10:34 -0500, Gabriel Beddingfield a écrit :
> I'm having trouble with a particular server via HTTPS. It appears
> that my local linux machines are sending incorrect ACK. However, I
> don't have enough expertise to know for sure.
>
> Using wireshark, the server sends:
>
> Transmission Control Protocol, Src Port: https (443), Dst Port:
> 36015 (36015), Seq: 27658, Ack: 827, Len: 18
>
> Local machine replies:
>
> Transmission Control Protocol, Src Port: 36015 (36015), Dst Port:
> https (443), Seq: 827, Ack: 27677, Len: 0
>
> It appears to me that the ACK is off-by-one (should have been 27676).
> The server replies again with retransmissions. This is with several
> kernel versions on local machines, ranging from 2.6.31 to 2.6.38.
>
> Can anyone shed insight into what's happening? This looks like a bug to me.
Not a bug if frame carried a FIN
18:32:16.177801 IP 10.150.51.213.52264 > 10.150.45.194.22: Flags [F.], seq 3312, ack 3241, win 412, length 0
18:32:16.178096 IP 10.150.45.194.22 > 10.150.51.213.52264: Flags [F.], seq 3241, ack 3313, win 203, length 0
18:32:16.256949 IP 10.150.51.213.52264 > 10.150.45.194.22: Flags [.], ack 3242, win 412, length 0
^ permalink raw reply
* Re: [RFC][PATCH] Failed neighbors attached to routes are not released
From: Eric Dumazet @ 2011-08-22 16:44 UTC (permalink / raw)
To: Guy Yur; +Cc: netdev
In-Reply-To: <CAC67Hz-MpmPGX4c+DTCgXkfnKdinNCrc7xc9Q03=35t=LLnW+g@mail.gmail.com>
Le vendredi 19 août 2011 à 21:53 +0300, Guy Yur a écrit :
> Hi,
>
> The issue I am seeing is with a neighbor used as a gateway in a route,
> when the neighbor gets nud FAILED it will not be removed from the
> neighbor cache.
> The reference count for the neighbor remains > 1
> when neigh_periodic_work() is called.
>
> Issue noticed with IPv6 neighbors on Arch Linux with kernel 3.0.3
> kernel config includes CONFIG_IPV6_ROUTER_PREF
>
> The problem affects routing when the neighbor loses connectivity and returns.
>
> Scenario: Using a default route and a static route through different interfaces.
> When the neighbor gateway of the static route goes down the traffic
> will move to the default gateway as expected.
> Once the static route neighbor comes back up it is not asked for
> neighbor solicitation
> because the route is marked as FAILED and the traffic will continue to
> pass through the default gateway.
>
> Steps to reproduce the route not being removed:
> 1. add an IPv6 address on an interface
> 2. add a route to a network through a gateway on the interface's network
> 3. make sure the gateway address is not reachable
> 4. ping6 a host in the route network
> 5. "ip -6 nei" will show the gateway neighbor as FAILED and it won't be released
>
> Steps to reproduce the routing problem:
> 1. client and two gateway machines (A and B)
> 2. on the client define a static route through A and a default route through B
> 3. disconnect eth on A
> 4. ping6 a host in the network that should go through A
> after a while the traffic will move through B which is the default route
> 5. reconnect eth on A
> 6. ping6 a host in the network again, the traffic will still go through B
> "ip -6 nei" on the client will still show A as FAILED
>
>
> Patch to change the nud state to NONE if the reference count > 1
> allowing the neighbor to be released in a future pass.
I wonder why a 'future pass' is needed at all.
Shouldnt we immediately detect link becomes alive and force an immediate
flush at this point, before waiting a garbage collect timer ?
>
> --- linux/net/core/neighbour.c.orig 2011-08-19 13:15:35.041524169 +0300
> +++ linux/net/core/neighbour.c 2011-08-19 18:19:07.271276096 +0300
> @@ -802,14 +802,16 @@ static void neigh_periodic_work(struct w
> if (time_before(n->used, n->confirmed))
> n->used = n->confirmed;
>
> - if (atomic_read(&n->refcnt) == 1 &&
> - (state == NUD_FAILED ||
> + if ((state == NUD_FAILED ||
> time_after(jiffies, n->used + n->parms->gc_staletime))) {
> - *np = n->next;
> - n->dead = 1;
> - write_unlock(&n->lock);
> - neigh_cleanup_and_release(n);
> - continue;
> + if (atomic_read(&n->refcnt) == 1) {
> + *np = n->next;
> + n->dead = 1;
> + write_unlock(&n->lock);
> + neigh_cleanup_and_release(n);
> + continue;
> + } else
> + n->nud_state = NUD_NONE;
> }
> write_unlock(&n->lock);
> --
^ permalink raw reply
* Re: [net-next 03/10] ixgbe: Drop the TX work limit and instead just leave it to budget
From: Ben Hutchings @ 2011-08-22 16:46 UTC (permalink / raw)
To: Alexander Duyck; +Cc: Jeff Kirsher, davem, netdev, gospo
In-Reply-To: <4E528437.5060302@intel.com>
On Mon, 2011-08-22 at 09:30 -0700, Alexander Duyck wrote:
> On 08/21/2011 07:01 AM, Ben Hutchings wrote:
> > On Sun, 2011-08-21 at 00:29 -0700, Jeff Kirsher wrote:
> >> From: Alexander Duyck<alexander.h.duyck@intel.com>
> >>
> >> This change makes it so that the TX work limit is now obsolete. Instead of
> >> using it we can instead rely on the NAPI budget for the number of packets
> >> we should clean per interrupt. The advantage to this approach is that it
> >> results in a much more balanced work flow since the same number of RX and
> >> TX packets should be cleaned per interrupts.
> > [...]
> >
> > This seems kind of sensible, but it's not how Dave has been recommending
> > people to account for TX work in NAPI.
> >
> > Ben.
> >
> I wasn't aware there was a recommended approach. Could you tell me more
> about it?
If a whole TX ring is cleaned then consider the budget spent; otherwise
don't count it.
Ben.
> As I stated in the patch description this approach works very well for
> me, especially in routing workloads since it typically keeps the TX
> clean-up in polling as long as the RX is in polling.
>
> Thanks,
>
> Alex
--
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
* Re: Miscalculated TCP ACK ?
From: Daniel Baluta @ 2011-08-22 17:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Gabriel Beddingfield, netdev
In-Reply-To: <1314031128.2307.110.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
>> It appears to me that the ACK is off-by-one (should have been 27676).
>> The server replies again with retransmissions. This is with several
>> kernel versions on local machines, ranging from 2.6.31 to 2.6.38.
> Not a bug if frame carried a FIN
Well then, why do retransmissions occur?
Gabriel could you post the full capture file?
thanks,
Daniel.
^ permalink raw reply
* RE: [PATCH net-next 3/5] be2net: fix erx->rx_drops_no_frags wrap around
From: Sathya.Perla @ 2011-08-22 17:22 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1314017087.2307.34.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>
OK, thanks Eric. I'll re-spin this patch series ...
-Sathya
________________________________________
From: Eric Dumazet [eric.dumazet@gmail.com]
Sent: Monday, August 22, 2011 6:14 PM
To: Perla, Sathya
Cc: netdev@vger.kernel.org
Subject: RE: [PATCH net-next 3/5] be2net: fix erx->rx_drops_no_frags wrap around
Le lundi 22 août 2011 à 05:15 -0700, Sathya.Perla@Emulex.Com a écrit :
> >-----Original Message-----
> >From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> >Sent: Monday, August 22, 2011 5:18 PM
> >
> >Le lundi 22 août 2011 à 13:43 +0200, Eric Dumazet a écrit :
> >
> >> This adds a race for lockless SNMP readers (in be_get_stats64())
> >>
> >> They can see the 32bit value going backward.
> >>
> >> You have to be very careful to read the *acc once, and write it once.
> >
> >The "write once" is the only requirement.
> >
> >Something like :
> >
> >u32 newval = hi(*acc) + val;
> >
> >if (wrapped)
> > newval += 65536;
> >ACCESS_ONCE(*acc) = newval;
> >
>
> Eric,
>
> I'm wondering why you'd need ACCESS_ONCE() here? Wouldn't using the temp variable (newval) as you suggested,
> ensure that the reader doesn't see a half-baked value?
>
If you write :
u32 newval = hi(*acc) + val;
if (wrapped)
newval += 65536;
*acc = newval;
C compiler (gcc) is free to eliminate the temp variable and to generate
same code than :
*acc = hi(*acc) + val;
if (wrapped)
*acc += 65536;
ACCESS_ONCE() here is the clean way (and self explanatory/documented) to
keep compiler to write to *acc twice.
^ permalink raw reply
* Re: [net-next 03/10] ixgbe: Drop the TX work limit and instead just leave it to budget
From: Alexander Duyck @ 2011-08-22 17:29 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Jeff Kirsher, davem, netdev, gospo
In-Reply-To: <1314031612.2803.7.camel@bwh-desktop>
On 08/22/2011 09:46 AM, Ben Hutchings wrote:
> On Mon, 2011-08-22 at 09:30 -0700, Alexander Duyck wrote:
>> On 08/21/2011 07:01 AM, Ben Hutchings wrote:
>>> On Sun, 2011-08-21 at 00:29 -0700, Jeff Kirsher wrote:
>>>> From: Alexander Duyck<alexander.h.duyck@intel.com>
>>>>
>>>> This change makes it so that the TX work limit is now obsolete. Instead of
>>>> using it we can instead rely on the NAPI budget for the number of packets
>>>> we should clean per interrupt. The advantage to this approach is that it
>>>> results in a much more balanced work flow since the same number of RX and
>>>> TX packets should be cleaned per interrupts.
>>> [...]
>>>
>>> This seems kind of sensible, but it's not how Dave has been recommending
>>> people to account for TX work in NAPI.
>>>
>>> Ben.
>>>
>> I wasn't aware there was a recommended approach. Could you tell me more
>> about it?
> If a whole TX ring is cleaned then consider the budget spent; otherwise
> don't count it.
>
> Ben.
The only problem I was seeing with that was that in certain cases it
seemed like the TX cleanup could consume enough CPU time to cause pretty
significant delays in processing the RX cleanup. This in turn was
causing single queue bi-directional routing tests to come out pretty
unbalanced since what seemed to happen is that one CPUs RX work would
overwhelm the other CPU with the TX processing resulting in an
unbalanced flow that was something like a 60/40 split between the
upstream and downstream throughput.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3)
From: Ben Hutchings @ 2011-08-22 17:33 UTC (permalink / raw)
To: Neil Horman; +Cc: netdev
In-Reply-To: <1314029857.2803.6.camel@bwh-desktop>
On Mon, 2011-08-22 at 17:17 +0100, Ben Hutchings wrote:
[...]
> b. The flag means 'safe to send shared skbs in the way ptkgen does', and
> the restrictions this places on the user and driver should be specified.
We could specify something like:
The ndo_start_xmit operation for this interface either does not
modify the given skb or modifies it idempotently. A single skb
may be transmitted repeatedly on a single queue of this
interface, but not on multiple queues or on multiple interfaces.
Does that look correct?
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
* Re: Miscalculated TCP ACK ?
From: Hagen Paul Pfeifer @ 2011-08-22 17:58 UTC (permalink / raw)
To: Gabriel Beddingfield; +Cc: netdev
In-Reply-To: <CAPbw_hwQSXtmFSxK49jDDgF2LuG8=cu5t=ZjQ_aO7=M9fm9xeA@mail.gmail.com>
* Gabriel Beddingfield | 2011-08-22 10:34:14 [-0500]:
>I'm having trouble with a particular server via HTTPS. It appears
>that my local linux machines are sending incorrect ACK. However, I
>don't have enough expertise to know for sure.
>
>Using wireshark, the server sends:
>
> Transmission Control Protocol, Src Port: https (443), Dst Port:
>36015 (36015), Seq: 27658, Ack: 827, Len: 18
>
>Local machine replies:
>
> Transmission Control Protocol, Src Port: 36015 (36015), Dst Port:
>https (443), Seq: 827, Ack: 27677, Len: 0
>
>It appears to me that the ACK is off-by-one (should have been 27676).
No, it is absolutely correct: ACK -> last seen Seq plus one (27658 + 18 + 1):
RFC 793:
If the ACK control bit is set this field contains the value of the
next sequence number the sender of the segment is expecting to
receive. Once a connection is established this is always sent.
Cheers, Hagen
^ permalink raw reply
* Re: Miscalculated TCP ACK ?
From: Gabriel Beddingfield @ 2011-08-22 18:05 UTC (permalink / raw)
To: netdev
In-Reply-To: <CAEnQRZAB=tKwr0wX2OrpG8iq5VaJPmssHEfC3U62bz1XL_2FHw@mail.gmail.com>
On Mon, Aug 22, 2011 at 12:19 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>>> It appears to me that the ACK is off-by-one (should have been 27676).
>>> The server replies again with retransmissions. This is with several
>>> kernel versions on local machines, ranging from 2.6.31 to 2.6.38.
>
>> Not a bug if frame carried a FIN
The server packet had the FIN bit set.
> Well then, why do retransmissions occur?
> Gabriel could you post the full capture file?
Sure: http://gabe.is-a-geek.org/tmp/20110822-skyward-conversation-segment.tcpdump
I clipped the conversation to the vicinity of the problem.
-gabriel
^ permalink raw reply
* Re: [PATCH 0/2] pktgen: Clone skb to avoid corruption of skbs in ndo_start_xmit methods (v3)
From: Neil Horman @ 2011-08-22 18:14 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev
In-Reply-To: <1314029857.2803.6.camel@bwh-desktop>
On Mon, Aug 22, 2011 at 05:17:37PM +0100, Ben Hutchings wrote:
> On Sun, 2011-08-21 at 20:27 -0400, Neil Horman wrote:
> > On Wed, Aug 17, 2011 at 04:07:17PM +0100, Ben Hutchings wrote:
> > > On Tue, 2011-07-26 at 12:05 -0400, Neil Horman wrote:
> > > > Ok, after considering all your comments, Dave suggested this as an alternate
> > > > approach:
> > > >
> > > > 1) We create a new priv_flag, IFF_SKB_TX_SHARED, to identify drivers capable of
> > > > handling shared skbs. Default is to not set this flag
> > > >
> > > > 2) Modify ether_setup to enable this flag, under the assumption that any driver
> > > > calling this function is initalizing a real ethernet device and as such can
> > > > handle shared skbs since they don't tend to store state in the skb struct.
> > > > Pktgen can then query this flag when a user script attempts to issue the
> > > > clone_skb command and decide if it is to be alowed or not.
> > > [...]
> > >
> > > A bunch of Ethernet drivers do skb_pad() or skb_padto() in their
> > > ndo_start_xmit implementations, either to avoid hardware bugs or because
> > > the MAC doesn't automatically pad to the minimum frame length. This
> > > presumably means they can't generally handle shared skbs, though in the
> > > specific case of pktgen it should be safe as long as a single skb is not
> > > submitted by multiple threads at once.
> > >
> > Agreed, given that pktgen is doing skb sharing in a serialized manner (i.e. one
> > thread of execution increasing skb->users rather than in multiple threads), the
> > skb_pad[to] cases are safe. Are there cases in which shared skbs are
> > transmitted in parallel threads that we need to check for?
>
> Not that I'm aware of. However, you have defined the flag to mean 'safe
> to send shared skbs', and this is not generally true for those drivers.
>
> So please decide whether:
> a. The flag means 'safe to send shared skbs' (i.e. the TX path does not
> modify the skb) and drivers which pad must clear it on their devices.
> b. The flag means 'safe to send shared skbs in the way ptkgen does', and
> the restrictions this places on the user and driver should be specified.
>
The flag means safe to send shared skbs.
Actually looking closer at this, I don't think this is much of a problem at all.
The flag is cleared on devices for which it is unsafe to send shared skbs, not
because there are multiple users of the skb in parallel, but because the driver
expects to have continued exclusive access to the skb after the drivers
ndo_start_xmit method returns.
In the pktgen case, skbs have skb->users > 1, but all users exist in the same
execution context, making their transmission serialized.
In the additional cases you are concerned with, multiple users of an skb may or
may not run in parallel. In the parallel case however, to avoid additional
races, one of the following must be true:
1) All n contexts treat the skb as immutible and wind up sending to the same
device and queue, at which point the xmit_lock in the net_device serializes
their operation.
2) Each of the n contexts (or a subset thereof) will modify the skb data, which
requires that they do their own locking between each other, which must encompass
the transmission of the skb in each context (lest they corrupt the skb during
transmission). Such locking again serializes the transmit peth.
In either case, access to the skb is serialized such that it is only ever
handled by one driver at one time, and during that window drivers may opt to
modify the state of the skb, as long as they dont' expect the sate to remain in
their control after the return from ndo_start_xmit.
So even though drivers that call skb_pad[to] modify the skb, its perfectly ok to
do so, as long as they don't assume that the struct skb will remain in that
state after the driver is done with it.
This works out perfectly well for skb_padto calls, since the function reduces to
a no-op after the minimal tail size has been reached.
Its a bit more problematic for skb_pad, since the possibility exists for
successive calls to skb_pad to iteratively increase the pad until the skb
tailroom is exhausted, causing a transmission error. That said however, there
are only 6 drivers that use skb_pad (sfx, niu, rt2800, rt61pci, rt37usb, and
claw), and they all only pad if the skb is not already a minimum length. These
calls should probably be audited and converted to skb_padto anyway.
So, in my view, these aren't really problems. If someone just does a blind skb
pad in the driver, then we have something of a problem, but one that is easily
fixed if and when the problem should arise.
Neil
^ permalink raw reply
* Re: [omega-g1:11072] Re: [PATCH] net: configurable sysctl parameter "net.core.tcp_lowat" for sk_stream_min_wspace()
From: David Miller @ 2011-08-22 18:35 UTC (permalink / raw)
To: jun.kondo
Cc: linux-kernel, omega-g1, notsuki, motokazu.kozaki, htaira, netdev,
tomohiko.takahashi, kotaro.sakai, ken.sugawara
In-Reply-To: <4E51A3F0.5010500@ctc-g.co.jp>
From: "Jun.Kondo" <jun.kondo@ctc-g.co.jp>
Date: Mon, 22 Aug 2011 09:33:52 +0900
> is it (really) network problem ?
> or is wmem not enough free to write?
Oh yes you can indeed make this determination, by using the socket
timeouts via the SO_RCVTIMEO and SO_SNDTIMEO socket options.
Timeouts, when hit, will return -EINTR, whereas lack of buffer space
on a non-blocking socket will return -EAGAIN.
I think you simply are unaware of the facilities available in the BSD
socket API.
^ permalink raw reply
* Re: Miscalculated TCP ACK ?
From: Eric Dumazet @ 2011-08-22 18:49 UTC (permalink / raw)
To: Gabriel Beddingfield; +Cc: netdev
In-Reply-To: <CAPbw_hy7xjhWSsiHnB9HN+ROCEUNzOS8aPsFwjvdY4FY+pRVjw@mail.gmail.com>
Le lundi 22 août 2011 à 13:05 -0500, Gabriel Beddingfield a écrit :
> On Mon, Aug 22, 2011 at 12:19 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >>> It appears to me that the ACK is off-by-one (should have been 27676).
> >>> The server replies again with retransmissions. This is with several
> >>> kernel versions on local machines, ranging from 2.6.31 to 2.6.38.
> >
> >> Not a bug if frame carried a FIN
>
> The server packet had the FIN bit set.
>
then its fine.
> > Well then, why do retransmissions occur?
> > Gabriel could you post the full capture file?
>
> Sure: http://gabe.is-a-geek.org/tmp/20110822-skyward-conversation-segment.tcpdump
>
> I clipped the conversation to the vicinity of the problem.
>
This sounds as a TSO bug, maybe driver/NIC related
What is the NIC/driver used on your server ?
try "ethtool -K eth0 tso off" on the server
$ tcpdump -p -n -S -r 20110822-skyward-conversation-segment.tcpdump
reading from file 20110822-skyward-conversation-segment.tcpdump, link-type EN10MB (Ethernet)
Sorry we miss a few frames just before.
14:48:31.368344 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], seq 401237709:401239157, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 1448
14:48:31.368410 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [.], ack 401239157, win 52128, options [nop,nop,TS val 3624365515 ecr 1663311], length 0
14:48:31.368556 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], seq 401239157:401240605, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 1448
14:48:31.369019 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], seq 401240605:401242053, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 1448
14:48:31.369099 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [.], ack 401242053, win 57920, options [nop,nop,TS val 3624365515 ecr 1663311], length 0
14:48:31.369357 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], seq 401242053:401243501, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 1448
14:48:31.369412 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [FP.], seq 401243501:401243519, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 18
following frames acknowledge all data sent from server and FIN flag.
14:48:31.369442 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [.], ack 401243520, win 57920, options [nop,nop,TS val 3624365516 ecr 1663311], length 0
Up to there, its OK.
But following frame makes no sense : this data was already acknowledged
14:48:31.373447 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], seq 401236261:401237709, ack 1404739303, win 64334, options [nop,nop,TS val 1663311 ecr 3624365496], length 1448
14:48:31.373532 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [.], ack 401243520, win 57920, options [nop,nop,TS val 3624365520 ecr 1663311,nop,nop,sack 1 {401236261:401237709}], length 0
client timeouts :
14:48:31.458943 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [F.], seq 1404739303, ack 401243520, win 57920, options [nop,nop,TS val 3624365605 ecr 1663311], length 0
14:48:31.475204 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], ack 1404739304, win 64334, options [nop,nop,TS val 1663321 ecr 3624365605], length 0
14:48:31.475351 IP 50.84.128.30.443 > 192.168.1.25.36015: Flags [.], ack 1404739304, win 0, length 0
14:48:31.475395 IP 192.168.1.25.36015 > 50.84.128.30.443: Flags [R], seq 1404739304, win 0, length 0
^ permalink raw reply
* pull request: wireless 2011-08-22
From: John W. Linville @ 2011-08-22 19:01 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
Dave,
This is a batch of fixes intended for 3.1. Included is rewrite of an
earlier iwlagn fix that was revealed to be flawed (bad pointer access
during module unload), a fix for a memory leak, and a trio of rt2x00
fixes with detailed changelogs. These have all been in linux-next
for the last week with no reported complaints.
Please let me know if there are problems!
John
---
The following changes since commit fbe5e29ec1886967255e76946aaf537b8cc9b81e:
atm: br2684: Fix oops due to skb->dev being NULL (2011-08-20 14:13:05 -0700)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git for-davem
Emmanuel Grumbach (2):
Revert "iwlagn: sysfs couldn't find the priv pointer"
iwlagn: sysfs couldn't find the priv pointer
John W. Linville (1):
Merge branch 'master' of git://git.kernel.org/.../linville/wireless into for-davem
Julia Lawall (1):
drivers/net/wireless/wl12xx: add missing kfree
Stanislaw Gruszka (3):
rt2x00: fix crash in rt2800usb_write_tx_desc
rt2x00: fix order of entry flags modification
rt2x00: fix crash in rt2800usb_get_txwi
drivers/net/wireless/iwlwifi/iwl-pci.c | 25 ++++++++++---------------
drivers/net/wireless/rt2x00/rt2800usb.c | 20 +++++++++++++++-----
drivers/net/wireless/rt2x00/rt2x00usb.c | 17 +++++++----------
drivers/net/wireless/wl12xx/acx.c | 6 +-----
drivers/net/wireless/wl12xx/testmode.c | 5 ++++-
5 files changed, 37 insertions(+), 36 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-pci.c b/drivers/net/wireless/iwlwifi/iwl-pci.c
index 69d4ec4..2fdbffa 100644
--- a/drivers/net/wireless/iwlwifi/iwl-pci.c
+++ b/drivers/net/wireless/iwlwifi/iwl-pci.c
@@ -478,27 +478,22 @@ out_no_pci:
return err;
}
-static void iwl_pci_down(struct iwl_bus *bus)
-{
- struct iwl_pci_bus *pci_bus = (struct iwl_pci_bus *) bus->bus_specific;
-
- pci_disable_msi(pci_bus->pci_dev);
- pci_iounmap(pci_bus->pci_dev, pci_bus->hw_base);
- pci_release_regions(pci_bus->pci_dev);
- pci_disable_device(pci_bus->pci_dev);
- pci_set_drvdata(pci_bus->pci_dev, NULL);
-
- kfree(bus);
-}
-
static void __devexit iwl_pci_remove(struct pci_dev *pdev)
{
struct iwl_priv *priv = pci_get_drvdata(pdev);
- void *bus_specific = priv->bus->bus_specific;
+ struct iwl_bus *bus = priv->bus;
+ struct iwl_pci_bus *pci_bus = IWL_BUS_GET_PCI_BUS(bus);
+ struct pci_dev *pci_dev = IWL_BUS_GET_PCI_DEV(bus);
iwl_remove(priv);
- iwl_pci_down(bus_specific);
+ pci_disable_msi(pci_dev);
+ pci_iounmap(pci_dev, pci_bus->hw_base);
+ pci_release_regions(pci_dev);
+ pci_disable_device(pci_dev);
+ pci_set_drvdata(pci_dev, NULL);
+
+ kfree(bus);
}
#ifdef CONFIG_PM
diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
index 9395631..dbf501c 100644
--- a/drivers/net/wireless/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/rt2x00/rt2800usb.c
@@ -464,6 +464,15 @@ static bool rt2800usb_txdone_entry_check(struct queue_entry *entry, u32 reg)
int wcid, ack, pid;
int tx_wcid, tx_ack, tx_pid;
+ if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
+ !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags)) {
+ WARNING(entry->queue->rt2x00dev,
+ "Data pending for entry %u in queue %u\n",
+ entry->entry_idx, entry->queue->qid);
+ cond_resched();
+ return false;
+ }
+
wcid = rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
ack = rt2x00_get_field32(reg, TX_STA_FIFO_TX_ACK_REQUIRED);
pid = rt2x00_get_field32(reg, TX_STA_FIFO_PID_TYPE);
@@ -529,12 +538,11 @@ static void rt2800usb_txdone(struct rt2x00_dev *rt2x00dev)
entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
if (rt2800usb_txdone_entry_check(entry, reg))
break;
+ entry = NULL;
}
- if (!entry || rt2x00queue_empty(queue))
- break;
-
- rt2800_txdone_entry(entry, reg);
+ if (entry)
+ rt2800_txdone_entry(entry, reg);
}
}
@@ -558,8 +566,10 @@ static void rt2800usb_work_txdone(struct work_struct *work)
while (!rt2x00queue_empty(queue)) {
entry = rt2x00queue_get_entry(queue, Q_INDEX_DONE);
- if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
+ if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
+ !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
break;
+
if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags))
rt2x00lib_txdone_noinfo(entry, TXDONE_FAILURE);
else if (rt2x00queue_status_timeout(entry))
diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c
index b6b4542..7fbb55c 100644
--- a/drivers/net/wireless/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/rt2x00/rt2x00usb.c
@@ -262,23 +262,20 @@ static void rt2x00usb_interrupt_txdone(struct urb *urb)
struct queue_entry *entry = (struct queue_entry *)urb->context;
struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;
- if (!test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
+ if (!test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
return;
-
- if (rt2x00dev->ops->lib->tx_dma_done)
- rt2x00dev->ops->lib->tx_dma_done(entry);
-
- /*
- * Report the frame as DMA done
- */
- rt2x00lib_dmadone(entry);
-
/*
* Check if the frame was correctly uploaded
*/
if (urb->status)
set_bit(ENTRY_DATA_IO_FAILED, &entry->flags);
+ /*
+ * Report the frame as DMA done
+ */
+ rt2x00lib_dmadone(entry);
+ if (rt2x00dev->ops->lib->tx_dma_done)
+ rt2x00dev->ops->lib->tx_dma_done(entry);
/*
* Schedule the delayed work for reading the TX status
* from the device.
diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
index 7e33f1f..34f6ab5 100644
--- a/drivers/net/wireless/wl12xx/acx.c
+++ b/drivers/net/wireless/wl12xx/acx.c
@@ -77,8 +77,6 @@ int wl1271_acx_sleep_auth(struct wl1271 *wl, u8 sleep_auth)
auth->sleep_auth = sleep_auth;
ret = wl1271_cmd_configure(wl, ACX_SLEEP_AUTH, auth, sizeof(*auth));
- if (ret < 0)
- return ret;
out:
kfree(auth);
@@ -624,10 +622,8 @@ int wl1271_acx_cca_threshold(struct wl1271 *wl)
ret = wl1271_cmd_configure(wl, ACX_CCA_THRESHOLD,
detection, sizeof(*detection));
- if (ret < 0) {
+ if (ret < 0)
wl1271_warning("failed to set cca threshold: %d", ret);
- return ret;
- }
out:
kfree(detection);
diff --git a/drivers/net/wireless/wl12xx/testmode.c b/drivers/net/wireless/wl12xx/testmode.c
index 5d5e1ef..88add68 100644
--- a/drivers/net/wireless/wl12xx/testmode.c
+++ b/drivers/net/wireless/wl12xx/testmode.c
@@ -139,12 +139,15 @@ static int wl1271_tm_cmd_interrogate(struct wl1271 *wl, struct nlattr *tb[])
if (ret < 0) {
wl1271_warning("testmode cmd interrogate failed: %d", ret);
+ kfree(cmd);
return ret;
}
skb = cfg80211_testmode_alloc_reply_skb(wl->hw->wiphy, sizeof(*cmd));
- if (!skb)
+ if (!skb) {
+ kfree(cmd);
return -ENOMEM;
+ }
NLA_PUT(skb, WL1271_TM_ATTR_DATA, sizeof(*cmd), cmd);
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply related
* RE: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
From: Bing Zhao @ 2011-08-22 19:02 UTC (permalink / raw)
To: Julia Lawall, Pierre Louis Aublin
Cc: kernel-janitors@vger.kernel.org, John W. Linville,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <Pine.LNX.4.64.1108221615450.10948@ask.diku.dk>
Hi Julia,
Thanks for the patch.
> -----Original Message-----
> From: Julia Lawall [mailto:julia@diku.dk]
> Sent: Monday, August 22, 2011 7:16 AM
> To: Pierre Louis Aublin
> Cc: Bing Zhao; kernel-janitors@vger.kernel.org; John W. Linville; linux-wireless@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
>
> From: Julia Lawall <julia@diku.dk>
>
> Test the just-initialized value rather than some other one.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> statement S;
> @@
>
> x = f(...);
> (
> if (\(x == NULL\|IS_ERR(x)\)) S
> |
> *if (\(y == NULL\|IS_ERR(y)\))
> { ... when != x
> return ...; }
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Acked-by: Bing Zhao <bzhao@marvell.com>
Thanks,
Bing
>
> ---
> drivers/net/wireless/mwifiex/scan.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> index b28241c..37ca2f9 100644
> --- a/drivers/net/wireless/mwifiex/scan.c
> +++ b/drivers/net/wireless/mwifiex/scan.c
> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
> return -ENOMEM;
> }
> beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> - if (!bss_desc) {
> - dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> + if (!beacon_ie) {
> + dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
> return -ENOMEM;
> }
> memcpy(beacon_ie, ie_buf, ie_len);
^ permalink raw reply
* Re: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
From: walter harms @ 2011-08-22 19:09 UTC (permalink / raw)
To: Bing Zhao
Cc: Julia Lawall, Pierre Louis Aublin,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John W. Linville,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <477F20668A386D41ADCC57781B1F70430802DD7B7E-r8ILAu4/owuHXkj8w7BxOhL4W9x8LtSr@public.gmane.org>
Am 22.08.2011 21:02, schrieb Bing Zhao:
> Hi Julia,
>
> Thanks for the patch.
>
>> -----Original Message-----
>> From: Julia Lawall [mailto:julia-dAYI7NvHqcQ@public.gmane.org]
>> Sent: Monday, August 22, 2011 7:16 AM
>> To: Pierre Louis Aublin
>> Cc: Bing Zhao; kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; John W. Linville; linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
>>
>> From: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
>>
>> Test the just-initialized value rather than some other one.
>>
>> The semantic match that finds this problem is as follows:
>> (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @r@
>> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
>> statement S;
>> @@
>>
>> x = f(...);
>> (
>> if (\(x == NULL\|IS_ERR(x)\)) S
>> |
>> *if (\(y == NULL\|IS_ERR(y)\))
>> { ... when != x
>> return ...; }
>> )
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
>
> Acked-by: Bing Zhao <bzhao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>
>
> Thanks,
> Bing
>
>>
>> ---
>> drivers/net/wireless/mwifiex/scan.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
>> index b28241c..37ca2f9 100644
>> --- a/drivers/net/wireless/mwifiex/scan.c
>> +++ b/drivers/net/wireless/mwifiex/scan.c
>> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
>> return -ENOMEM;
>> }
>> beacon_ie = kzalloc(ie_len, GFP_KERNEL);
>> - if (!bss_desc) {
>> - dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
>> + if (!beacon_ie) {
>> + dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
>> return -ENOMEM;
>> }
>> memcpy(beacon_ie, ie_buf, ie_len);
> --
this looks like a case for kmemdup()
just my 2 cents,
re,
wh
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [patch net-next-2.6] net: vlan: goto another_round instead of calling __netif_receive_skb
From: Jiri Pirko @ 2011-08-22 19:08 UTC (permalink / raw)
To: netdev; +Cc: davem, shemminger, eric.dumazet, mirqus, jesse
Now, when vlan tag on untagged in non-accelerated path is stripped from
skb, headers are reset right away. Benefit from that and avoid calling
__netif_receive_skb recursivelly and just use another_round.
Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
net/core/dev.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index c2442b4..a4306f7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3236,10 +3236,9 @@ ncls:
ret = deliver_skb(skb, pt_prev, orig_dev);
pt_prev = NULL;
}
- if (vlan_do_receive(&skb)) {
- ret = __netif_receive_skb(skb);
- goto out;
- } else if (unlikely(!skb))
+ if (vlan_do_receive(&skb))
+ goto another_round;
+ else if (unlikely(!skb))
goto out;
}
--
1.7.6
^ permalink raw reply related
* RE: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
From: Bing Zhao @ 2011-08-22 19:29 UTC (permalink / raw)
To: wharms-fPG8STNUNVg@public.gmane.org
Cc: Julia Lawall, Pierre Louis Aublin,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
John W. Linville,
linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <4E52A966.3090502-fPG8STNUNVg@public.gmane.org>
> Am 22.08.2011 21:02, schrieb Bing Zhao:
> > Hi Julia,
> >
> > Thanks for the patch.
> >
> >> -----Original Message-----
> >> From: Julia Lawall [mailto:julia-dAYI7NvHqcQ@public.gmane.org]
> >> Sent: Monday, August 22, 2011 7:16 AM
> >> To: Pierre Louis Aublin
> >> Cc: Bing Zhao; kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; John W. Linville; linux-wireless-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> >> netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: [PATCH 4/4] drivers/net/wireless/mwifiex/scan.c: test the just-initialized value
> >>
> >> From: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
> >>
> >> Test the just-initialized value rather than some other one.
> >>
> >> The semantic match that finds this problem is as follows:
> >> (http://coccinelle.lip6.fr/)
> >>
> >> // <smpl>
> >> @r@
> >> identifier x,y,f!={PTR_ERR,ERR_PTR,ERR_CAST};
> >> statement S;
> >> @@
> >>
> >> x = f(...);
> >> (
> >> if (\(x == NULL\|IS_ERR(x)\)) S
> >> |
> >> *if (\(y == NULL\|IS_ERR(y)\))
> >> { ... when != x
> >> return ...; }
> >> )
> >> // </smpl>
> >>
> >> Signed-off-by: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
> >
> > Acked-by: Bing Zhao <bzhao-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> >
> >
> > Thanks,
> > Bing
> >
> >>
> >> ---
> >> drivers/net/wireless/mwifiex/scan.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> >> index b28241c..37ca2f9 100644
> >> --- a/drivers/net/wireless/mwifiex/scan.c
> >> +++ b/drivers/net/wireless/mwifiex/scan.c
> >> @@ -1480,8 +1480,8 @@ mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
> >> return -ENOMEM;
> >> }
> >> beacon_ie = kzalloc(ie_len, GFP_KERNEL);
> >> - if (!bss_desc) {
> >> - dev_err(priv->adapter->dev, " failed to alloc bss_desc\n");
> >> + if (!beacon_ie) {
> >> + dev_err(priv->adapter->dev, " failed to alloc beacon_ie\n");
> >> return -ENOMEM;
> >> }
> >> memcpy(beacon_ie, ie_buf, ie_len);
> > --
>
>
> this looks like a case for kmemdup()
Hi Walter,
You are right. I believe there are some more cases where kmemdup can be used.
Hi John,
Please apply Julia's patch as I will send a separate patch later to address Walter's comment. Thanks.
>
> just my 2 cents,
Thanks a lot for your 2 cents.
Bing
>
> re,
> wh
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: pull request: wireless 2011-08-22
From: David Miller @ 2011-08-22 19:33 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20110822190136.GJ2534@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Mon, 22 Aug 2011 15:01:37 -0400
> This is a batch of fixes intended for 3.1. Included is rewrite of an
> earlier iwlagn fix that was revealed to be flawed (bad pointer access
> during module unload), a fix for a memory leak, and a trio of rt2x00
> fixes with detailed changelogs. These have all been in linux-next
> for the last week with no reported complaints.
>
> Please let me know if there are problems!
Pulled, thanks John.
^ 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