* Re: [PATCH net-next-2.6] net: speedup netdev_set_master()
From: David Miller @ 2010-03-19 6:11 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1268977513.2894.223.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 19 Mar 2010 06:45:13 +0100
> Le jeudi 18 mars 2010 à 22:36 -0700, David Miller a écrit :
>> It was not an issue of related, but rather "B won't apply cleanly
>> without A". Even line offsets can make "git apply" reject.
>> --
>
> I see, I can resubmit later if you prefer, once net-2.6 fix is pulled in
> net-next, there is no hurry :)
No need, just let me know about this stuff in the future, that's
all :-)
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 6:21 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319060322.GA22319@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 07:48:57AM +0200, Timo Teräs wrote:
>> But it always matches. The caching happens using the inner
>> flow. Inner flow always matches with the same bundle unless
>> the bundle expires or goes stale. What happens is that I get
>> multiple cache entries per-inner flow each referencing to the
>> same bundle.
>
> Sorry for being slow, but if it always matches, doesn't that mean
> you'll only have a single bundle in the policy bundle list? IOW
> why do we need this at all?
No. The bundle created for specific flow, matches always later
that flow.
With transport mode wildcard policy, e.g. single policy saying
encrypt traffic with protocol X to all IP-address, you get a
separate bundle per-public IP destination. Bundle matches only
that specific IP since it gets a separate xfrm_state. But you
can talk to all the hosts in internet using same policy, so
you can end up with a whole lot of valid bundles in the same
policy.
I'm not sure how this works in tunnel mode. It might be that
single bundle can be reused for all packets. But I think the same
applies to tunnel mode. Since afinfo->fill_dst() puts the
inner flow to bundle xfrm_dst->u.rt.fl, which is later compared
against the inner flow by afinfo->find_bundle(). I think this
implies that for each flow traveling inside tunnel, it gets it's
separate xfrm_dst, so again you end up with a whole lot of
valid bundles in the same policy.
> Or have I misread your patch? You *are* proposing to cache the last
> used bundle in the policy, right?
Yes and no. The bundle used is cached on per-flow basis.
The flow cache can have lot of entries each referring to
same policy but separate bundle.
>> True. But if we go and prune a bundle due to it being bad or
>> needing garbage collection we need to invalidate all bundles
>> pointers, and we cannot access the back-pointer. Alternatively
>
> Why can't you access the back-pointer? You should always have
> a reference held on the policy, either explicit or implicit.
>
>> we need to keep xfrm_dst references again in the flow cache
>> requiring an expensive iteration of all flow cache entries
>> whenever a xfrm_dst needs to be deleted (which happens often).
>
> So does the IPv4 routing cache. I think what this reflects is
> just that the IPsec garbage collection mechanism is broken.
>
> There is no point in doing a GC on every dst_alloc if we know
> that it isn't going to go below the threshold. It should gain
> a minimum GC interval like IPv4. Or perhaps we can move the
> minimum GC interval check into the dst core.
Yes, I reported xfrm_dst GC being broke in the earlier mail.
But keeping policy and bundle in cache is still a win. If we
kill the xfrm_dst due to GC, we will also lose the policy
the flow matched. We might need to kill xfrm_dst due to the
inside dst going old, but the flow cache would still give
hit with policy info (but no bundle) the next a packet comes
in using the same flow.
- Timo
^ permalink raw reply
* Re: [iproute2] iproute2 question
From: Stephen Hemminger @ 2010-03-19 6:27 UTC (permalink / raw)
To: thomas yang; +Cc: hadi, netdev
In-Reply-To: <f4f837ab1003182253x12727855k118f7d67115daf23@mail.gmail.com>
On Fri, 19 Mar 2010 13:53:23 +0800
thomas yang <lampsu@gmail.com> wrote:
> 2010/3/18 Stephen Hemminger <shemminger@vyatta.com>:
> > On Thu, 18 Mar 2010 17:02:16 +0800
> > thomas yang <lampsu@gmail.com> wrote:
> >
> >> Hi,
> >>
> >> Linux router R (has some interfaces) use the table "main" as default
> >> routing table to route packets. If some link connected to R failed,
> >> I want to use another table "backup_tbl" to route packets; if the
> >> broken link repaired , go back to use table "main" . How to do this?
> >
> > That's now it works. Use a real routing daemon.
> >
>
> I want to reduce the packet loss that happens while routers converge
> after a topology change due to a failure, use precalculated backup
> table to do rapid failure repair (repair faster than routing daemon,
> fast reroute).
> I do static routing on my linux routers. When some link failed,
> I want to use backup tables to route packets. Can iproute2 do this?
You could probably kludge something with multiple route tables
and modifying a single 'ip rule'.
Something like:
http://www.linuxhorizon.ro/iproute2.html
^ permalink raw reply
* [PATCH net-next-2.6] ipv6: Reduce timer events for addrconf_verify().
From: YOSHIFUJI Hideaki @ 2010-03-18 9:01 UTC (permalink / raw)
To: davem; +Cc: yoshfuji, netdev, stephen.hemminger
This patch reduces timer events while keeping accuracy by rounding
our timer and/or batching several address validations in addrconf_verify().
addrconf_verify() is called at earliest timeout among interface addresses'
timeouts, but at maximum ADDR_CHECK_FREQUENCY (120 secs).
In most cases, all of timeouts of interface addresses are long enough
(e.g. several hours or days vs 2 minutes), this timer is usually called
every ADDR_CHECK_FREQUENCY, and it is okay to be lazy.
(Note this timer could be eliminated if all code paths which modifies
variables related to timeouts call us manually, but it is another story.)
However, in other least but important cases, we try keeping accuracy.
When the real interface address timeout is coming, and the timeout
is just before the rounded timeout, we accept some error.
When a timeout has been reached, we also try batching other several
events in very near future.
Signed-off-by: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
---
net/ipv6/addrconf.c | 27 +++++++++++++++++++++++----
1 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 8d41abc..b716083 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -99,6 +99,10 @@
#define INFINITY_LIFE_TIME 0xFFFFFFFF
#define TIME_DELTA(a,b) ((unsigned long)((long)(a) - (long)(b)))
+#define ADDRCONF_TIMER_FUZZ_MINUS (HZ > 50 ? HZ/50 : 1)
+#define ADDRCONF_TIMER_FUZZ (HZ / 4)
+#define ADDRCONF_TIMER_FUZZ_MAX (HZ)
+
#ifdef CONFIG_SYSCTL
static void addrconf_sysctl_register(struct inet6_dev *idev);
static void addrconf_sysctl_unregister(struct inet6_dev *idev);
@@ -3124,12 +3128,12 @@ int ipv6_chk_home_addr(struct net *net, struct in6_addr *addr)
static void addrconf_verify(unsigned long foo)
{
struct inet6_ifaddr *ifp;
- unsigned long now, next;
+ unsigned long now, next, next_sec, next_sched;
int i;
spin_lock_bh(&addrconf_verify_lock);
now = jiffies;
- next = now + ADDR_CHECK_FREQUENCY;
+ next = round_jiffies_up(now + ADDR_CHECK_FREQUENCY);
del_timer(&addr_chk_timer);
@@ -3147,7 +3151,8 @@ restart:
continue;
spin_lock(&ifp->lock);
- age = (now - ifp->tstamp) / HZ;
+ /* We try to batch several events at once. */
+ age = (now - ifp->tstamp + ADDRCONF_TIMER_FUZZ_MINUS) / HZ;
#ifdef CONFIG_IPV6_PRIVACY
regen_advance = ifp->idev->cnf.regen_max_retry *
@@ -3222,7 +3227,21 @@ restart:
read_unlock(&addrconf_hash_lock);
}
- addr_chk_timer.expires = time_before(next, jiffies + HZ) ? jiffies + HZ : next;
+ next_sec = round_jiffies_up(next);
+ next_sched = next;
+
+ /* If rounded timeout is accurate enough, accept it. */
+ if (time_before(next_sec, next + ADDRCONF_TIMER_FUZZ))
+ next_sched = next_sec;
+
+ /* And minimum interval is ADDRCONF_TIMER_FUZZ_MAX. */
+ if (time_before(next_sched, jiffies + ADDRCONF_TIMER_FUZZ_MAX))
+ next_sched = jiffies + ADDRCONF_TIMER_FUZZ_MAX;
+
+ ADBG((KERN_DEBUG "now = %lu, schedule = %lu, rounded schedule = %lu => %lu\n",
+ now, next, next_sec, next_sched));
+
+ addr_chk_timer.expires = next_sched;
add_timer(&addr_chk_timer);
spin_unlock_bh(&addrconf_verify_lock);
}
--
1.5.6.5
^ permalink raw reply related
* Re: [Uclinux-dist-devel] [PATCH] can: bfin_can: switch to common Blackfin can header
From: Mike Frysinger @ 2010-03-19 6:35 UTC (permalink / raw)
To: David Miller
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA,
urs.thuermann-l29pVbxQd1IUtdQbppsyvg,
oliver.hartkopp-l29pVbxQd1IUtdQbppsyvg,
uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b
In-Reply-To: <20100316.225150.213941689.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Wed, Mar 17, 2010 at 01:51, David Miller wrote:
> From: Mike Frysinger <vapier.adi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> if the next tree is merged into linux-next, then the header is already there
>
> One of the highest priority trees in -next is net-next-2.6,
> because so much other stuff ends up depending upon it.
>
> Therefore I keep it building independently, so no matter what Stephen
> Rothwell does the build isn't likely to break.
>
> Just get your stuff merged properly to Linus then resubmit
> your patch.
Linus has pulled my tree, so there shouldnt be any reason to not merge
this patch now
-mike
^ permalink raw reply
* [PATCH net-next-2.6] net: snmp mib cleanup
From: Eric Dumazet @ 2010-03-19 6:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev
There is no point to align or pad mibs to cache lines, they are per cpu
allocated with a 8 bytes alignment anyway.
This wastes space for no gain. This patch removes __SNMP_MIB_ALIGN__
Since SNMP mibs contain "unsigned long" fields only, we can relax the
allocation alignment from "unsigned long long" to "unsigned long"
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/sctp/sctp.h | 2 +-
include/net/snmp.h | 29 +++++++----------------------
net/dccp/dccp.h | 2 +-
net/ipv4/af_inet.c | 4 ++--
4 files changed, 11 insertions(+), 26 deletions(-)
diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 78740ec..5915155 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -268,7 +268,7 @@ enum {
#define SCTP_MIB_MAX __SCTP_MIB_MAX
struct sctp_mib {
unsigned long mibs[SCTP_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* Print debugging messages. */
diff --git a/include/net/snmp.h b/include/net/snmp.h
index 692ee00..884fdbb 100644
--- a/include/net/snmp.h
+++ b/include/net/snmp.h
@@ -52,26 +52,11 @@ struct snmp_mib {
* count on the 20Gb/s + networks people expect in a few years time!
*/
-/*
- * The rule for padding:
- * Best is power of two because then the right structure can be found by a
- * simple shift. The structure should be always cache line aligned.
- * gcc needs n=alignto(cachelinesize, popcnt(sizeof(bla_mib))) shift/add
- * instructions to emulate multiply in case it is not power-of-two.
- * Currently n is always <=3 for all sizes so simple cache line alignment
- * is enough.
- *
- * The best solution would be a global CPU local area , especially on 64
- * and 128byte cacheline machine it makes a *lot* of sense -AK
- */
-
-#define __SNMP_MIB_ALIGN__ ____cacheline_aligned
-
/* IPstats */
#define IPSTATS_MIB_MAX __IPSTATS_MIB_MAX
struct ipstats_mib {
unsigned long mibs[IPSTATS_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* ICMP */
#define ICMP_MIB_DUMMY __ICMP_MIB_MAX
@@ -79,36 +64,36 @@ struct ipstats_mib {
struct icmp_mib {
unsigned long mibs[ICMP_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
#define ICMPMSG_MIB_MAX __ICMPMSG_MIB_MAX
struct icmpmsg_mib {
unsigned long mibs[ICMPMSG_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* ICMP6 (IPv6-ICMP) */
#define ICMP6_MIB_MAX __ICMP6_MIB_MAX
struct icmpv6_mib {
unsigned long mibs[ICMP6_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
#define ICMP6MSG_MIB_MAX __ICMP6MSG_MIB_MAX
struct icmpv6msg_mib {
unsigned long mibs[ICMP6MSG_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* TCP */
#define TCP_MIB_MAX __TCP_MIB_MAX
struct tcp_mib {
unsigned long mibs[TCP_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* UDP */
#define UDP_MIB_MAX __UDP_MIB_MAX
struct udp_mib {
unsigned long mibs[UDP_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
/* Linux */
#define LINUX_MIB_MAX __LINUX_MIB_MAX
diff --git a/net/dccp/dccp.h b/net/dccp/dccp.h
index 5ef32c2..53f8e12 100644
--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -189,7 +189,7 @@ enum {
#define DCCP_MIB_MAX __DCCP_MIB_MAX
struct dccp_mib {
unsigned long mibs[DCCP_MIB_MAX];
-} __SNMP_MIB_ALIGN__;
+};
DECLARE_SNMP_STAT(struct dccp_mib, dccp_statistics);
#define DCCP_INC_STATS(field) SNMP_INC_STATS(dccp_statistics, field)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 33b7dff..55e1190 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1401,10 +1401,10 @@ EXPORT_SYMBOL_GPL(snmp_fold_field);
int snmp_mib_init(void __percpu *ptr[2], size_t mibsize)
{
BUG_ON(ptr == NULL);
- ptr[0] = __alloc_percpu(mibsize, __alignof__(unsigned long long));
+ ptr[0] = __alloc_percpu(mibsize, __alignof__(unsigned long));
if (!ptr[0])
goto err0;
- ptr[1] = __alloc_percpu(mibsize, __alignof__(unsigned long long));
+ ptr[1] = __alloc_percpu(mibsize, __alignof__(unsigned long));
if (!ptr[1])
goto err1;
return 0;
^ permalink raw reply related
* AW: [Uclinux-dist-devel] [PATCH] can: bfin_can: switch to common Blackfin can header
From: Hartkopp, Oliver (K-EFFI/P) @ 2010-03-19 6:56 UTC (permalink / raw)
To: Mike Frysinger, David Miller
Cc: socketcan-core, netdev, uclinux-dist-devel,
Thuermann, Urs, Dr. (K-EFFI/I)
In-Reply-To: <8bd0f97a1003182335l3c121d87v9369145852c9545d@mail.gmail.com>
Hi Mike,
your patch is just a cleanup and definitely not mission-critical.
Please wait until your needed environment emerges in Dave's net-2.6 or
net-next-2.6 tree until you post your patch for the specific tree then.
This makes it much easier - at least for Dave.
Thanks,
Oliver
-----Ursprüngliche Nachricht-----
Von: Mike Frysinger [mailto:vapier.adi@gmail.com]
Gesendet: Freitag, 19. März 2010 07:35
An: David Miller
Cc: socketcan-core@lists.berlios.de; netdev@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org; Hartkopp, Oliver (K-EFFI/P); Thuermann, Urs, Dr. (K-EFFI/I)
Betreff: Re: [Uclinux-dist-devel] [PATCH] can: bfin_can: switch to common Blackfin can header
On Wed, Mar 17, 2010 at 01:51, David Miller wrote:
> From: Mike Frysinger <vapier.adi@gmail.com>
>> if the next tree is merged into linux-next, then the header is already there
>
> One of the highest priority trees in -next is net-next-2.6,
> because so much other stuff ends up depending upon it.
>
> Therefore I keep it building independently, so no matter what Stephen
> Rothwell does the build isn't likely to break.
>
> Just get your stuff merged properly to Linus then resubmit
> your patch.
Linus has pulled my tree, so there shouldnt be any reason to not merge
this patch now
-mike
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 7:17 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA317CE.4050503@iki.fi>
On Fri, Mar 19, 2010 at 08:21:02AM +0200, Timo Teräs wrote:
>
>> Or have I misread your patch? You *are* proposing to cache the last
>> used bundle in the policy, right?
>
> Yes and no. The bundle used is cached on per-flow basis.
> The flow cache can have lot of entries each referring to
> same policy but separate bundle.
OK so I did misread your patch.
In fact it is already doing exactly what I was suggesting, I'll
review your patch again with this new insignt :)
> But keeping policy and bundle in cache is still a win. If we
> kill the xfrm_dst due to GC, we will also lose the policy
> the flow matched. We might need to kill xfrm_dst due to the
> inside dst going old, but the flow cache would still give
> hit with policy info (but no bundle) the next a packet comes
> in using the same flow.
We were in complete agreement all along :)
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] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 7:20 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev
In-Reply-To: <1268655610-7845-1-git-send-email-timo.teras@iki.fi>
On Mon, Mar 15, 2010 at 02:20:10PM +0200, Timo Teras wrote:
>
> - policy = flow_cache_lookup(net, fl, dst_orig->ops->family,
> - dir, xfrm_policy_lookup);
> - err = PTR_ERR(policy);
> - if (IS_ERR(policy)) {
> - XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
> - goto dropdst;
> + fce = flow_cache_lookup(&net->xfrm.flow_cache,
> + fl, family, dir);
> + if (fce == NULL)
> + goto no_cache;
> +
> + xf = container_of(fce, struct xfrm_flow_cache_entry, fce);
> + xfrm_flow_cache_entry_validate(&net->xfrm.flow_cache, fce);
This doesn't work.
The flow cache operates without locking as it is a per-cpu cache.
To make this work you must ensure that you stay on the same CPU
or use some other form of synchronoisation if you write to the
object returned.
AFAICS there is no synchronisation here and you're writing to fce.
So you'll need to disable preemption around the bit that touches
fce.
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] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 7:27 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <4BA317CE.4050503@iki.fi>
Timo Teräs wrote:
> Herbert Xu wrote:
>> On Fri, Mar 19, 2010 at 07:48:57AM +0200, Timo Teräs wrote:
>>> But it always matches. The caching happens using the inner
>>> flow. Inner flow always matches with the same bundle unless
>>> the bundle expires or goes stale. What happens is that I get
>>> multiple cache entries per-inner flow each referencing to the
>>> same bundle.
>>
>> Sorry for being slow, but if it always matches, doesn't that mean
>> you'll only have a single bundle in the policy bundle list? IOW
>> why do we need this at all?
>
> No. The bundle created for specific flow, matches always later
> that flow.
Just figured that's it's easier to explain with an example.
We have SPD:
10.1.0.0/16 - 10.2.0.0/16 tunnel
1.2.3.4 - 4.3.2.1
Now we get n+1 clients to connect to server in 10.2.0.1.
They each get separate bundle, since the xfrm_dst will be
created and search using flow id's like:
src 10.1.x.x dst 10.2.0.1
So there's one xfrm_policy and xfrm_state, but n+1
xfrm_dst's.
Since the flow cache caches the result of lookups on the
inner flow "10.1.x.x->10.2.0.1" basis, it always returns
matching valid bundle in O(1) time unless the xfrm_dst
expired.
Currently it's looked up with O(n) search in find_bundle.
Same thing happens with wildcard transport mode SPD's.
E.g. SPD:
0.0.0.0/0 - 0.0.0.0/0 proto gre, transport
We are talking with gre to n+1 tunnel destinations.
We get n+1 xfrm_dst's in that xfrm_policy. Flow cache
works on inner flow using flows like:
src 1.2.3.4 dst 4.3.2.1 proto gre
And can keep in cache the right policy always, and
the bundle to use as long as it stays valid.
Hopefully this explains why I think the patch is
useful.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 7:48 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319072053.GA22913@gondor.apana.org.au>
Herbert Xu wrote:
> On Mon, Mar 15, 2010 at 02:20:10PM +0200, Timo Teras wrote:
>> - policy = flow_cache_lookup(net, fl, dst_orig->ops->family,
>> - dir, xfrm_policy_lookup);
>> - err = PTR_ERR(policy);
>> - if (IS_ERR(policy)) {
>> - XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
>> - goto dropdst;
>> + fce = flow_cache_lookup(&net->xfrm.flow_cache,
>> + fl, family, dir);
>> + if (fce == NULL)
>> + goto no_cache;
>> +
>> + xf = container_of(fce, struct xfrm_flow_cache_entry, fce);
>> + xfrm_flow_cache_entry_validate(&net->xfrm.flow_cache, fce);
>
> This doesn't work.
>
> The flow cache operates without locking as it is a per-cpu cache.
> To make this work you must ensure that you stay on the same CPU
> or use some other form of synchronoisation if you write to the
> object returned.
>
> AFAICS there is no synchronisation here and you're writing to fce.
>
> So you'll need to disable preemption around the bit that touches
> fce.
But flow_cache_lookup disables pre-emption until _put is called.
So it should work. Would there be a cleaner way?
However, now I figured that we need to make walk.dead atomic
because it's read some times without taking the policy lock.
- Timo
^ permalink raw reply
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Pekka Savola @ 2010-03-19 7:58 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
In-Reply-To: <20100318105939.57f8d377@nehalam>
On Thu, 18 Mar 2010, Stephen Hemminger wrote:
>> The experimental, earlier spec (GTSH, RFC3682) did not have this
>> requirement. Most if not all implementations support only GTSH mode.
>> So a backward-compatibility option may be desirable.
>
> The ICMP receive error handling does need to be updated.
>
> But any application using GTSM should be setting IP_TTL socket option
> to set send TTL. But, not sure if Linux TCP ever sends ICMP
> for existing sessions at all.
Thanks, Stephen! It's nice to see at least one compliant RFC5082
implementation ;-)
Good point that no one should should probably even be sending ICMP
messages for TCP sockets, but on receive side the checks are important
:-)
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Eric Dumazet @ 2010-03-19 8:21 UTC (permalink / raw)
To: Pekka Savola; +Cc: Stephen Hemminger, David Miller, netdev
In-Reply-To: <alpine.LRH.2.00.1003190837210.6428@netcore.fi>
Le vendredi 19 mars 2010 à 09:58 +0200, Pekka Savola a écrit :
>
> > But any application using GTSM should be setting IP_TTL socket
> option
> > to set send TTL. But, not sure if Linux TCP ever sends ICMP
> > for existing sessions at all.
>
> Thanks, Stephen! It's nice to see at least one compliant RFC5082
> implementation ;-)
>
> Good point that no one should should probably even be sending ICMP
> messages for TCP sockets, but on receive side the checks are
> important
> :-)
This requires that any router in the path between the client and server
also respects the MINTTL when sending ICMP. Not sure how practical it
is...
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 70df409..a9d3ba5 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -367,6 +367,9 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info)
if (sock_owned_by_user(sk))
NET_INC_STATS_BH(net, LINUX_MIB_LOCKDROPPEDICMPS);
+ if (iph->ttl < inet_sk(sk)->min_ttl)
+ goto out;
+
if (sk->sk_state == TCP_CLOSE)
goto out;
^ permalink raw reply related
* Re: [PATCH] tcp: Generalized TTL Security Mechanism
From: Pekka Savola @ 2010-03-19 8:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, David Miller, netdev
In-Reply-To: <1268986871.3048.9.camel@edumazet-laptop>
On Fri, 19 Mar 2010, Eric Dumazet wrote:
> This requires that any router in the path between the client and server
> also respects the MINTTL when sending ICMP. Not sure how practical it
> is...
You're correct that with multihop GTSM, ICMP becomes trickier. I'm
not sure how applicable GTSM is really in multihop scenarios, though.
I would not recommend using it to secure e.g. with minttl=250 or
something that uncontrollable. Your comment relates mainly to ICMP
soft/hard errors which are not critical for correct operation.
http://tools.ietf.org/html/draft-ietf-tcpm-icmp-attacks-11 discusses
this.
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 8:29 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA32C41.2020000@iki.fi>
On Fri, Mar 19, 2010 at 09:48:17AM +0200, Timo Teräs wrote:
>
> But flow_cache_lookup disables pre-emption until _put is called.
> So it should work. Would there be a cleaner way?
Previously the flow cache returned a policy directly which works
because whenever we modify that policy we'd take the appropriate
lock.
Your patch changes it so that it now returns an fce. But nothing
is guarding the code that modifies fce. So two CPUs may end up
modifying the same fce.
However, it would appear that this race could be harmless, provided
that you are careful about dereferencing fce->policy and fce->dst.
IOW, this is not OK
if (fce->policy)
use fce->policy;
and this should work
policy = fce->policy;
if (policy)
use policy;
Actually on second tought, even this isn't totally safe. Who
is taking a reference count on the policy and dst? I see a ref
count on the fce, but nothing on fce->dst and fce->policy. Do
you have an implicit reference on them?
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] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 8:37 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319082909.GA23363@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 09:48:17AM +0200, Timo Teräs wrote:
>> But flow_cache_lookup disables pre-emption until _put is called.
>> So it should work. Would there be a cleaner way?
>
> Previously the flow cache returned a policy directly which works
> because whenever we modify that policy we'd take the appropriate
> lock.
>
> Your patch changes it so that it now returns an fce. But nothing
> is guarding the code that modifies fce. So two CPUs may end up
> modifying the same fce.
But I changed that. the flow cache now does *not* call local_bh_enable
if it returns something. This is deferred until corresponding _put
call. So bh's are disable while we are touching the lookup results.
It'd probably make sense to remove that. And require _lookup to
be called with bh disabled so it's more obvious that bh's are
disabled when touching the cache entry.
> However, it would appear that this race could be harmless, provided
> that you are careful about dereferencing fce->policy and fce->dst.
>
> IOW, this is not OK
>
> if (fce->policy)
> use fce->policy;
>
> and this should work
>
> policy = fce->policy;
> if (policy)
> use policy;
Not a race. We need to keep bh's disabled while touching fce
for various reasons.
> Actually on second tought, even this isn't totally safe. Who
> is taking a reference count on the policy and dst? I see a ref
> count on the fce, but nothing on fce->dst and fce->policy. Do
> you have an implicit reference on them?
Noone. When policy and dst is on cache there's no reference.
The cache generation id's ensure that the object exists when
they are in cache. It might make sense to add references to
both objects and do a BUG_ON if the flow cache flusher would
need to delete an object. I guess this would be the proper
way, since that's how the dst stuff works too.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-19 8:47 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA337E6.4010508@iki.fi>
On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote:
>
> But I changed that. the flow cache now does *not* call local_bh_enable
> if it returns something. This is deferred until corresponding _put
> call. So bh's are disable while we are touching the lookup results.
I'm sorry but making a function like flow_cache_lookup return with
BH disabled is just wrong!
> It'd probably make sense to remove that. And require _lookup to
> be called with bh disabled so it's more obvious that bh's are
> disabled when touching the cache entry.
That would be better but it's still hacky. Proper reference
counting like we had before would be my preference.
> Not a race. We need to keep bh's disabled while touching fce
> for various reasons.
What are those reasons (apart from this race)?
> Noone. When policy and dst is on cache there's no reference.
> The cache generation id's ensure that the object exists when
> they are in cache. It might make sense to add references to
> both objects and do a BUG_ON if the flow cache flusher would
> need to delete an object. I guess this would be the proper
> way, since that's how the dst stuff works too.
The cache genid is not enough:
CPU1 CPU2
check genid == OK
update genid
kill policy
kfree on policy
use policy == BOOM
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] ipv4: check rt_genid in dst_check
From: Herbert Xu @ 2010-03-19 8:56 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev, timo.teras
In-Reply-To: <1268978083-32070-1-git-send-email-timo.teras@iki.fi>
Timo Teras <timo.teras@iki.fi> wrote:
>
> @@ -1726,7 +1726,9 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
>
> static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
> {
> - return NULL;
> + if (dst && rt_is_expired((struct rtable *)dst))
> + return NULL;
Sorry I didn't spot this the first time around, but the dst check
is redundant too. Since the only path leading to this code is
static inline struct dst_entry *dst_check(struct dst_entry *dst, u32 cookie)
{
if (dst->obsolete)
dst = dst->ops->check(dst, cookie);
return dst;
}
I'll send a patch to remove the same check on the IPv6 path.
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] ipv4: check rt_genid in dst_check
From: Herbert Xu @ 2010-03-19 9:00 UTC (permalink / raw)
To: Timo Teras, David S. Miller; +Cc: netdev
In-Reply-To: <20100319085600.GA23705@gondor.apana.org.au>
On Fri, Mar 19, 2010 at 04:56:00PM +0800, Herbert Xu wrote:
>
> I'll send a patch to remove the same check on the IPv6 path.
ipv6: Remove redundant dst NULL check in ip6_dst_check
As the only path leading to ip6_dst_check makes an indirect call
through dst->ops, dst cannot be NULL in ip6_dst_check.
This patch removes this check in case it misleads people who
come across this code.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 52cd3ef..7fcb0e5 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -879,7 +879,7 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie)
rt = (struct rt6_info *) dst;
- if (rt && rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
+ if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie))
return dst;
return NULL;
Thanks,
--
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 related
* Re: [PATCH 2/3] can: add support for Janz VMOD-ICAN3 Intelligent CAN module
From: Wolfgang Grandegger @ 2010-03-19 9:01 UTC (permalink / raw)
To: Ira W. Snyder
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, sameo-VuQAYsv1563Yd54FQh9/CA
In-Reply-To: <1268930324-29841-3-git-send-email-iws-lulEs6mt1IksTUYHLfqkUA@public.gmane.org>
Hi Ira,
we already discussed this patch on the SocketCAN mailing list and there
are just a few minor issues and the request to add support for the new
"berr-reporting" option, if feasible. See:
commit 52c793f24054f5dc30d228e37e0e19cc8313f086
Author: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
Date: Mon Feb 22 22:21:17 2010 +0000
can: netlink support for bus-error reporting and counters
This patch makes the bus-error reporting configurable and allows to
retrieve the CAN TX and RX bus error counters via netlink interface.
I have added support for the SJA1000. The TX and RX bus error counters
are also copied to the data fields 6..7 of error messages when state
changes are reported.
Should not be a big deal.
More inline...
Ira W. Snyder wrote:
> The Janz VMOD-ICAN3 is a MODULbus daughterboard which fits onto any
> MODULbus carrier board. It is an intelligent CAN controller with a
> microcontroller and associated firmware.
>
> Signed-off-by: Ira W. Snyder <iws-lulEs6mt1IksTUYHLfqkUA@public.gmane.org>
> Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w@public.gmane.org
> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> drivers/net/can/Kconfig | 10 +
> drivers/net/can/Makefile | 1 +
> drivers/net/can/janz-ican3.c | 1659 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1670 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/can/janz-ican3.c
>
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 05b7517..2c5227c 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -63,6 +63,16 @@ config CAN_BFIN
> To compile this driver as a module, choose M here: the
> module will be called bfin_can.
>
> +config CAN_JANZ_ICAN3
> + tristate "Janz VMOD-ICAN3 Intelligent CAN controller"
> + depends on CAN_DEV && MFD_JANZ_CMODIO
> + ---help---
> + Driver for Janz VMOD-ICAN3 Intelligent CAN controller module, which
> + connects to a MODULbus carrier board.
> +
> + This driver can also be built as a module. If so, the module will be
> + called janz-ican3.ko.
> +
> source "drivers/net/can/mscan/Kconfig"
>
> source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 7a702f2..9047cd0 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -15,5 +15,6 @@ obj-$(CONFIG_CAN_AT91) += at91_can.o
> obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o
> obj-$(CONFIG_CAN_MCP251X) += mcp251x.o
> obj-$(CONFIG_CAN_BFIN) += bfin_can.o
> +obj-$(CONFIG_CAN_JANZ_ICAN3) += janz-ican3.o
>
> ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> new file mode 100644
> index 0000000..94d4995
> --- /dev/null
> +++ b/drivers/net/can/janz-ican3.c
[snip]
+struct ican3_dev {
> +
> + /* must be the first member */
> + struct can_priv can;
> +
> + /* CAN network device */
> + struct net_device *ndev;
> + struct napi_struct napi;
> +
> + /* Device for printing */
> + struct device *dev;
> +
> + /* module number */
> + unsigned int num;
> +
> + /* base address of registers and IRQ */
> + struct janz_cmodio_onboard_regs __iomem *ctrl;
> + struct ican3_dpm_control *dpmctrl;
> + void __iomem *dpm;
> + int irq;
> +
> + /* old and new style host interface */
> + unsigned int iftype;
> + spinlock_t lock;
Please describe what the lock is used for.
> + /* new host interface */
> + unsigned int rx_int;
> + unsigned int rx_num;
> + unsigned int tx_num;
> +
> + /* fast host interface */
> + unsigned int fastrx_start;
> + unsigned int fastrx_int;
> + unsigned int fastrx_num;
> + unsigned int fasttx_start;
> + unsigned int fasttx_num;
> +
> + /* first free DPM page */
> + unsigned int free_page;
> +};
[snip]
> +static void ican3_to_can_frame(struct ican3_dev *mod,
> + struct ican3_fast_desc *desc,
> + struct can_frame *cf)
> +{
> + if ((desc->command & ICAN3_CAN_TYPE_MASK) == ICAN3_CAN_TYPE_SFF) {
> + dev_dbg(mod->dev, "%s: old frame format\n", __func__);
This prints a debug message for every message which is not really
useful for the user. Please remove.
> + if (desc->data[1] & ICAN3_SFF_RTR)
> + cf->can_id |= CAN_RTR_FLAG;
> +
> + cf->can_id |= desc->data[0] << 3;
> + cf->can_id |= (desc->data[1] & 0xe0) >> 5;
> + cf->can_dlc = desc->data[1] & ICAN3_CAN_DLC_MASK;
> + memcpy(cf->data, &desc->data[2], sizeof(cf->data));
> + } else {
> + dev_dbg(mod->dev, "%s: new frame format\n", __func__);
Ditto.
> + cf->can_dlc = desc->data[0] & ICAN3_CAN_DLC_MASK;
> + if (desc->data[0] & ICAN3_EFF_RTR)
> + cf->can_id |= CAN_RTR_FLAG;
> +
> + if (desc->data[0] & ICAN3_EFF) {
> + cf->can_id |= CAN_EFF_FLAG;
> + cf->can_id |= desc->data[2] << 21; /* 28-21 */
> + cf->can_id |= desc->data[3] << 13; /* 20-13 */
> + cf->can_id |= desc->data[4] << 5; /* 12-5 */
> + cf->can_id |= (desc->data[5] & 0xf8) >> 3;
> + } else {
> + cf->can_id |= desc->data[2] << 3; /* 10-3 */
> + cf->can_id |= desc->data[3] >> 5; /* 2-0 */
> + }
> +
> + memcpy(cf->data, &desc->data[6], sizeof(cf->data));
> + }
> +}
> +
> +static void can_frame_to_ican3(struct ican3_dev *mod,
> + struct can_frame *cf,
> + struct ican3_fast_desc *desc)
> +{
> + /* clear out any stale data in the descriptor */
> + memset(desc->data, 0, sizeof(desc->data));
> +
> + /* we always use the extended format, with the ECHO flag set */
> + desc->command = ICAN3_CAN_TYPE_EFF;
> + desc->data[0] |= cf->can_dlc;
> + desc->data[1] |= ICAN3_ECHO;
> +
> + if (cf->can_id & CAN_RTR_FLAG)
> + desc->data[0] |= ICAN3_EFF_RTR;
> +
> + /* pack the id into the correct places */
> + if (cf->can_id & CAN_EFF_FLAG) {
> + dev_dbg(mod->dev, "%s: extended frame\n", __func__);
Ditto.
> + desc->data[0] |= ICAN3_EFF;
> + desc->data[2] = (cf->can_id & 0x1fe00000) >> 21; /* 28-21 */
> + desc->data[3] = (cf->can_id & 0x001fe000) >> 13; /* 20-13 */
> + desc->data[4] = (cf->can_id & 0x00001fe0) >> 5; /* 12-5 */
> + desc->data[5] = (cf->can_id & 0x0000001f) << 3; /* 4-0 */
> + } else {
> + dev_dbg(mod->dev, "%s: standard frame\n", __func__);
Ditto.
> + desc->data[2] = (cf->can_id & 0x7F8) >> 3; /* bits 10-3 */
> + desc->data[3] = (cf->can_id & 0x007) << 5; /* bits 2-0 */
> + }
> +
> + /* copy the data bits into the descriptor */
> + memcpy(&desc->data[6], cf->data, sizeof(cf->data));
> +}
[snip]
> +/*
> + * Handle CAN Event Indication Messages from the firmware
> + *
> + * The ICAN3 firmware provides the values of some SJA1000 registers when it
> + * generates this message. The code below is largely copied from the
> + * drivers/net/can/sja1000/sja1000.c file, and adapted as necessary
> + */
> +static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> +{
> + struct net_device *dev = mod->ndev;
> + struct net_device_stats *stats = &dev->stats;
> + enum can_state state = mod->can.state;
> + struct can_frame *cf;
> + struct sk_buff *skb;
> + u8 status, isrc;
> +
> + /* we can only handle the SJA1000 part */
> + if (msg->data[1] != CEVTIND_CHIP_SJA1000) {
> + dev_err(mod->dev, "unable to handle errors on non-SJA1000\n");
> + return -ENODEV;
> + }
> +
> + /* check the message length for sanity */
> + if (msg->len < 6) {
> + dev_err(mod->dev, "error message too short\n");
> + return -EINVAL;
> + }
> +
> + skb = alloc_can_err_skb(dev, &cf);
> + if (skb == NULL)
> + return -ENOMEM;
> +
> + isrc = msg->data[0];
> + status = msg->data[3];
> +
> + /* data overrun interrupt */
> + if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
Here and for the other errors below a dev_dbg() would be useful. Please
check sja1000.c.
> + cf->can_id |= CAN_ERR_CRTL;
> + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> + stats->rx_over_errors++;
> + stats->rx_errors++;
> + dev_info(mod->dev, "%s: overflow frame generated\n", __func__);
s/dev_info/dev_dbg/ ?
> + }
> +
> + /* error warning interrupt */
> + if (isrc == CEVTIND_EI) {
> + if (status & SR_BS) {
> + state = CAN_STATE_BUS_OFF;
> + cf->can_id |= CAN_ERR_BUSOFF;
> + can_bus_off(dev);
> + } else if (status & SR_ES) {
> + state = CAN_STATE_ERROR_WARNING;
> + } else {
> + state = CAN_STATE_ERROR_ACTIVE;
> + }
> + }
> +
> + /* bus error interrupt */
> + if (isrc == CEVTIND_BEI) {
> + u8 ecc = msg->data[2];
Add an empty line, please.
> + mod->can.can_stats.bus_error++;
> + stats->rx_errors++;
> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> + switch (ecc & ECC_MASK) {
> + case ECC_BIT:
> + cf->data[2] |= CAN_ERR_PROT_BIT;
> + break;
> + case ECC_FORM:
> + cf->data[2] |= CAN_ERR_PROT_FORM;
> + break;
> + case ECC_STUFF:
> + cf->data[2] |= CAN_ERR_PROT_STUFF;
> + break;
> + default:
> + cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> + cf->data[3] = ecc & ECC_SEG;
> + break;
> + }
> +
> + if ((ecc & ECC_DIR) == 0)
> + cf->data[2] |= CAN_ERR_PROT_TX;
> + }
> +
> + if (state != mod->can.state && (state == CAN_STATE_ERROR_WARNING ||
> + state == CAN_STATE_ERROR_PASSIVE)) {
> + u8 rxerr = msg->data[4];
> + u8 txerr = msg->data[5];
Ditto.
> + cf->can_id |= CAN_ERR_CRTL;
> + if (state == CAN_STATE_ERROR_WARNING) {
> + mod->can.can_stats.error_warning++;
> + cf->data[1] = (txerr > rxerr) ?
> + CAN_ERR_CRTL_TX_WARNING :
> + CAN_ERR_CRTL_RX_WARNING;
> + } else {
> + mod->can.can_stats.error_passive++;
> + cf->data[1] = (txerr > rxerr) ?
> + CAN_ERR_CRTL_TX_PASSIVE :
> + CAN_ERR_CRTL_RX_PASSIVE;
> + }
> + }
> +
> + mod->can.state = state;
> + stats->rx_errors++;
> + stats->rx_bytes += cf->can_dlc;
> + netif_rx(skb);
> + return 0;
> +}
[snip]
> +static irqreturn_t ican3_irq(int irq, void *dev_id)
> +{
> + struct ican3_dev *mod = dev_id;
> + u8 stat;
> +
> + /*
> + * The interrupt status register on this device reports interrupts
> + * as zeroes instead of using ones like most other devices
> + */
> + stat = ioread8(&mod->ctrl->int_disable) & (1 << mod->num);
> + if (stat == (1 << mod->num))
> + return IRQ_NONE;
> +
> + dev_dbg(mod->dev, "IRQ: module %d\n", mod->num);
Please remove this dev_dbg() as well.
[snip]
> +/*
> + * Startup an ICAN module, bringing it into fast mode
> + */
> +static int __devinit ican3_startup_module(struct ican3_dev *mod)
> +{
> + int ret;
> +
> + ret = ican3_reset_module(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to reset module\n");
> + return ret;
> + }
> +
> + /* re-enable interrupts so we can send messages */
> + iowrite8(1 << mod->num, &mod->ctrl->int_enable);
> +
> + ret = ican3_msg_connect(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to connect to module\n");
> + return ret;
> + }
> +
> + ican3_init_new_host_interface(mod);
> + ret = ican3_msg_newhostif(mod);
> + if (ret) {
> + dev_err(mod->dev, "unable to switch to new-style interface\n");
> + return ret;
> + }
> +
> + ret = ican3_set_termination(mod, true);
> + if (ret) {
> + dev_err(mod->dev, "unable to enable termination\n");
> + return ret;
> + }
Could you please allow the user to disable termination, e.g. via module parameter
"bus_termination=0" in case he uses a cable with built-in termination.
[snip]
> +static int __devinit ican3_probe(struct platform_device *pdev)
> +{
> + struct janz_platform_data *pdata;
> + struct net_device *ndev;
> + struct ican3_dev *mod;
> + struct resource *res;
> + struct device *dev;
> + int ret;
> +
> + pdata = pdev->dev.platform_data;
> + if (!pdata)
> + return -ENXIO;
> +
> + dev_dbg(&pdev->dev, "probe: module number %d\n", pdata->modno);
> +
> + /* save the struct device for printing */
> + dev = &pdev->dev;
> +
> + /* allocate the CAN device and private data */
> + ndev = alloc_candev(sizeof(*mod), 0);
> + if (!ndev) {
> + dev_err(dev, "unable to allocate CANdev\n");
> + ret = -ENOMEM;
> + goto out_return;
> + }
> +
> + platform_set_drvdata(pdev, ndev);
> + mod = netdev_priv(ndev);
> + mod->ndev = ndev;
> + mod->dev = &pdev->dev;
> + mod->num = pdata->modno;
> + netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
> + spin_lock_init(&mod->lock);
> +
> + /* the first unallocated page in the DPM is 9 */
> + mod->free_page = DPM_FREE_START;
> +
> + ndev->netdev_ops = &ican3_netdev_ops;
> + ndev->flags |= IFF_ECHO;
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> + mod->can.clock.freq = 8000000;
Please use a constant here.
[snip]
Please fix and resubmit with my:
"Acked-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>"
for the SocketCAN part.
Thanks,
Wolfgang.
^ permalink raw reply
* [PATCH] pktgen node allocation
From: Robert Olsson @ 2010-03-19 8:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, robert
Hi,
Here is patch to manipulate packet node allocation and implicitly
how packets are DMA'd etc.
The flag NODE_ALLOC enables the function and numa_node_id();
when enabled it can also be explicitly controlled via a new
node parameter
Tested this with 10 Intel 82599 ports w. TYAN S7025 E5520 CPU's.
Was able to TX/DMA ~80 Gbit/s to Ethernet wires.
Cheers
--ro
Signed-off-by: Robert Olsson <robert.olsson@its.uu.se>
diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 4392381..c195fd0 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -169,7 +169,7 @@
#include <asm/dma.h>
#include <asm/div64.h> /* do_div */
-#define VERSION "2.72"
+#define VERSION "2.73"
#define IP_NAME_SZ 32
#define MAX_MPLS_LABELS 16 /* This is the max label stack depth */
#define MPLS_STACK_BOTTOM htonl(0x00000100)
@@ -190,6 +190,7 @@
#define F_IPSEC_ON (1<<12) /* ipsec on for flows */
#define F_QUEUE_MAP_RND (1<<13) /* queue map Random */
#define F_QUEUE_MAP_CPU (1<<14) /* queue map mirrors smp_processor_id() */
+#define F_NODE (1<<15) /* Node memory alloc*/
/* Thread control flag bits */
#define T_STOP (1<<0) /* Stop run */
@@ -372,6 +373,7 @@ struct pktgen_dev {
u16 queue_map_min;
u16 queue_map_max;
+ int node; /* Memory node */
#ifdef CONFIG_XFRM
__u8 ipsmode; /* IPSEC mode (config) */
@@ -607,6 +609,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
if (pkt_dev->traffic_class)
seq_printf(seq, " traffic_class: 0x%02x\n", pkt_dev->traffic_class);
+ if (pkt_dev->node >= 0)
+ seq_printf(seq, " node: %d\n", pkt_dev->node);
+
seq_printf(seq, " Flags: ");
if (pkt_dev->flags & F_IPV6)
@@ -660,6 +665,9 @@ static int pktgen_if_show(struct seq_file *seq, void *v)
if (pkt_dev->flags & F_SVID_RND)
seq_printf(seq, "SVID_RND ");
+ if (pkt_dev->flags & F_NODE)
+ seq_printf(seq, "NODE_ALLOC ");
+
seq_puts(seq, "\n");
/* not really stopped, more like last-running-at */
@@ -1074,6 +1082,21 @@ static ssize_t pktgen_if_write(struct file *file,
pkt_dev->dst_mac_count);
return count;
}
+ if (!strcmp(name, "node")) {
+ len = num_arg(&user_buffer[i], 10, &value);
+ if (len < 0)
+ return len;
+
+ i += len;
+
+ if(node_possible(value)) {
+ pkt_dev->node = value;
+ sprintf(pg_result, "OK: node=%d", pkt_dev->node);
+ }
+ else
+ sprintf(pg_result, "ERROR: node not possible");
+ return count;
+ }
if (!strcmp(name, "flag")) {
char f[32];
memset(f, 0, 32);
@@ -1166,12 +1189,18 @@ static ssize_t pktgen_if_write(struct file *file,
else if (strcmp(f, "!IPV6") == 0)
pkt_dev->flags &= ~F_IPV6;
+ else if (strcmp(f, "NODE_ALLOC") == 0)
+ pkt_dev->flags |= F_NODE;
+
+ else if (strcmp(f, "!NODE_ALLOC") == 0)
+ pkt_dev->flags &= ~F_NODE;
+
else {
sprintf(pg_result,
"Flag -:%s:- unknown\nAvailable flags, (prepend ! to un-set flag):\n%s",
f,
"IPSRC_RND, IPDST_RND, UDPSRC_RND, UDPDST_RND, "
- "MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, IPSEC\n");
+ "MACSRC_RND, MACDST_RND, TXSIZE_RND, IPV6, MPLS_RND, VID_RND, SVID_RND, FLOW_SEQ, IPSEC, NODE_ALLOC\n");
return count;
}
sprintf(pg_result, "OK: flags=0x%x", pkt_dev->flags);
@@ -2572,9 +2601,27 @@ static struct sk_buff *fill_packet_ipv4(struct net_device *odev,
mod_cur_headers(pkt_dev);
datalen = (odev->hard_header_len + 16) & ~0xf;
- skb = __netdev_alloc_skb(odev,
- pkt_dev->cur_pkt_size + 64
- + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT);
+
+ if(pkt_dev->flags & F_NODE) {
+ int node;
+
+ if(pkt_dev->node >= 0)
+ node = pkt_dev->node;
+ else
+ node = numa_node_id();
+
+ skb = __alloc_skb(NET_SKB_PAD + pkt_dev->cur_pkt_size + 64
+ + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT, 0, node);
+ if (likely(skb)) {
+ skb_reserve(skb, NET_SKB_PAD);
+ skb->dev = odev;
+ }
+ }
+ else
+ skb = __netdev_alloc_skb(odev,
+ pkt_dev->cur_pkt_size + 64
+ + datalen + pkt_dev->pkt_overhead, GFP_NOWAIT);
+
if (!skb) {
sprintf(pkt_dev->result, "No memory");
return NULL;
@@ -3674,6 +3721,7 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
pkt_dev->svlan_p = 0;
pkt_dev->svlan_cfi = 0;
pkt_dev->svlan_id = 0xffff;
+ pkt_dev->node = -1;
err = pktgen_setup_dev(pkt_dev, ifname);
if (err)
^ permalink raw reply related
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-19 9:12 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100319084717.GA23567@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 10:37:58AM +0200, Timo Teräs wrote:
>> But I changed that. the flow cache now does *not* call local_bh_enable
>> if it returns something. This is deferred until corresponding _put
>> call. So bh's are disable while we are touching the lookup results.
>
> I'm sorry but making a function like flow_cache_lookup return with
> BH disabled is just wrong!
>
>> It'd probably make sense to remove that. And require _lookup to
>> be called with bh disabled so it's more obvious that bh's are
>> disabled when touching the cache entry.
>
> That would be better but it's still hacky. Proper reference
> counting like we had before would be my preference.
Well, the cache entry is still referenced only very shortly,
I don't see why keeping bh disabled why doing it is considered
a hack. Refcounting the cache entries is trickier. Though,
it could be used to optimize the update process: we could safely
update it instead of doing now lookup later.
>> Not a race. We need to keep bh's disabled while touching fce
>> for various reasons.
>
> What are those reasons (apart from this race)?
This. And that the cache is synchronized by flow_cache_flush
executing stuff on other cpu's, ensuring that it's not running
any protected cache accessing code. See below.
>
>> Noone. When policy and dst is on cache there's no reference.
>> The cache generation id's ensure that the object exists when
>> they are in cache. It might make sense to add references to
>> both objects and do a BUG_ON if the flow cache flusher would
>> need to delete an object. I guess this would be the proper
>> way, since that's how the dst stuff works too.
>
> The cache genid is not enough:
>
> CPU1 CPU2
> check genid == OK
> update genid
> kill policy
> kfree on policy
> use policy == BOOM
The sequence goes currently.
CPU1 CPU2
disable_bh
check genid == OK
update genid
call cache_flush
blocks
use policy == OK
and take refcount
enable_bh
cache_flush smpcall executes and ublocks cpu2
returns from cache_flush
kill policy
kfree on policy
- Timo
^ permalink raw reply
* Re: [PATCH 1/3] mfd: add support for Janz CMOD-IO PCI MODULbus Carrier Board
From: Wolfgang Grandegger @ 2010-03-19 9:13 UTC (permalink / raw)
To: Ira W. Snyder; +Cc: linux-kernel, socketcan-core, netdev, sameo
In-Reply-To: <1268930324-29841-2-git-send-email-iws@ovro.caltech.edu>
Ira W. Snyder wrote:
> The Janz CMOD-IO PCI MODULbus carrier board is a PCI to MODULbus bridge,
> which may host many different types of MODULbus daughterboards, including
> CAN and GPIO controllers.
>
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
You can add my "Reviewed-by: Wolfgang Grandegger <wg@grandegger.com>".
Just one concern:
> ---
> drivers/mfd/Kconfig | 8 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/janz-cmodio.c | 339 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/janz.h | 54 +++++++
> 4 files changed, 402 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mfd/janz-cmodio.c
> create mode 100644 include/linux/mfd/janz.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8782978..f1858d7 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -27,6 +27,14 @@ config MFD_SM501_GPIO
> lines on the SM501. The platform data is used to supply the
> base number for the first GPIO line to register.
>
> +config MFD_JANZ_CMODIO
> + tristate "Support for Janz CMOD-IO PCI MODULbus Carrier Board"
> + ---help---
> + This is the core driver for the Janz CMOD-IO PCI MODULbus
> + carrier board. This device is a PCI to MODULbus bridge which may
> + host many different types of MODULbus daughterboards, including
> + CAN and GPIO controllers.
> +
> config MFD_ASIC3
> bool "Support for Compaq ASIC3"
> depends on GENERIC_HARDIRQS && GPIOLIB && ARM
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e09eb48..e8fa905 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -3,6 +3,7 @@
> #
>
> obj-$(CONFIG_MFD_SM501) += sm501.o
> +obj-$(CONFIG_MFD_JANZ_CMODIO) += janz-cmodio.o
> obj-$(CONFIG_MFD_ASIC3) += asic3.o tmio_core.o
> obj-$(CONFIG_MFD_SH_MOBILE_SDHI) += sh_mobile_sdhi.o
>
> diff --git a/drivers/mfd/janz-cmodio.c b/drivers/mfd/janz-cmodio.c
> new file mode 100644
> index 0000000..914280e
> --- /dev/null
> +++ b/drivers/mfd/janz-cmodio.c
> @@ -0,0 +1,339 @@
> +/*
> + * Janz CMOD-IO MODULbus Carrier Board PCI Driver
> + *
> + * Copyright (c) 2010 Ira W. Snyder <iws@ovro.caltech.edu>
> + *
> + * Lots of inspiration and code was copied from drivers/mfd/sm501.c
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/janz.h>
> +
> +#define DRV_NAME "janz-cmodio"
> +
> +/* Size of each MODULbus module in PCI BAR4 */
> +#define CMODIO_MODULBUS_SIZE 0x200
> +
> +/* Maximum number of MODULbus modules on a CMOD-IO carrier board */
> +#define CMODIO_MAX_MODULES 4
> +
> +/* Module Parameters */
> +static unsigned int num_modules = CMODIO_MAX_MODULES;
> +static unsigned char *modules[CMODIO_MAX_MODULES] = {
> + "janz-ican3",
> + "janz-ican3",
> + "",
> + "janz-ttl",
> +};
This is probably not a good default but just your private configuration.
Wolfgang.
^ permalink raw reply
* [PATCH v3] ipv4: check rt_genid in dst_check
From: Timo Teras @ 2010-03-19 9:20 UTC (permalink / raw)
To: netdev; +Cc: Timo Teras
Xfrm_dst keeps a reference to ipv4 rtable entries on each
cached bundle. The only way to renew xfrm_dst when the underlying
route has changed, is to implement dst_check for this. This is
what ipv6 side does too.
The problems started after 87c1e12b5eeb7b30b4b41291bef8e0b41fc3dde9
which fixed a bug causing xfrm_dst to not get reused, until that all
lookups always generated new xfrm_dst with new route reference
and path mtu worked. But after the fix, the old routes started
to get reused even after they were expired causing pmtu to break
(well it would occationally work if the rtable gc had run recently
and marked the route obsolete causing dst_check to get called).
Signed-off-by: Timo Teras <timo.teras@iki.fi>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
net/ipv4/route.c | 14 ++++++++++----
1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index a770df2..32d3961 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1441,7 +1441,7 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
dev_hold(rt->u.dst.dev);
if (rt->idev)
in_dev_hold(rt->idev);
- rt->u.dst.obsolete = 0;
+ rt->u.dst.obsolete = -1;
rt->u.dst.lastuse = jiffies;
rt->u.dst.path = &rt->u.dst;
rt->u.dst.neighbour = NULL;
@@ -1506,7 +1506,7 @@ static struct dst_entry *ipv4_negative_advice(struct dst_entry *dst)
struct dst_entry *ret = dst;
if (rt) {
- if (dst->obsolete) {
+ if (dst->obsolete > 0) {
ip_rt_put(rt);
ret = NULL;
} else if ((rt->rt_flags & RTCF_REDIRECTED) ||
@@ -1726,7 +1726,9 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
{
- return NULL;
+ if (rt_is_expired((struct rtable *)dst))
+ return NULL;
+ return dst;
}
static void ipv4_dst_destroy(struct dst_entry *dst)
@@ -1888,7 +1890,8 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
if (!rth)
goto e_nobufs;
- rth->u.dst.output= ip_rt_bug;
+ rth->u.dst.output = ip_rt_bug;
+ rth->u.dst.obsolete = -1;
atomic_set(&rth->u.dst.__refcnt, 1);
rth->u.dst.flags= DST_HOST;
@@ -2054,6 +2057,7 @@ static int __mkroute_input(struct sk_buff *skb,
rth->fl.oif = 0;
rth->rt_spec_dst= spec_dst;
+ rth->u.dst.obsolete = -1;
rth->u.dst.input = ip_forward;
rth->u.dst.output = ip_output;
rth->rt_genid = rt_genid(dev_net(rth->u.dst.dev));
@@ -2218,6 +2222,7 @@ local_input:
goto e_nobufs;
rth->u.dst.output= ip_rt_bug;
+ rth->u.dst.obsolete = -1;
rth->rt_genid = rt_genid(net);
atomic_set(&rth->u.dst.__refcnt, 1);
@@ -2444,6 +2449,7 @@ static int __mkroute_output(struct rtable **result,
rth->rt_spec_dst= fl->fl4_src;
rth->u.dst.output=ip_output;
+ rth->u.dst.obsolete = -1;
rth->rt_genid = rt_genid(dev_net(dev_out));
RT_CACHE_STAT_INC(out_slow_tot);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] pktgen node allocation
From: Eric Dumazet @ 2010-03-19 9:28 UTC (permalink / raw)
To: Robert Olsson; +Cc: David Miller, netdev
In-Reply-To: <19363.14702.909265.380669@gargle.gargle.HOWL>
Le vendredi 19 mars 2010 à 09:44 +0100, Robert Olsson a écrit :
>
> Hi,
> Here is patch to manipulate packet node allocation and implicitly
> how packets are DMA'd etc.
>
> The flag NODE_ALLOC enables the function and numa_node_id();
> when enabled it can also be explicitly controlled via a new
> node parameter
>
> Tested this with 10 Intel 82599 ports w. TYAN S7025 E5520 CPU's.
> Was able to TX/DMA ~80 Gbit/s to Ethernet wires.
>
> Cheers
> --ro
>
I cannot understand how this can help.
__netdev_alloc_skb() is supposed to already take into account NUMA
properties :
int node = dev->dev.parent ? dev_to_node(dev->dev.parent) : -1;
If this doesnt work, we should correct core stack, not only pktgen :)
Are you allocating memory in the node where pktgen CPU is running or the
node close to the NIC ?
Thanks
^ 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