* Re: [PATCH] cnic: Fix ISCSI_KEVENT_IF_DOWN message handling.
From: Mike Christie @ 2009-07-29 8:17 UTC (permalink / raw)
To: Michael Chan
Cc: davem@davemloft.net, James.Bottomley@HansenPartnership.com,
netdev@vger.kernel.org, linux-scsi@vger.kernel.org,
open-iscsi@googlegroups.com
In-Reply-To: <C27F8246C663564A84BB7AB343977242178EE780E1@IRVEXCHCCR01.corp.ad.broadcom.com>
Michael Chan wrote:
> Mike Christie wrote:
>
>> Michael Chan wrote:
>>> When a net device goes down or when the bnx2i driver is unloaded,
>>> the code was not generating the ISCSI_KEVENT_IF_DOWN message
>>> properly and this could cause the userspace driver to crash.
>>>
>>> This is fixed by sending the message properly in the shutdown path.
>>> cnic_uio_stop() is also added to send the message when bnx2i is
>>> unregistering.
>>>
>>> Signed-off-by: Michael Chan <mchan@broadcom.com>
>>> ---
>>> drivers/net/cnic.c | 23 +++++++++++++++++++++--
>>> 1 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
>>> index 4d1515f..4869d77 100644
>>> --- a/drivers/net/cnic.c
>>> +++ b/drivers/net/cnic.c
>>> @@ -227,7 +227,7 @@ static int cnic_send_nlmsg(struct
>> cnic_local *cp, u32 type,
>>> }
>>>
>>> rcu_read_lock();
>>> - ulp_ops = rcu_dereference(cp->ulp_ops[CNIC_ULP_ISCSI]);
>>> + ulp_ops = rcu_dereference(cnic_ulp_tbl[CNIC_ULP_ISCSI]);
>>> if (ulp_ops)
>>> ulp_ops->iscsi_nl_send_msg(cp->dev, msg_type, buf, len);
>>> rcu_read_unlock();
>>> @@ -319,6 +319,20 @@ static int cnic_abort_prep(struct
>> cnic_sock *csk)
>>> return 0;
>>> }
>>>
>>> +static void cnic_uio_stop(void)
>>> +{
>>> + struct cnic_dev *dev;
>>> +
>>> + read_lock(&cnic_dev_lock);
>>> + list_for_each_entry(dev, &cnic_dev_list, list) {
>>> + struct cnic_local *cp = dev->cnic_priv;
>>> +
>>> + if (cp->cnic_uinfo)
>>> + cnic_send_nlmsg(cp, ISCSI_KEVENT_IF_DOWN, NULL);
>> I don't think you can call this with the cnic_dev_lock held.
>> They have
>> the same sleeping restrictions as a spin_lock right? If so,
>> the problem
>> is that iscsi_nl_send_ms calls iscsi_offload_mesg which uses GFP_NOIO
>> and can sleep.
>>
>>
> In that case, can I send in a patch to change iscsi_offload_mesg() to
> use GFP_ATOMIC?
>
Yes, I guess so.
^ permalink raw reply
* Re: [PATCH] cnic: Fix ISCSI_KEVENT_IF_DOWN message handling.
From: Michael Chan @ 2009-07-29 7:55 UTC (permalink / raw)
To: 'Mike Christie'
Cc: davem@davemloft.net, James.Bottomley@HansenPartnership.com,
netdev@vger.kernel.org, linux-scsi@vger.kernel.org,
open-iscsi@googlegroups.com
In-Reply-To: <4A6FFF47.1040107@cs.wisc.edu>
Mike Christie wrote:
> Michael Chan wrote:
> > When a net device goes down or when the bnx2i driver is unloaded,
> > the code was not generating the ISCSI_KEVENT_IF_DOWN message
> > properly and this could cause the userspace driver to crash.
> >
> > This is fixed by sending the message properly in the shutdown path.
> > cnic_uio_stop() is also added to send the message when bnx2i is
> > unregistering.
> >
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > ---
> > drivers/net/cnic.c | 23 +++++++++++++++++++++--
> > 1 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
> > index 4d1515f..4869d77 100644
> > --- a/drivers/net/cnic.c
> > +++ b/drivers/net/cnic.c
> > @@ -227,7 +227,7 @@ static int cnic_send_nlmsg(struct
> cnic_local *cp, u32 type,
> > }
> >
> > rcu_read_lock();
> > - ulp_ops = rcu_dereference(cp->ulp_ops[CNIC_ULP_ISCSI]);
> > + ulp_ops = rcu_dereference(cnic_ulp_tbl[CNIC_ULP_ISCSI]);
> > if (ulp_ops)
> > ulp_ops->iscsi_nl_send_msg(cp->dev, msg_type, buf, len);
> > rcu_read_unlock();
> > @@ -319,6 +319,20 @@ static int cnic_abort_prep(struct
> cnic_sock *csk)
> > return 0;
> > }
> >
> > +static void cnic_uio_stop(void)
> > +{
> > + struct cnic_dev *dev;
> > +
> > + read_lock(&cnic_dev_lock);
> > + list_for_each_entry(dev, &cnic_dev_list, list) {
> > + struct cnic_local *cp = dev->cnic_priv;
> > +
> > + if (cp->cnic_uinfo)
> > + cnic_send_nlmsg(cp, ISCSI_KEVENT_IF_DOWN, NULL);
>
> I don't think you can call this with the cnic_dev_lock held.
> They have
> the same sleeping restrictions as a spin_lock right? If so,
> the problem
> is that iscsi_nl_send_ms calls iscsi_offload_mesg which uses GFP_NOIO
> and can sleep.
>
>
In that case, can I send in a patch to change iscsi_offload_mesg() to
use GFP_ATOMIC?
^ permalink raw reply
* Re: [PATCH 1/3] au1000_eth: use write accessors
From: Manuel Lauss @ 2009-07-29 7:51 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev
In-Reply-To: <200907282306.27738.florian@openwrt.org>
Hi Florian,
On Tue, Jul 28, 2009 at 11:06 PM, Florian Fainelli<florian@openwrt.org> wrote:
> Hi David,
>
> The next 3 patches apply to on top of the patch series that I
> sent earlier converting au1000_eth to become a platform_driver.
> Thanks !
> --
> From: Florian Fainelli <florian@openwrt.org>
> This patch replaces all occurences of *aup->enable = val
> by writel(val, &aup->enable)
>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
> diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
> index 57cc4f6..1f724da 100644
> --- a/drivers/net/au1000_eth.c
> +++ b/drivers/net/au1000_eth.c
> @@ -155,10 +155,10 @@ static void enable_mac(struct net_device *dev, int force_reset)
> spin_lock_irqsave(&aup->lock, flags);
>
> if(force_reset || (!aup->mac_enabled)) {
> - *aup->enable = MAC_EN_CLOCK_ENABLE;
> + writel(MAC_EN_CLOCK_ENABLE, &aup->enable);
> au_sync_delay(2);
> - *aup->enable = (MAC_EN_RESET0 | MAC_EN_RESET1 | MAC_EN_RESET2
> - | MAC_EN_CLOCK_ENABLE);
> + writel((MAC_EN_RESET0 | MAC_EN_RESET1 | MAC_EN_RESET2
> + | MAC_EN_CLOCK_ENABLE), &aup->enable);
> au_sync_delay(2);
While you're at it, please also replace all "au_sync_delay()" with
mmiowb() + msleep()
Thanks!
Manuel Lauss
^ permalink raw reply
* Re: [PATCH] cnic: Fix ISCSI_KEVENT_IF_DOWN message handling.
From: Mike Christie @ 2009-07-29 7:50 UTC (permalink / raw)
To: Michael Chan; +Cc: davem, James.Bottomley, netdev, linux-scsi, open-iscsi
In-Reply-To: <1248318115-14214-1-git-send-email-mchan@broadcom.com>
Michael Chan wrote:
> When a net device goes down or when the bnx2i driver is unloaded,
> the code was not generating the ISCSI_KEVENT_IF_DOWN message
> properly and this could cause the userspace driver to crash.
>
> This is fixed by sending the message properly in the shutdown path.
> cnic_uio_stop() is also added to send the message when bnx2i is
> unregistering.
>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
> drivers/net/cnic.c | 23 +++++++++++++++++++++--
> 1 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/cnic.c b/drivers/net/cnic.c
> index 4d1515f..4869d77 100644
> --- a/drivers/net/cnic.c
> +++ b/drivers/net/cnic.c
> @@ -227,7 +227,7 @@ static int cnic_send_nlmsg(struct cnic_local *cp, u32 type,
> }
>
> rcu_read_lock();
> - ulp_ops = rcu_dereference(cp->ulp_ops[CNIC_ULP_ISCSI]);
> + ulp_ops = rcu_dereference(cnic_ulp_tbl[CNIC_ULP_ISCSI]);
> if (ulp_ops)
> ulp_ops->iscsi_nl_send_msg(cp->dev, msg_type, buf, len);
> rcu_read_unlock();
> @@ -319,6 +319,20 @@ static int cnic_abort_prep(struct cnic_sock *csk)
> return 0;
> }
>
> +static void cnic_uio_stop(void)
> +{
> + struct cnic_dev *dev;
> +
> + read_lock(&cnic_dev_lock);
> + list_for_each_entry(dev, &cnic_dev_list, list) {
> + struct cnic_local *cp = dev->cnic_priv;
> +
> + if (cp->cnic_uinfo)
> + cnic_send_nlmsg(cp, ISCSI_KEVENT_IF_DOWN, NULL);
I don't think you can call this with the cnic_dev_lock held. They have
the same sleeping restrictions as a spin_lock right? If so, the problem
is that iscsi_nl_send_ms calls iscsi_offload_mesg which uses GFP_NOIO
and can sleep.
^ permalink raw reply
* netpoll + xmit_lock == deadlock
From: Herbert Xu @ 2009-07-29 7:35 UTC (permalink / raw)
To: David S. Miller, Matt Mackall, netdev; +Cc: Matt Carlson
Hi:
While working on TX mitigiation, I noticed that while netpoll
takes care to avoid recursive dead locks on the NAPI path, it
has no protection against the TX path when calling the poll
function.
So if a driver is in the TX path, and a printk occurs, then a
recursive dead lock can occur if that driver tries to take the
xmit lock in its poll function to clean up descriptors.
Fortunately not a lot of drivers do this but at least some are
vulnerable to it, e.g., tg3.
So we need to make it very clear that the poll function must
not take any locks or they must use try_lock if the driver is
to support netpoll.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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
* Re: [PATCH] pppoe: fix race at init time
From: Eric Dumazet @ 2009-07-29 4:33 UTC (permalink / raw)
To: Igor M Podlesny
Cc: Cyrill Gorcunov, David S. Miller, Pavel Emelyanov, Andrew Morton,
netdev
In-Reply-To: <43d009740907282055t44bfa2e2md3afa514d4b2d7e6@mail.gmail.com>
Igor M Podlesny a écrit :
> 2009/7/29 Cyrill Gorcunov <gorcunov@gmail.com>:
>> [Eric Dumazet - Tue, Jul 28, 2009 at 07:46:37PM +0200]
>> ...
>> | So Igor still has a panic... lets try a third patch then :)
> [...]
>> Hmm. It seems the problem is not in pppoe_init_net since it's
>> called *before* dev_add_pack via register_pernet_gen_device
>> (which is protected by a global net mutex). Or I miss something?
>>
>> (sorry guys I have quite a limited internet connection this week)
>
> At last the 3rd patch was also unable to fix the bug.
>
Yes, Cyril is right, my patch was not necessary.
Any chance you give us CR2 value of fault ?
Do you still have a corruption by a 2 value somewhere ?
(High 32 bits order in first jpeg you sent)
00000002.3dcc5244 instead of ffffXXXX.3dcc5244
^ permalink raw reply
* Re: [PATCH] pppoe: fix race at init time
From: Igor M Podlesny @ 2009-07-29 3:55 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Eric Dumazet, David S. Miller, Pavel Emelyanov, Andrew Morton,
netdev
In-Reply-To: <20090728184834.GD4678@lenovo>
2009/7/29 Cyrill Gorcunov <gorcunov@gmail.com>:
> [Eric Dumazet - Tue, Jul 28, 2009 at 07:46:37PM +0200]
> ...
> | So Igor still has a panic... lets try a third patch then :)
[...]
> Hmm. It seems the problem is not in pppoe_init_net since it's
> called *before* dev_add_pack via register_pernet_gen_device
> (which is protected by a global net mutex). Or I miss something?
>
> (sorry guys I have quite a limited internet connection this week)
At last the 3rd patch was also unable to fix the bug.
--
End of message. Next message?
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-29 3:15 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090729023809.GA2218@gondor.apana.org.au>
On Wed, Jul 29, 2009 at 10:38:09AM +0800, Herbert Xu wrote:
>
> I did say it wasn't a good answer :)
Here's another crazy idea:
Forget about using multiple hardware TX queues with rate limiting
qdiscs. Why? Because these qdiscs are fundamentally serialising
so once the traffic has gone through it there is nothing to be
gained from multiplexing them into the hardware only to be merged
again onto the wire right away.
So does this mean that we give up on CPU scalability on limiting
qdiscs? No, the bottleneck is in fact in the qdisc, not in the
NIC. So we implement multiqueue in the qdisc instead.
This can be done with a lockless ring buffer. Each CPU would
deposit its packets in a qdisc ringer buffer assigned to it.
The qdisc would then run on a single CPU harvesting all the
ring buffers in some fair manner.
Essentially we can treat rate limiting qdiscs as NICs. Another
way of thinking about this is that we're essentially decoupling
a single system into two. One that is using a default qdisc to
achieve maximum throughput. This is then fed into a second system
that is dedicated to rate limiting.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-29 2:38 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090728.192247.247655474.davem@davemloft.net>
On Tue, Jul 28, 2009 at 07:22:47PM -0700, David Miller wrote:
>
> Of course, you can solve the issue by redefining problem. :-)
>
> However, such a semantic is not what users of this feature
> are after. I doubt there would be much uptake of it even
> if we did implement it.
I did say it wasn't a good answer :)
My main point is that whatever we do for the limiting qdiscs
should not penalise the non-limiting qdiscs because for the
moment people are trying to get more out of their NICs rather
than less :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: David Miller @ 2009-07-29 2:22 UTC (permalink / raw)
To: herbert; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090729021212.GA1946@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Jul 2009 10:12:12 +0800
> One possibility is to partition the bandwidth equally between
> the queues and apply rate limiting locally within each queue.
Of course, you can solve the issue by redefining problem. :-)
However, such a semantic is not what users of this feature
are after. I doubt there would be much uptake of it even
if we did implement it.
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-29 2:12 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090728.183451.93398827.davem@davemloft.net>
On Tue, Jul 28, 2009 at 06:34:51PM -0700, David Miller wrote:
>
> Wouldn't that bypass any rate limiting enforcement done by
> the qdisc too?
You'd only bypass it for the default qdisc (or any other qdisc
that didn't care). It can either be achived through a flag as
Krishna proposed or perhaps we can make the qdisc's enqueue
function return a special value that indicates the packet can
be dequeued immediately and given to the hardware.
I don't have a good answer for how we can achieve CPU scalability
for rate limiting qdiscs yet. However, that shouldn't stop us
from getting better CPU scalability for the default qdisc which
is what most people will use for the time being.
One possibility is to partition the bandwidth equally between
the queues and apply rate limiting locally within each queue.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Serge E. Hallyn @ 2009-07-29 1:56 UTC (permalink / raw)
To: Dan Smith; +Cc: Oren Laadan, containers, netdev, Alexey Dobriyan
In-Reply-To: <87ljm8czsf.fsf@caffeine.danplanet.com>
Quoting Dan Smith (danms@us.ibm.com):
> OL> Unless/until we decide otherwise, let's keep all ckpt_hdr_xxx
> OL> definitions in a single place -- checkpoint_hdr.h
>
> That's how I initially had it, but Serge asked for it to be moved into
> the network headers. Serge?
Yes, it seems to me we want to keep those close to the maintainer's
code rather than expect them to go browsing through cr headers when
they change their own code.
Of course just putting ckpt_hdr_xxx into the subsystems' headers
isn't sufficient - we need to have better discussions with the
maintainers about ways to make maintaining their subsystems with
cr pretty much the same as it was without cr.
-serge
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: David Miller @ 2009-07-29 1:34 UTC (permalink / raw)
To: herbert; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090729012523.GA1583@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Jul 2009 09:25:23 +0800
> On Tue, Jul 28, 2009 at 06:06:47PM -0700, David Miller wrote:
>>
>> More interesting to me is the case where the queue is not
>> filled up, or is very nearly so. :-)
>
> Do you mean the hardware queue? In that case perhaps Krishna's
> proposal of bypassing the qdisc would be the best.
Wouldn't that bypass any rate limiting enforcement done by
the qdisc too?
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-29 1:25 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090728.180647.241258705.davem@davemloft.net>
On Tue, Jul 28, 2009 at 06:06:47PM -0700, David Miller wrote:
>
> More interesting to me is the case where the queue is not
> filled up, or is very nearly so. :-)
Do you mean the hardware queue? In that case perhaps Krishna's
proposal of bypassing the qdisc would be the best.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: David Miller @ 2009-07-29 1:06 UTC (permalink / raw)
To: herbert; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090729004428.GA765@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 29 Jul 2009 08:44:28 +0800
> Suppose that we have a single large flow going through that has
> filled up the hardware queue and is now backlogged in the qdisc
> with qdisc_run on CPU A. Now some other flow comes along and
> sends a packet on CPU B.
>
> So now CPU A and B will both be processing packets for the first
> flow causing loads of lock contention.
>
> But worse yet, we have introduced packet reordering. So are you
> convinced now :)
More interesting to me is the case where the queue is not
filled up, or is very nearly so. :-)
But yes I do see your point.
^ permalink raw reply
* Re: [RFC] [PATCH] Don't run __qdisc_run() on a stopped TX queue
From: Herbert Xu @ 2009-07-29 0:44 UTC (permalink / raw)
To: David Miller; +Cc: krkumar2, jarkao2, netdev
In-Reply-To: <20090728.125919.146001472.davem@davemloft.net>
On Tue, Jul 28, 2009 at 12:59:19PM -0700, David Miller wrote:
>
> The premise is that there'd be only one. The qdisc lock.
>
> If the traffic is distributed, flow wise, the driver XMIT
> lock would spread due to multiqueue.
Suppose that we have a single large flow going through that has
filled up the hardware queue and is now backlogged in the qdisc
with qdisc_run on CPU A. Now some other flow comes along and
sends a packet on CPU B.
So now CPU A and B will both be processing packets for the first
flow causing loads of lock contention.
But worse yet, we have introduced packet reordering. So are you
convinced now :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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] xfrm: select sane defaults for xfrm[4|6] gc_thresh
From: Neil Horman @ 2009-07-29 0:34 UTC (permalink / raw)
To: netdev; +Cc: davem, kuznet, pekkas, jmorris, yoshfuji, kaber, nhorman
Choose saner defaults for xfrm[4|6] gc_thresh values on init
Currently, the xfrm[4|6] code has hard-coded initial gc_thresh values (set to
1024). Given that the ipv4 and ipv6 routing caches are sized dynamically at
boot time, the static selections can be non-sensical. This patch dynamically
selects an appropriate gc threshold based on the corresponding main routing
table size, using the assumption that we should in the worst case be able to
handle as many connections as the routing table can.
For ipv4, the maximum route cache size is 16 * the number of hash buckets in the
route cache. Given that xfrm4 starts garbage collection at the gc_thresh and
prevents new allocations at 2 * gc_thresh, we set gc_thresh to half the maximum
route cache size.
For ipv6, its a bit trickier. there is no maximum route cache size, but the
ipv6 dst_ops gc_thresh is statically set to 1024. It seems sane to select a
simmilar gc_thresh for the xfrm6 code that is half the number of hash buckets in
the v6 route cache times 16 (like the v4 code does).
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
include/net/ip6_fib.h | 6 ++++++
include/net/xfrm.h | 2 +-
net/ipv4/route.c | 2 +-
net/ipv4/xfrm4_policy.c | 13 ++++++++++++-
net/ipv6/ip6_fib.c | 6 ------
net/ipv6/xfrm6_policy.c | 15 +++++++++++++++
6 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 7c5c0f7..5f9abb7 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -22,6 +22,12 @@
#include <net/flow.h>
#include <net/netlink.h>
+#ifdef CONFIG_IPV6_MULTIPLE_TABLES
+#define FIB_TABLE_HASHSZ 256
+#else
+#define FIB_TABLE_HASHSZ 1
+#endif
+
struct rt6_info;
struct fib6_config
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 9e3a3f4..223e90a 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1280,7 +1280,7 @@ struct xfrm6_tunnel {
};
extern void xfrm_init(void);
-extern void xfrm4_init(void);
+extern void xfrm4_init(int rt_hash_size);
extern int xfrm_state_init(struct net *net);
extern void xfrm_state_fini(struct net *net);
extern void xfrm4_state_init(void);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 278f46f..fafbe16 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -3442,7 +3442,7 @@ int __init ip_rt_init(void)
printk(KERN_ERR "Unable to create route proc files\n");
#ifdef CONFIG_XFRM
xfrm_init();
- xfrm4_init();
+ xfrm4_init(ip_rt_max_size);
#endif
rtnl_register(PF_INET, RTM_GETROUTE, inet_rtm_getroute, NULL);
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 018ac8b..00f7f80 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -290,10 +290,21 @@ static void __exit xfrm4_policy_fini(void)
xfrm_policy_unregister_afinfo(&xfrm4_policy_afinfo);
}
-void __init xfrm4_init(void)
+void __init xfrm4_init(int rt_max_size)
{
xfrm4_state_init();
xfrm4_policy_init();
+ /*
+ * Select a default value for the gc_thresh based on the main route
+ * table hash size. It seems to me the worst case scenario is when
+ * we have ipsec operating in transport mode, in which we create a
+ * dst_entry per socket. The xfrm gc algorithm starts trying to remove
+ * entries at gc_thresh, and prevents new allocations as 2*gc_thresh
+ * so lets set an initial xfrm gc_thresh value at the rt_max_size/2.
+ * That will let us store an ipsec connection per route table entry,
+ * and start cleaning when were 1/2 full
+ */
+ xfrm4_dst_ops.gc_thresh = rt_max_size/2;
sysctl_hdr = register_net_sysctl_table(&init_net, net_ipv4_ctl_path,
xfrm4_policy_table);
}
diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
index 52ee1dc..9e915d3 100644
--- a/net/ipv6/ip6_fib.c
+++ b/net/ipv6/ip6_fib.c
@@ -164,12 +164,6 @@ static __inline__ void rt6_release(struct rt6_info *rt)
dst_free(&rt->u.dst);
}
-#ifdef CONFIG_IPV6_MULTIPLE_TABLES
-#define FIB_TABLE_HASHSZ 256
-#else
-#define FIB_TABLE_HASHSZ 1
-#endif
-
static void fib6_link_table(struct net *net, struct fib6_table *tb)
{
unsigned int h;
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 4acc308..db1f927 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -323,6 +323,7 @@ static struct ctl_table_header *sysctl_hdr;
int __init xfrm6_init(void)
{
int ret;
+ unsigned int gc_thresh;
ret = xfrm6_policy_init();
if (ret)
@@ -331,6 +332,20 @@ int __init xfrm6_init(void)
ret = xfrm6_state_init();
if (ret)
goto out_policy;
+ /*
+ * We need a good default value for the xfrm6 gc threshold.
+ * In ipv4 we set it to the route hash table size * 8, which
+ * is half the size of the maximaum route cache for ipv4. It
+ * would be good to do the same thing for v6, except the table is
+ * constructed differently here. Here each table for a net namespace
+ * can have FIB_TABLE_HASHSZ entries, so lets go with the same
+ * computation that we used for ipv4 here. Also, lets keep the initial
+ * gc_thresh to a minimum of 1024, since, the ipv6 route cache defaults
+ * to that as a minimum as well
+ */
+ gc_thresh = FIB_TABLE_HASHSZ * 8;
+ xfrm6_dst_ops.gc_thresh = (gc_thresh < 1024) ? 1024 : gc_thresh;
+
sysctl_hdr = register_net_sysctl_table(&init_net, net_ipv6_ctl_path,
xfrm6_policy_table);
out:
^ permalink raw reply related
* Re: [PATCH 2/2] c/r: Add AF_INET support (v3)
From: John Dykstra @ 2009-07-29 0:28 UTC (permalink / raw)
To: Dan Smith; +Cc: containers, netdev, Oren Laaden, Alexey Dobriyan
In-Reply-To: <874oswer21.fsf@caffeine.danplanet.com>
On Tue, 2009-07-28 at 09:00 -0700, Dan Smith wrote:
> I think that we need to increment timers like this on restore anyway,
> which should make sure that the TIME_WAIT timers run for the same
> amount of wall-clock time regardless of how much time was spent in the
> process of migration, right?
Agreed.
> Since checkpoint is not aware of a potential migration, a regular
> (i.e. not intended for migration) checkpoint operation on a task
> running in the init netns will leave the TIME_WAIT socket in place
> until the timer expires.
Can this relationship be counted on--that checkpoints in the init
namespace will be stored indefinitely, and that otherwise the
checkpointed tree will be migrated in a bounded amount of time? I think
you'll need to differentiate the two cases to get proper network
behavior, so there needs to be some reliable and documented indicator.
Or should this be an advise option?
> >> + if (h->sock.state == TCP_LISTEN) {
> >> + ret = socket->ops->listen(socket, h->sock.backlog);
> >> + ckpt_debug("inet listen: %i", ret);
> >> + if (ret < 0)
> >> + goto out;
> >> +
> >> + ret = sock_add_parent(ctx, socket->sk);
> >> + if (ret < 0)
> >> + goto out;
>
> JD> So this is just dummied off as a proof-of-concept for LISTEN?
>
> I'm not sure what you mean...
It's not as functional as the rest of the code. The listening address
is hardwired and you're not restoring the socket state. I'm assuming
that was intended as a development step.
> JD> At a minimum, you'll want to start the TCP retransmit timer if there is
> JD> unacknowledged data outstanding. And other timers for other states, as
> JD> they're supported.
>
> JD> And you probably do want to do slow-start again--disregard my babbling
> JD> from yesterday.
It seems there's a wide range of requirements here. A tree migrating
within a rack doesn't want to do slow-start again, nor restart its RTT
metrics. But in the general case, you do need to. Maybe the scaling of
congestion window and RTT on restore should be a tunable.
> JD> ret = sock_create(h->sock_common.family, h->sock.type, 0, &socket);
> JD> if (ret < 0)
> JD> return ERR_PTR(ret);
>
>
> JD> You _really_ want to pass the actual protocol number to sock_create().
> JD> The size of the sk it creates depends on this. You'll quickly be in
> JD> memory corruption hell without it.
>
> You mean I need to verify that the protocol is one of IPPROTO_TCP,
> IPPROTO_UDP, or PF_UNIX, right?
Right now, you're passing zero. For inet and inet6 sockets, you want to
pass field num out of ckpt_hdr_socket_inet.
> JD> Network sockets also use sk->sk_wmem_alloc to track references to
> JD> the sock from egress skb's
>
> IIUC, this function is part of the framework's leak detection. That
> code looks to make sure objects don't gain any additional users
> between the start and end of the checkpoint operation. I think the
> sk_refcnt satisfies that requirement here, no?
As long as the function is only used for this, you're fine. But it's
named very generally, and if it get used in the future for something
else, there might be a hole.
-- John
^ permalink raw reply
* [patch 64/71] nf_conntrack: nf_conntrack_alloc() fixes
From: Greg KH @ 2009-07-28 23:41 UTC (permalink / raw)
To: linux-kernel, stable
Cc: stable-review, torvalds, akpm, alan, netdev, netfilter-devel,
Patrick McHardy, Eric Dumazet, Paul E. McKenney
In-Reply-To: <20090728234756.GA11917@kroah.com>
[-- Attachment #1: nf_conntrack-nf_conntrack_alloc-fixes.patch --]
[-- Type: text/plain, Size: 3488 bytes --]
2.6.30-stable review patch. If anyone has any objections, please let us know.
------------------
From: Eric Dumazet <eric.dumazet@gmail.com>
commit 941297f443f871b8c3372feccf27a8733f6ce9e9 upstream.
When a slab cache uses SLAB_DESTROY_BY_RCU, we must be careful when allocating
objects, since slab allocator could give a freed object still used by lockless
readers.
In particular, nf_conntrack RCU lookups rely on ct->tuplehash[xxx].hnnode.next
being always valid (ie containing a valid 'nulls' value, or a valid pointer to next
object in hash chain.)
kmem_cache_zalloc() setups object with NULL values, but a NULL value is not valid
for ct->tuplehash[xxx].hnnode.next.
Fix is to call kmem_cache_alloc() and do the zeroing ourself.
As spotted by Patrick, we also need to make sure lookup keys are committed to
memory before setting refcount to 1, or a lockless reader could get a reference
on the old version of the object. Its key re-check could then pass the barrier.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
Documentation/RCU/rculist_nulls.txt | 7 ++++++-
net/netfilter/nf_conntrack_core.c | 21 ++++++++++++++++++---
2 files changed, 24 insertions(+), 4 deletions(-)
--- a/Documentation/RCU/rculist_nulls.txt
+++ b/Documentation/RCU/rculist_nulls.txt
@@ -83,11 +83,12 @@ not detect it missed following items in
obj = kmem_cache_alloc(...);
lock_chain(); // typically a spin_lock()
obj->key = key;
-atomic_inc(&obj->refcnt);
/*
* we need to make sure obj->key is updated before obj->next
+ * or obj->refcnt
*/
smp_wmb();
+atomic_set(&obj->refcnt, 1);
hlist_add_head_rcu(&obj->obj_node, list);
unlock_chain(); // typically a spin_unlock()
@@ -159,6 +160,10 @@ out:
obj = kmem_cache_alloc(cachep);
lock_chain(); // typically a spin_lock()
obj->key = key;
+/*
+ * changes to obj->key must be visible before refcnt one
+ */
+smp_wmb();
atomic_set(&obj->refcnt, 1);
/*
* insert obj in RCU way (readers might be traversing chain)
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -525,22 +525,37 @@ struct nf_conn *nf_conntrack_alloc(struc
}
}
- ct = kmem_cache_zalloc(nf_conntrack_cachep, gfp);
+ /*
+ * Do not use kmem_cache_zalloc(), as this cache uses
+ * SLAB_DESTROY_BY_RCU.
+ */
+ ct = kmem_cache_alloc(nf_conntrack_cachep, gfp);
if (ct == NULL) {
pr_debug("nf_conntrack_alloc: Can't alloc conntrack.\n");
atomic_dec(&net->ct.count);
return ERR_PTR(-ENOMEM);
}
-
- atomic_set(&ct->ct_general.use, 1);
+ /*
+ * Let ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.next
+ * and ct->tuplehash[IP_CT_DIR_REPLY].hnnode.next unchanged.
+ */
+ memset(&ct->tuplehash[IP_CT_DIR_MAX], 0,
+ sizeof(*ct) - offsetof(struct nf_conn, tuplehash[IP_CT_DIR_MAX]));
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple = *orig;
+ ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.pprev = NULL;
ct->tuplehash[IP_CT_DIR_REPLY].tuple = *repl;
+ ct->tuplehash[IP_CT_DIR_REPLY].hnnode.pprev = NULL;
/* Don't set timer yet: wait for confirmation */
setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
#ifdef CONFIG_NET_NS
ct->ct_net = net;
#endif
+ /*
+ * changes to lookup keys must be done before setting refcnt to 1
+ */
+ smp_wmb();
+ atomic_set(&ct->ct_general.use, 1);
return ct;
}
EXPORT_SYMBOL_GPL(nf_conntrack_alloc);
^ permalink raw reply
* Re: [PATCH v2 2/2] IPVS: use pr_err and friends instead of IP_VS_ERR and friends
From: Simon Horman @ 2009-07-28 23:33 UTC (permalink / raw)
To: Joe Perches; +Cc: Hannes Eder, lvs-devel, netdev, linux-kernel, Jan Engelhardt
In-Reply-To: <1248814615.18284.22.camel@Joe-Laptop.home>
On Tue, Jul 28, 2009 at 01:56:55PM -0700, Joe Perches wrote:
> On Tue, 2009-07-28 at 22:35 +0200, Hannes Eder wrote:
> > Since pr_err and friends is used instead of printk there is not point
> > in keeping IP_VS_ERR and friends.
> > + pr_err("Schedule: port zero only supported "
> > + "in persistent services, "
> > + "check your ipvs configuration\n");
>
> You might consider ignoring the checkpatch 80 char limit here
> and consolidate the message into a single string.
Personally, I prefer it the way it is.
> It helps when grepping.
>
> > @@ -377,8 +377,8 @@ static int ip_vs_svc_hash(struct ip_vs_service *svc)
> > static int ip_vs_svc_unhash(struct ip_vs_service *svc)
> > {
> > if (!(svc->flags & IP_VS_SVC_F_HASHED)) {
> > - IP_VS_ERR("ip_vs_svc_unhash(): request for unhash flagged, "
> > - "called from %p\n", __builtin_return_address(0));
> > + pr_err("ip_vs_svc_unhash(): request for unhash flagged, "
> > + "called from %p\n", __builtin_return_address(0));
>
> Maybe use something like:
> "%s(): <foo> called from %pf",
> __func__, __builtin_return_address(0)
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [Bugme-new] [Bug 13830] New: 8139too - packets dropped/lost by ethernet stack
From: Andrew Morton @ 2009-07-28 22:26 UTC (permalink / raw)
To: netdev; +Cc: bugzilla-daemon, bugme-daemon, aldursys
In-Reply-To: <bug-13830-10286@http.bugzilla.kernel.org/>
(switched to email. Please respond via emailed reply-to-all, not via the
bugzilla web interface).
On Sun, 26 Jul 2009 10:32:52 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=13830
>
> Summary: 8139too - packets dropped/lost by ethernet stack
> Product: Drivers
> Version: 2.5
> Kernel Version: 2.6.28, 2.6.31
> Platform: All
> OS/Version: Linux
> Tree: Mainline
> Status: NEW
> Severity: normal
> Priority: P1
> Component: Network
> AssignedTo: drivers_network@kernel-bugs.osdl.org
> ReportedBy: aldursys@gmail.com
> Regression: No
>
>
> I'm losing receive packets with the 8139too REALTEK driver when I place the
> driver under load.
>
> Details:
>
> distribution: Ubuntu Jaunty 9.04, Ubuntu Karmic Alpha-3.
>
> eth0 Link encap:Ethernet HWaddr 00:1b:38:08:7f:d3
> inet addr:192.168.2.3 Bcast:192.168.2.255 Mask:255.255.255.0
> inet6 addr: fe80::21b:38ff:fe08:7fd3/64 Scope:Link
> UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1
> RX packets:9164 errors:94 dropped:94 overruns:94 frame:0
> TX packets:7497 errors:0 dropped:0 overruns:0 carrier:0
> collisions:0 txqueuelen:1000
> RX bytes:12058463 (12.0 MB) TX bytes:792762 (792.7 KB)
> Interrupt:21 Base address:0x2000
>
> Dmesg details
>
> [ 4.357046] 8139cp: 10/100 PCI Ethernet driver v1.3 (Mar 22, 2004)
> [ 4.357092] 8139cp 0000:05:01.0: This (id 10ec:8139 rev 10) is not an 8139C+
> compatible chip, use 8139too
> [ 4.359452] 8139too Fast Ethernet driver 0.9.28
> [ 4.359513] 8139too 0000:05:01.0: PCI INT A -> GSI 21 (level, low) -> IRQ 21
> [ 4.359527] 8139too 0000:05:01.0: setting latency timer to 64
> [ 4.360886] eth0: RealTek RTL8139 at 0x2000, 00:1b:38:08:7f:d3, IRQ 21
> [ 4.360889] eth0: Identified 8139 chip type 'RTL-8100B/8139D'
>
>
> The problem appears to goes away if I enable just the debug message at the end
> of the function in the interrupt handler (rtl8139_interrupt) which sort of
> suggests that something is coming into the driver faster than it can handle or
> it is missing a state somewhere.
>
> (I get much better throughput with the debug message on than without it).
>
> Also none of the error counters appear to be incremented by the driver itself -
> suggesting again that the higher level is expecting something the driver isn't
> providing.
>
> Bug 10682 may be related (http://bugzilla.kernel.org/show_bug.cgi?id=10682) but
> isn't exactly the same.
>
> More details at https://bugs.launchpad.net/ubuntu/+source/linux/+bug/401891
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: Add networking wireless drivers section
From: Johannes Berg @ 2009-07-28 21:33 UTC (permalink / raw)
To: David Miller
Cc: joe, linville, julia, netdev, linux-kernel, kernel-janitors,
linux-wireless
In-Reply-To: <20090728.113353.08715297.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
On Tue, 2009-07-28 at 11:33 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Tue, 28 Jul 2009 11:06:37 -0700
>
> > On Tue, 2009-07-28 at 10:30 -0700, David Miller wrote:
> >> Wireless patches need to be CC:'d to linux-wireless so that
> >> John Linville can notice and pick them up
> >
> > Signed-off-by: Joe Perches <joe@perches.com>
>
> Why not just create a plain "WIRELESS" umbrella entry
> that contains all of drivers/net/wireless, net/mac80211,
> and net/wireless?
I'll happily maintain net/mac80211 and net/wireless, but won't be
touching all the old crappy drivers/net/wireless with a long stick.
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] c/r: Add AF_UNIX support (v6)
From: Oren Laadan @ 2009-07-28 21:18 UTC (permalink / raw)
To: Dan Smith; +Cc: containers, Alexey Dobriyan, netdev
In-Reply-To: <87ljm8czsf.fsf@caffeine.danplanet.com>
Dan Smith wrote:
> OL> obj_sock_users() is only required for objects that are to be
> OL> tested for leaks in full container checkpoint. I don't think it's
> OL> needed, because the relation sock <-> file is 1-to-1. (If it
> OL> were, then you would also need to collect sockets).
>
> Okay, that also answers the question posed by John in the other
> thread.
>
> OL> (Actually, I will remove checkpoint_bad and restore_bad and modify
> OL> checkpoint_obj() and restore_obj() to fail if the respective
> OL> method is NULL).
>
> Okay, should I expect that to show up in v17-dev soon?
Yes.
>
> OL> Nit: I already got confused a few times because of the similar
> OL> names. Perhaps change CKPT_HDR_SOCKET_BUFFERS to
> OL> CKPT_HDR_SOCKET_QUEUE (and adjust the header name accordingly).
>
> Agreed. I remember writing that and remarking to myself how
> ridiculous of a name it was, but never went back to change it.
>
> OL> Unless/until we decide otherwise, let's keep all ckpt_hdr_xxx
> OL> definitions in a single place -- checkpoint_hdr.h
>
> That's how I initially had it, but Serge asked for it to be moved into
> the network headers. Serge?
>
> OL> Unless you intend to use the struct name 'ckpt_socket' elsewhere,
> OL> can we get rid of it (leaving only 'struct {') ?
>
> Yep.
>
> OL> Can you use fill_name() from checkpoint/file.c ?
>
> Yeah, looks like it.
>
> OL> This direct call to ->getname skips security checks through
> OL> getsockname(). You may want to refactor sys_getsockname() similar
> OL> to sys_bind().
>
> Okay.
>
>>> + if ((h->sock.userlocks & SOCK_SNDBUF_LOCK) &&
>>> + ((h->sock.sndbuf < SOCK_MIN_SNDBUF) ||
>>> + (h->sock.sndbuf > sysctl_wmem_max)))
>>> + return -EINVAL;
>
> OL> At least for afunix, if the user did not explicitly set the
> OL> sndbuf, then userlocks won't have SOCK_SNDBUF_LOCK set.
>
> OL> Also, if userlocks in the image doesn't have SOCK_SNDBUF_LOCK,
> OL> then the sndbuf value isn't checked ?
>
> I think I was thinking that I only needed to verify the buffer value
> if the user claimed to have set it (as if it would be ignored
> otherwise), but that doesn't seem right. So, I think the proper
> thing to do here is always check it (i.e., remove the first check of
> the lock).
>
> OL> What about verifying sock.flags itself ?
> OL> In doing that, some options may assume/require some state --
> OL> - SOCK_USE_WRITE_QUEUE only used in ipv4/ipv6/sctp
> OL> - SOCK_DESTROY only used in some protocols
>
> OL> Perhaps sanitize sock.flags per protocol ?
>
> Hmm, okay.
>
> OL> Many of these direct copy into the socket and sock effectively
> OL> bypass security checks that take place in {get,set}sockopt(),
> OL> either explicitly (e.g. sk_sndtimeo) or implicitly (e.g.
> OL> SOCK_LINGER in sock.flags, reflecting SO_LINGER option).
>
> OL> This applies both to checkpoint (potentially bypassing permission
> OL> of the checkpointer to view this data) and restart (bypassing
> OL> permissions of user to set these values).
>
> OL> The alternative is to use socksetopt/sockgetopt for those values
> OL> that should be subject to security checks.
>
> Yeah, I suppose so. I've resisted that thus far because it will make
> the sync operation so much harder to read, but I suppose it's
> unavoidable.
>
> OL> There should also be per-protocol consistency checks. E.g. afunix
> OL> cannot be in socket.state == SS_{DIS,}CONNECTING
>
> I suppose so, but I don't see anything in af_unix.c that seems to care :)
>
> OL> Better yet: -ENOSYS ?
>
> Okay.
>
>>> + ret = kernel_sendmsg(sock->sk_socket, &msg, &kvec, 1, len);
>
> OL> Is it possible to avoid the extra copy using splice() instead ?
>
> It's possible, but it will require some refactoring to get access to
> all the right pointers. I'd propose we push that optimization out
> until after we've got these patches integrated into the c/r tree.
Hmmm.. what about splice_direct_to_actor() ?
>
> OL> SOCK_DEAD in sk->flags may also pose a problem. (Do we at all
> OL> need to checkpoint and/or restore SOCK_DEAD ?!)
>
> Is there any reasonable way we could arrive at a SOCK_DEAD socket via
> a valid descriptor?
That's a good point. I think you are right, and there isn't.
Still need to keep it in mind for inet when including those lingering
sockets that don't belong to anyone.
This also means that a peer (of a dgram socket) that was closed will
not be checkpointed, so restoring the rcvbuf of the remaining dgram
socket wouldn't work.
>
> OL> Hmm... this test is quite hidden here - maybe a fat comment with a
> OL> reference to why it's here and what it is doing ?
>
> Sure.
>
>>> + else {
>>> + ckpt_debug("Buffer total %u exceeds limit %u\n",
>>> + h->total_bytes, *bufsize);
>>> + ret = -EINVAL;
>>> + goto out;
>>> + }
>>> + }
>>> +
>>> + for (i = 0; i < h->skb_count; i++) {
>>> + ret = sock_read_buffer(ctx, sock);
>>> + ckpt_debug("read buffer %i: %i\n", i, ret);
>>> + if (ret < 0)
>>> + break;
>>> +
>>> + total += ret;
>>> + if (total > h->total_bytes) {
>
> OL> What if 'total' overflows (for CAP_NET_ADMIN) ? perhaps:
> OL> if (total > h->total_bytes || total < ret) {
>
> Actually, I've changed this locally to require fewer variables and
> special cases. Let me know when I re-post if you don't think it's a
> better way to handle this.
>
> OL> Does the following bypass security checks for sys_connect() ?
>
> I don't think so. We're basically replicating sys_socketpair() here,
> which does not do a security check, presumably because all you're
> doing is hooking two sockets together that both belong to you. That's
> not to say that we're as safe as that limited operation, but I don't
> think it's totally clear. Perhaps someone more confident will
> comment.
Yes, please ... Serge ?
To me it sounds plausible. If we adopt it, then a comment in the
code is worthwhile.
>
>>> + /* Prime the socket's buffer limit with the maximum */
>>> + peer->sk_userlocks |= SOCK_SNDBUF_LOCK;
>>> + peer->sk_sndbuf = sysctl_wmem_max;
>
> OL> I suppose this meant to be temporary ?
>
> Right, it will be overridden when we synchronize the socket
> properties. I'll strengthen the comment.
Hmm.. then what happens when you have a circular dependency ?
For example, three dgram sockets, A, B and C where: A->B, B->C
and C->A ('->' means connected).
I suspect that sock_unix_restore_connect() will fail, because
neither:
+ if (!IS_ERR(this) && !IS_ERR(peer)) {
nor
+ } else if ((PTR_ERR(this) == -EINVAL) && (PTR_ERR(peer) == -EINVAL)) {
will hold true, therefore:
+ } else {
+ ckpt_debug("Order Error\n");
+ ret = PTR_ERR(this);
+ goto out;
+ }
Either that, or the temporary change cited above will become
permanent, because A won't be restored again.
>
> OL> It is indeed a good idea to make it generic for all sockets, but I
> OL> suspect the way sock_read_buffers() works will only be suitable
> OL> for afunix, and perhaps for in-container inet4/6 (by
> OL> "in-container" I mean that both sides of the connection are in the
> OL> checkpoint image).
>
> Yeah, I meant to put a comment in the commit log about this. As I
> mentioned before, I was hoping to put most of the effort into the UNIX
> patch and move forward with that as we iterate on the INET patch.
> Thus, this was part of the buffer restore re-swizzle that clearly
> won't work for INET. In the meantime I've been working on the INET
> patch and splitting it up while retaining the necessary behavioral
> changes made here. In my next posting these should look better.
>
> OL> Hmm... does the code below work well with dgram sockets who
> OL> received data from the peer, and then the peer died (before
> OL> checkpoint) ?
>
> I'm not sure that I've replicated that exact scenario in my tests.
> However, when we're restoring socket A with outstanding incoming
> buffers, we restore them via B. If B is not in a reasonable state, I
> put it back into connected state to appease the sendmsg function and
> restore it when complete. See the "unshutdown" comment.
The problem isn't that B is not in a reasonable state, but
rather that B is not checkpointed in the first place (because it
is not reachable via any fd), so it isn't restored either.
Oren.
>
>>> +static struct file *sock_alloc_attach_fd(struct socket *socket)
>
> OL> Nit: I think this name is misleading - sock_attach_fd() itself is
> OL> already and should have been sock_attach_file() IMHO - so perhaps
> OL> s/fd/file here ?
>
> The name was chosen to reflect the fact that we're allocating a socket
> and then calling sock_attach_fd() on it. If you think it's less
> misleading to be inconsistent with the other call, I'll change it, but
> I wouldn't really agree... :)
> Thanks!
>
^ permalink raw reply
* [PATCH 3/3] au1000_eth: set MODULE_VERSION
From: Florian Fainelli @ 2009-07-28 21:06 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Manuel Lauss
This patch sets MODULE_VERSION to ease the tracking
of the driver versions.
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
index f5611a5..04b9e85 100644
--- a/drivers/net/au1000_eth.c
+++ b/drivers/net/au1000_eth.c
@@ -81,6 +81,7 @@ static int au1000_debug = 3;
MODULE_AUTHOR(DRV_AUTHOR);
MODULE_DESCRIPTION(DRV_DESC);
+MODULE_VERSION(DRV_VERSION);
MODULE_LICENSE("GPL");
/*
^ permalink raw reply related
* [PATCH 2/3] au1000_eth: remove unused variable in update_rx_stats
From: Florian Fainelli @ 2009-07-28 21:06 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Manuel Lauss
This patch removes the unused aup variable in update_rx_stats
Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
index 1f724da..f5611a5 100644
--- a/drivers/net/au1000_eth.c
+++ b/drivers/net/au1000_eth.c
@@ -649,7 +649,6 @@ static int au1000_init(struct net_device *dev)
static inline void update_rx_stats(struct net_device *dev, u32 status)
{
- struct au1000_private *aup = netdev_priv(dev);
struct net_device_stats *ps = &dev->stats;
ps->rx_packets++;
^ permalink raw reply related
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