* Re: Re[2]: [PATCH v7] PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)
From: Eric Dumazet @ 2010-08-20 15:50 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: netdev
In-Reply-To: <E1OmTW8-0002E6-00.xeb-mail-ru@f289.mail.ru>
Le vendredi 20 août 2010 à 19:28 +0400, Dmitry Kozlov a écrit :
> > > Ang again...
> > > This patch contains:
> > > 1. pptp driver
> > > 2. gre demultiplexer driver for demultiplexing gre packets with different gre version
> > > so ip_gre and pptp may coexists
> > > 3. ip_gre modification
> > > 4. other stuff
> > >
> > > Changes from v6:
> > > 1. memory allocation moved to begin of module initialization
> > > 2. fixed coding style issues
> > > Thanks to Eric Dumazet.
> > >
> > > --
> > > MAINTAINERS | 14 +
> > > drivers/net/Kconfig | 11 +
> > > drivers/net/Makefile | 1 +
> > > drivers/net/pptp.c | 726 ++++++++++++++++++++++++++++++++++++++++++++++
> > > include/linux/if_pppox.h | 59 +++--
> > > include/net/gre.h | 18 ++
> > > net/ipv4/Kconfig | 7 +
> > > net/ipv4/Makefile | 1 +
> > > net/ipv4/gre.c | 151 ++++++++++
> > > net/ipv4/ip_gre.c | 14 +-
> > > 10 files changed, 975 insertions(+), 27 deletions(-)
> >
> >
> > Seems fine to me, but you need to make a nice looking Changelog and add
> > your "Signed-off-by: ...." signature.
> >
> > Thanks
>
> What do you mean, Changelog of patch ?
I mean the head of your patch that read :
-----------------------------------------------------------------
Ang again...
This patch contains:
1. pptp driver
2. gre demultiplexer driver for demultiplexing gre packets with
different gre version
so ip_gre and pptp may coexists
3. ip_gre modification
4. other stuff
Changes from v6:
1. memory allocation moved to begin of module initialization
2. fixed coding style issues
Thanks to Eric Dumazet.
-------------------------------------------------------------------
This is not a very good Changelog for the final code inclusion in
Linux :). You spent a lot of time in preparing these patches, you can
polish the text so that casual reader have an idea of what you did...
'Ang again...' ?
Dont include "changes from v6" or include all changes (v2,v3, ...)
Of course, once I reviewed your final version, I'll add my signature,
and other netdev developpers (including David S. Miller) might find
other problems that I missed, so what I called 'final code' might be a
not so final...
Thanks
^ permalink raw reply
* Re: Re[2]: [PATCH v7] PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)
From: Eric Dumazet @ 2010-08-20 15:59 UTC (permalink / raw)
To: Dmitry Kozlov; +Cc: netdev
In-Reply-To: <1282319459.2484.249.camel@edumazet-laptop>
Le vendredi 20 août 2010 à 17:51 +0200, Eric Dumazet a écrit :
> Dont include "changes from v6" or include all changes (v2,v3, ...)
The usual way of documenting changes so that they dont pollute the
permanent changelog is adding text after the "---" separator :
[PATCH v8] PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)
The fine changelog that will be stored in git
Signed-off-by: The fine author bbbb@bbbb.com
---
Hello everybody ! (not stored in git history)
Changes from v2 : the fine changes
I hope this is the last round :)
diff....
Check Documentation/SubmittingPatches line 492
^ permalink raw reply
* [PATCH 1/2] netfilter: fix the hash random initializing race
From: Changli Gao @ 2010-08-20 16:06 UTC (permalink / raw)
To: Patrick McHardy
Cc: Eric Dumazet, David S. Miller, netfilter-devel, netdev,
Changli Gao
nf_conntrack_alloc() isn't called with nf_conntrack_lock locked, so hash
random initializing code maybe executed more than once on different CPUs.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
net/netfilter/nf_conntrack_core.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index df3eedb..7ae7f71 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -65,8 +65,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max);
DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
-static int nf_conntrack_hash_rnd_initted;
-static unsigned int nf_conntrack_hash_rnd;
+static unsigned int nf_conntrack_hash_rnd __read_mostly;
static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
u16 zone, unsigned int size, unsigned int rnd)
@@ -574,10 +573,16 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
{
struct nf_conn *ct;
- if (unlikely(!nf_conntrack_hash_rnd_initted)) {
- get_random_bytes(&nf_conntrack_hash_rnd,
- sizeof(nf_conntrack_hash_rnd));
- nf_conntrack_hash_rnd_initted = 1;
+ if (unlikely(!nf_conntrack_hash_rnd)) {
+ unsigned int rand;
+
+ /* Why not initialize nf_conntrack_rnd in a "init()" function ?
+ * Because there isn't enough entropy when system initializing,
+ * and we initialize it as late as possible. */
+ do {
+ get_random_bytes(&rand, sizeof(rand));
+ } while (!rand);
+ cmpxchg(&nf_conntrack_hash_rnd, 0, rand);
}
/* We don't want any race condition at early drop stage */
^ permalink raw reply related
* [PATCH v4 2/2] netfilter: save the hash of the tuple in the original direction for latter use
From: Changli Gao @ 2010-08-20 16:07 UTC (permalink / raw)
To: Patrick McHardy
Cc: David S. Miller, Eric Dumazet, Mathieu Desnoyers, akpm,
netfilter-devel, netdev, linux-kernel, Changli Gao
Since we don't change the tuple in the original direction, we can save it
in ct->tuplehash[IP_CT_DIR_REPLY].hnode.pprev for __nf_conntrack_confirm()
use.
__hash_conntrack() is split into two steps: ____hash_conntrack() is used
to get the raw hash, and __hash_bucket() is used to get the bucket id.
In SYN-flood case, early_drop() doesn't need to recompute the hash again.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v4: init rnd when allocating conntrack.
v3: define static variable rnd out of the function ____hash_conntrack(),
and call get_random_bytes() until we get a non-zero random int.
v2: use cmpxchg() to save 2 variables.
net/netfilter/nf_conntrack_core.c | 104 +++++++++++++++++++++++++++-----------
1 file changed, 75 insertions(+), 29 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 7ae7f71..ca6b6ab 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -67,8 +67,7 @@ EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
static unsigned int nf_conntrack_hash_rnd __read_mostly;
-static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
- u16 zone, unsigned int size, unsigned int rnd)
+static u32 ____hash_conntrack(const struct nf_conntrack_tuple *tuple, u16 zone)
{
unsigned int n;
u_int32_t h;
@@ -78,18 +77,33 @@ static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
* three bytes manually.
*/
n = (sizeof(tuple->src) + sizeof(tuple->dst.u3)) / sizeof(u32);
- h = jhash2((u32 *)tuple, n,
- zone ^ rnd ^ (((__force __u16)tuple->dst.u.all << 16) |
- tuple->dst.protonum));
+ h = jhash2((u32 *)tuple, n, zone ^ nf_conntrack_hash_rnd ^
+ (((__force __u16)tuple->dst.u.all << 16) |
+ tuple->dst.protonum));
+
+ return h;
+}
+
+static u32 __hash_bucket(u32 __hash, unsigned int size)
+{
+ return ((u64)__hash * size) >> 32;
+}
+
+static u32 hash_bucket(u32 __hash, const struct net *net)
+{
+ return __hash_bucket(__hash, net->ct.htable_size);
+}
- return ((u64)h * size) >> 32;
+static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
+ u16 zone, unsigned int size)
+{
+ return __hash_bucket(____hash_conntrack(tuple, zone), size);
}
static inline u_int32_t hash_conntrack(const struct net *net, u16 zone,
const struct nf_conntrack_tuple *tuple)
{
- return __hash_conntrack(tuple, zone, net->ct.htable_size,
- nf_conntrack_hash_rnd);
+ return __hash_conntrack(tuple, zone, net->ct.htable_size);
}
bool
@@ -291,13 +305,13 @@ static void death_by_timeout(unsigned long ul_conntrack)
* OR
* - Caller must lock nf_conntrack_lock before calling this function
*/
-struct nf_conntrack_tuple_hash *
-__nf_conntrack_find(struct net *net, u16 zone,
- const struct nf_conntrack_tuple *tuple)
+static struct nf_conntrack_tuple_hash *
+____nf_conntrack_find(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *tuple, u32 __hash)
{
struct nf_conntrack_tuple_hash *h;
struct hlist_nulls_node *n;
- unsigned int hash = hash_conntrack(net, zone, tuple);
+ unsigned int hash = hash_bucket(__hash, net);
/* Disable BHs the entire time since we normally need to disable them
* at least once for the stats anyway.
@@ -326,19 +340,27 @@ begin:
return NULL;
}
+
+struct nf_conntrack_tuple_hash *
+__nf_conntrack_find(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *tuple)
+{
+ return ____nf_conntrack_find(net, zone, tuple,
+ ____hash_conntrack(tuple, zone));
+}
EXPORT_SYMBOL_GPL(__nf_conntrack_find);
/* Find a connection corresponding to a tuple. */
-struct nf_conntrack_tuple_hash *
-nf_conntrack_find_get(struct net *net, u16 zone,
- const struct nf_conntrack_tuple *tuple)
+static struct nf_conntrack_tuple_hash *
+__nf_conntrack_find_get(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *tuple, u32 __hash)
{
struct nf_conntrack_tuple_hash *h;
struct nf_conn *ct;
rcu_read_lock();
begin:
- h = __nf_conntrack_find(net, zone, tuple);
+ h = ____nf_conntrack_find(net, zone, tuple, __hash);
if (h) {
ct = nf_ct_tuplehash_to_ctrack(h);
if (unlikely(nf_ct_is_dying(ct) ||
@@ -356,6 +378,14 @@ begin:
return h;
}
+
+struct nf_conntrack_tuple_hash *
+nf_conntrack_find_get(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *tuple)
+{
+ return __nf_conntrack_find_get(net, zone, tuple,
+ ____hash_conntrack(tuple, zone));
+}
EXPORT_SYMBOL_GPL(nf_conntrack_find_get);
static void __nf_conntrack_hash_insert(struct nf_conn *ct,
@@ -408,7 +438,8 @@ __nf_conntrack_confirm(struct sk_buff *skb)
return NF_ACCEPT;
zone = nf_ct_zone(ct);
- hash = hash_conntrack(net, zone, &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
+ /* reuse the __hash saved before */
+ hash = hash_bucket(*(unsigned long *)&ct->tuplehash[IP_CT_DIR_REPLY].hnnode.pprev, net);
repl_hash = hash_conntrack(net, zone, &ct->tuplehash[IP_CT_DIR_REPLY].tuple);
/* We're not in hash table, and we refuse to set up related
@@ -566,10 +597,11 @@ static noinline int early_drop(struct net *net, unsigned int hash)
return dropped;
}
-struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
- const struct nf_conntrack_tuple *orig,
- const struct nf_conntrack_tuple *repl,
- gfp_t gfp)
+static struct nf_conn *
+__nf_conntrack_alloc(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *orig,
+ const struct nf_conntrack_tuple *repl,
+ gfp_t gfp, u32 __hash)
{
struct nf_conn *ct;
@@ -583,6 +615,9 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
get_random_bytes(&rand, sizeof(rand));
} while (!rand);
cmpxchg(&nf_conntrack_hash_rnd, 0, rand);
+
+ /* recompute the hash as nf_conntrack_hash_rnd is initialized */
+ __hash = ____hash_conntrack(orig, zone);
}
/* We don't want any race condition at early drop stage */
@@ -590,7 +625,7 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
if (nf_conntrack_max &&
unlikely(atomic_read(&net->ct.count) > nf_conntrack_max)) {
- unsigned int hash = hash_conntrack(net, zone, orig);
+ unsigned int hash = hash_bucket(__hash, net);
if (!early_drop(net, hash)) {
atomic_dec(&net->ct.count);
if (net_ratelimit())
@@ -621,7 +656,8 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
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;
+ /* save __hash for reusing when confirming */
+ *(unsigned long *)(&ct->tuplehash[IP_CT_DIR_REPLY].hnnode.pprev) = __hash;
/* Don't set timer yet: wait for confirmation */
setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
write_pnet(&ct->ct_net, net);
@@ -648,6 +684,14 @@ out_free:
return ERR_PTR(-ENOMEM);
#endif
}
+
+struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
+ const struct nf_conntrack_tuple *orig,
+ const struct nf_conntrack_tuple *repl,
+ gfp_t gfp)
+{
+ return __nf_conntrack_alloc(net, zone, orig, repl, gfp, 0);
+}
EXPORT_SYMBOL_GPL(nf_conntrack_alloc);
void nf_conntrack_free(struct nf_conn *ct)
@@ -669,7 +713,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
struct nf_conntrack_l3proto *l3proto,
struct nf_conntrack_l4proto *l4proto,
struct sk_buff *skb,
- unsigned int dataoff)
+ unsigned int dataoff, u32 __hash)
{
struct nf_conn *ct;
struct nf_conn_help *help;
@@ -683,7 +727,8 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
return NULL;
}
- ct = nf_conntrack_alloc(net, zone, tuple, &repl_tuple, GFP_ATOMIC);
+ ct = __nf_conntrack_alloc(net, zone, tuple, &repl_tuple, GFP_ATOMIC,
+ __hash);
if (IS_ERR(ct)) {
pr_debug("Can't allocate conntrack.\n");
return (struct nf_conntrack_tuple_hash *)ct;
@@ -760,6 +805,7 @@ resolve_normal_ct(struct net *net, struct nf_conn *tmpl,
struct nf_conntrack_tuple_hash *h;
struct nf_conn *ct;
u16 zone = tmpl ? nf_ct_zone(tmpl) : NF_CT_DEFAULT_ZONE;
+ u32 __hash;
if (!nf_ct_get_tuple(skb, skb_network_offset(skb),
dataoff, l3num, protonum, &tuple, l3proto,
@@ -769,10 +815,11 @@ resolve_normal_ct(struct net *net, struct nf_conn *tmpl,
}
/* look for tuple match */
- h = nf_conntrack_find_get(net, zone, &tuple);
+ __hash = ____hash_conntrack(&tuple, zone);
+ h = __nf_conntrack_find_get(net, zone, &tuple, __hash);
if (!h) {
h = init_conntrack(net, tmpl, &tuple, l3proto, l4proto,
- skb, dataoff);
+ skb, dataoff, __hash);
if (!h)
return NULL;
if (IS_ERR(h))
@@ -1312,8 +1359,7 @@ int nf_conntrack_set_hashsize(const char *val, struct kernel_param *kp)
ct = nf_ct_tuplehash_to_ctrack(h);
hlist_nulls_del_rcu(&h->hnnode);
bucket = __hash_conntrack(&h->tuple, nf_ct_zone(ct),
- hashsize,
- nf_conntrack_hash_rnd);
+ hashsize);
hlist_nulls_add_head_rcu(&h->hnnode, &hash[bucket]);
}
}
^ permalink raw reply related
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Eric Dumazet @ 2010-08-20 16:17 UTC (permalink / raw)
To: Changli Gao; +Cc: Patrick McHardy, David S. Miller, netfilter-devel, netdev
In-Reply-To: <1282320404-2948-1-git-send-email-xiaosuo@gmail.com>
Le samedi 21 août 2010 à 00:06 +0800, Changli Gao a écrit :
> nf_conntrack_alloc() isn't called with nf_conntrack_lock locked, so hash
> random initializing code maybe executed more than once on different CPUs.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> net/netfilter/nf_conntrack_core.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index df3eedb..7ae7f71 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -65,8 +65,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_max);
> DEFINE_PER_CPU(struct nf_conn, nf_conntrack_untracked);
> EXPORT_PER_CPU_SYMBOL(nf_conntrack_untracked);
>
> -static int nf_conntrack_hash_rnd_initted;
> -static unsigned int nf_conntrack_hash_rnd;
> +static unsigned int nf_conntrack_hash_rnd __read_mostly;
>
> static u_int32_t __hash_conntrack(const struct nf_conntrack_tuple *tuple,
> u16 zone, unsigned int size, unsigned int rnd)
> @@ -574,10 +573,16 @@ struct nf_conn *nf_conntrack_alloc(struct net *net, u16 zone,
> {
> struct nf_conn *ct;
>
> - if (unlikely(!nf_conntrack_hash_rnd_initted)) {
> - get_random_bytes(&nf_conntrack_hash_rnd,
> - sizeof(nf_conntrack_hash_rnd));
> - nf_conntrack_hash_rnd_initted = 1;
> + if (unlikely(!nf_conntrack_hash_rnd)) {
> + unsigned int rand;
> +
> + /* Why not initialize nf_conntrack_rnd in a "init()" function ?
> + * Because there isn't enough entropy when system initializing,
> + * and we initialize it as late as possible. */
This patch is fine but a fine multi line comment is :
/* Some
* fine
* comment
*/
> + do {
> + get_random_bytes(&rand, sizeof(rand));
> + } while (!rand);
> + cmpxchg(&nf_conntrack_hash_rnd, 0, rand);
> }
>
> /* We don't want any race condition at early drop stage */
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Western Union
From: Christine.Rey-Caillat @ 2010-08-20 16:17 UTC (permalink / raw)
Good day,
My working partner has helped me to send your
first payment of US$7,500 to you as
instructed by Mr. David Cameron and will
keep sending you US$7,500 twice a week until
the payment of (US$360,000) is completed
within six months and here is the information
below:
MONEY TRANSFER CONTROL NUMBER (MTCN):
522-905-9427
SENDER'S NAME: Mr. Mark Daniel
AMOUNT: US$7,500
To track your funds forward Western Union
Money Transfer agent your Full Names and
Mobile Number via Email to:
Mr Gary Moore
E-mail:western.union_department@w.cn
D/L Number: +44 (0) 702 403 4679
Fax Number: +44 (0) 844 774 1410
Please direct all enquiring to:
western.union_department@w.cn
Best Regards,
Mrs. Larisa Alexander.
^ permalink raw reply
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Jan Engelhardt @ 2010-08-20 16:20 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <1282321023.2484.285.camel@edumazet-laptop>
On Friday 2010-08-20 18:17, Eric Dumazet wrote:
>> + if (unlikely(!nf_conntrack_hash_rnd)) {
>> + unsigned int rand;
>> +
>> + /* Why not initialize nf_conntrack_rnd in a "init()" function ?
>> + * Because there isn't enough entropy when system initializing,
>> + * and we initialize it as late as possible. */
>
>This patch is fine but a fine multi line comment is :
> /* Some
> * fine
> * comment
> */
Actually,
/*
* Some
*/
But that's in CodingStyle already. Always a good read, even if reread.
^ permalink raw reply
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Eric Dumazet @ 2010-08-20 16:26 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <alpine.LSU.2.01.1008201819080.7497@obet.zrqbmnf.qr>
Le vendredi 20 août 2010 à 18:20 +0200, Jan Engelhardt a écrit :
> On Friday 2010-08-20 18:17, Eric Dumazet wrote:
> >> + if (unlikely(!nf_conntrack_hash_rnd)) {
> >> + unsigned int rand;
> >> +
> >> + /* Why not initialize nf_conntrack_rnd in a "init()" function ?
> >> + * Because there isn't enough entropy when system initializing,
> >> + * and we initialize it as late as possible. */
> >
> >This patch is fine but a fine multi line comment is :
> > /* Some
> > * fine
> > * comment
> > */
>
> Actually,
>
> /*
> * Some
> */
>
> But that's in CodingStyle already. Always a good read, even if reread.
Sorry, I documented David S. Miller choice, or else I would have
mentioned CodingStyle :)
/* Some
* fine
* comment
*/
And it really is better ;)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Jan Engelhardt @ 2010-08-20 16:34 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <1282321600.2484.297.camel@edumazet-laptop>
On Friday 2010-08-20 18:26, Eric Dumazet wrote:
>> Actually,
>> /*
>> * Some
>> */
>> But that's in CodingStyle already. Always a good read, even if reread.
>
>Sorry, I documented David S. Miller choice, or else I would have
>mentioned CodingStyle :)
>
> /* Some
> * fine
> * comment
> */
>
>And it really is better ;)
Looks unsymmetric, which is probably the reason CodingStyle was
conceived differently in the first place.
^ permalink raw reply
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Eric Dumazet @ 2010-08-20 16:35 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <alpine.LSU.2.01.1008201832500.7497@obet.zrqbmnf.qr>
Le vendredi 20 août 2010 à 18:34 +0200, Jan Engelhardt a écrit :
> On Friday 2010-08-20 18:26, Eric Dumazet wrote:
>
> >> Actually,
> >> /*
> >> * Some
> >> */
> >> But that's in CodingStyle already. Always a good read, even if reread.
> >
> >Sorry, I documented David S. Miller choice, or else I would have
> >mentioned CodingStyle :)
> >
> > /* Some
> > * fine
> > * comment
> > */
> >
> >And it really is better ;)
>
> Looks unsymmetric, which is probably the reason CodingStyle was
> conceived differently in the first place.
Who said comments must be symmetric ? :)
http://www.spinics.net/lists/netdev/msg134727.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* (unknown),
From: Mr. Vincent Cheng @ 2010-08-20 16:52 UTC (permalink / raw)
Good Day,
I have a business proposal of USD $22,500,000.00 only for you to
transact with me from my bank to your country.
Reply to address:choi_chu008@yahoo.co.jp and I will let you know what
is required of you.
Best Regards,
Mr. Vincent Cheng
^ permalink raw reply
* Re: TAHI CN-6-4-1 failed on Linux 2.6.32 kernel
From: Steve Chen @ 2010-08-20 17:16 UTC (permalink / raw)
To: David Miller; +Cc: brian.haley, usagi-users-ctl, netdev
In-Reply-To: <20100819.170649.212693227.davem@davemloft.net>
On Thu, Aug 19, 2010 at 7:06 PM, David Miller <davem@davemloft.net> wrote:
> From: Steve Chen <schen@mvista.com>
> Date: Thu, 19 Aug 2010 13:35:14 -0500
>
>> I trace through the code. It appears that the network driver (e1000e
>> for my setup) always set ip_summed to CHECKSUM_UNNECESSARY. I have
>> been unsuccessful to get the driver to take the other branch where
>> ip_summed is set to CHECKSUM_COMPLETE. Even when I hard code
>> ip_summed to CHECKSUM_COMPLETE, __skb_checksum_complete_head set
>> ip_summed to CHECKSUM_UNNECESSARY after recomputing the checksum.
>>
>> So far the only way I'm able to get ICMP to recompute checksum is
>> through the attached hack. Even though I can get all the tests to
>> pass, but it just seem wrong.
>
> If turning off hardware RX checksumming with ethtool has no effect,
> and the problem is seen with multiple ethernet cards, the problem
> is elsewhere.
>
> First of all, if you turn RX checksumming off, the checksum field
> of the SKB should always be skb->ip_summed = 0. If this is not
> happening, find out why.
Ahhh, thats my problem. I incorrectly thought the ip_summed should be
2. The ip_summed is set to 1 in
__skb_checksum_complete_head. Looking at the code, shouldn't
if (likely(!sum))
be
if (likely(sum))
Since sum == 0 would indicate an error?
Thanks
Steve
^ permalink raw reply
* Re: [PATCH 1/2] netfilter: fix the hash random initializing race
From: Jan Engelhardt @ 2010-08-20 17:20 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, Patrick McHardy, David S. Miller, netfilter-devel,
netdev
In-Reply-To: <1282322155.2484.309.camel@edumazet-laptop>
On Friday 2010-08-20 18:35, Eric Dumazet wrote:
>>>/* Some
>>> * fine
>>> * comment
>>> */
>>>And it really is better
>>Looks unsymmetric
>Who said comments must be symmetric ?
I can live with sub-level maintainers' free interpretation of areas
not covered by CodingStyle, but top-level maintainers directly going
against the law of Linux code is outrageous! The system is
undeniable :)
^ permalink raw reply
* Re: TAHI CN-6-4-1 failed on Linux 2.6.32 kernel
From: Jesse Gross @ 2010-08-20 17:39 UTC (permalink / raw)
To: Steve Chen; +Cc: David Miller, brian.haley, usagi-users-ctl, netdev
In-Reply-To: <AANLkTimSishHmLTQ9RF8zMq_eOBQ+HiBroKoygFS27-x@mail.gmail.com>
On Fri, Aug 20, 2010 at 1:16 PM, Steve Chen <schen@mvista.com> wrote:
>
> On Thu, Aug 19, 2010 at 7:06 PM, David Miller <davem@davemloft.net> wrote:
> > From: Steve Chen <schen@mvista.com>
> > Date: Thu, 19 Aug 2010 13:35:14 -0500
> >
> >> I trace through the code. It appears that the network driver (e1000e
> >> for my setup) always set ip_summed to CHECKSUM_UNNECESSARY. I have
> >> been unsuccessful to get the driver to take the other branch where
> >> ip_summed is set to CHECKSUM_COMPLETE. Even when I hard code
> >> ip_summed to CHECKSUM_COMPLETE, __skb_checksum_complete_head set
> >> ip_summed to CHECKSUM_UNNECESSARY after recomputing the checksum.
> >>
> >> So far the only way I'm able to get ICMP to recompute checksum is
> >> through the attached hack. Even though I can get all the tests to
> >> pass, but it just seem wrong.
> >
> > If turning off hardware RX checksumming with ethtool has no effect,
> > and the problem is seen with multiple ethernet cards, the problem
> > is elsewhere.
> >
> > First of all, if you turn RX checksumming off, the checksum field
> > of the SKB should always be skb->ip_summed = 0. If this is not
> > happening, find out why.
>
> Ahhh, thats my problem. I incorrectly thought the ip_summed should be
> 2. The ip_summed is set to 1 in
> __skb_checksum_complete_head. Looking at the code, shouldn't
>
> if (likely(!sum))
>
> be
>
> if (likely(sum))
>
> Since sum == 0 would indicate an error?
sum == 0 indicates that the checksum is correct.
If you compute the checksum of a packet containing the correct
checksum the result is 0. It's like a slightly more complicated
varient of a parity bit.
^ permalink raw reply
* Re: [iproute2] iproute2: Fix filtering related to flushing IP addresses.
From: Ben Greear @ 2010-08-20 17:49 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <1281978008-6383-1-git-send-email-greearb@candelatech.com>
On 08/16/2010 10:00 AM, Ben Greear wrote:
> The old 'ip addr flush' logic had several flaws:
Stephen, any chance this can be accepted upstream?
Thanks,
Ben
>
> * It reversed logic for primary v/s secondary flags
> (though, it sort of worked right anyway)
>
> * The code tried to remove secondaries and then primaries,
> but in practice, it always removed one primary per loop,
> which not at all efficient.
>
> * The filter logic in the core would run only the first
> filter in most cases.
>
> * If you used '-s -s', the ifa_flags member would be
> modified, which could make future filters fail
> to function fine.
>
> This patch attempts to fix all of these issues.
>
> Tested-by: Brian Haley<brian.haley@hp.com>
> Signed-off-by: Ben Greear<greearb@candelatech.com>
> ---
> :100644 100644 3a411b1... 19b3d6e... M ip/ipaddress.c
> :100644 100644 cfeb894... ee4f045... M lib/libnetlink.c
> ip/ipaddress.c | 34 +++++++++++++++++++++++-----------
> lib/libnetlink.c | 23 ++++++++++++++++-------
> 2 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 3a411b1..19b3d6e 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -453,6 +453,8 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
> struct ifaddrmsg *ifa = NLMSG_DATA(n);
> int len = n->nlmsg_len;
> int deprecated = 0;
> + /* Use local copy of ifa_flags to not interfere with filtering code */
> + unsigned int ifa_flags;
> struct rtattr * rta_tb[IFA_MAX+1];
> char abuf[256];
> SPRINT_BUF(b1);
> @@ -572,40 +574,41 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
> abuf, sizeof(abuf)));
> }
> fprintf(fp, "scope %s ", rtnl_rtscope_n2a(ifa->ifa_scope, b1, sizeof(b1)));
> + ifa_flags = ifa->ifa_flags;
> if (ifa->ifa_flags&IFA_F_SECONDARY) {
> - ifa->ifa_flags&= ~IFA_F_SECONDARY;
> + ifa_flags&= ~IFA_F_SECONDARY;
> if (ifa->ifa_family == AF_INET6)
> fprintf(fp, "temporary ");
> else
> fprintf(fp, "secondary ");
> }
> if (ifa->ifa_flags&IFA_F_TENTATIVE) {
> - ifa->ifa_flags&= ~IFA_F_TENTATIVE;
> + ifa_flags&= ~IFA_F_TENTATIVE;
> fprintf(fp, "tentative ");
> }
> if (ifa->ifa_flags&IFA_F_DEPRECATED) {
> - ifa->ifa_flags&= ~IFA_F_DEPRECATED;
> + ifa_flags&= ~IFA_F_DEPRECATED;
> deprecated = 1;
> fprintf(fp, "deprecated ");
> }
> if (ifa->ifa_flags&IFA_F_HOMEADDRESS) {
> - ifa->ifa_flags&= ~IFA_F_HOMEADDRESS;
> + ifa_flags&= ~IFA_F_HOMEADDRESS;
> fprintf(fp, "home ");
> }
> if (ifa->ifa_flags&IFA_F_NODAD) {
> - ifa->ifa_flags&= ~IFA_F_NODAD;
> + ifa_flags&= ~IFA_F_NODAD;
> fprintf(fp, "nodad ");
> }
> if (!(ifa->ifa_flags&IFA_F_PERMANENT)) {
> fprintf(fp, "dynamic ");
> } else
> - ifa->ifa_flags&= ~IFA_F_PERMANENT;
> + ifa_flags&= ~IFA_F_PERMANENT;
> if (ifa->ifa_flags&IFA_F_DADFAILED) {
> - ifa->ifa_flags&= ~IFA_F_DADFAILED;
> + ifa_flags&= ~IFA_F_DADFAILED;
> fprintf(fp, "dadfailed ");
> }
> - if (ifa->ifa_flags)
> - fprintf(fp, "flags %02x ", ifa->ifa_flags);
> + if (ifa_flags)
> + fprintf(fp, "flags %02x ", ifa_flags);
> if (rta_tb[IFA_LABEL])
> fprintf(fp, "%s", (char*)RTA_DATA(rta_tb[IFA_LABEL]));
> if (rta_tb[IFA_CACHEINFO]) {
> @@ -638,7 +641,7 @@ int print_addrinfo_primary(const struct sockaddr_nl *who, struct nlmsghdr *n,
> {
> struct ifaddrmsg *ifa = NLMSG_DATA(n);
>
> - if (!ifa->ifa_flags& IFA_F_SECONDARY)
> + if (ifa->ifa_flags& IFA_F_SECONDARY)
> return 0;
>
> return print_addrinfo(who, n, arg);
> @@ -649,7 +652,7 @@ int print_addrinfo_secondary(const struct sockaddr_nl *who, struct nlmsghdr *n,
> {
> struct ifaddrmsg *ifa = NLMSG_DATA(n);
>
> - if (ifa->ifa_flags& IFA_F_SECONDARY)
> + if (!(ifa->ifa_flags& IFA_F_SECONDARY))
> return 0;
>
> return print_addrinfo(who, n, arg);
> @@ -849,6 +852,7 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
> exit(1);
> }
> if (filter.flushed == 0) {
> +flush_done:
> if (show_stats) {
> if (round == 0)
> printf("Nothing to flush.\n");
> @@ -866,6 +870,14 @@ static int ipaddr_list_or_flush(int argc, char **argv, int flush)
> printf("\n*** Round %d, deleting %d addresses ***\n", round, filter.flushed);
> fflush(stdout);
> }
> +
> + /* If we are flushing, and specifying primary, then we
> + * want to flush only a single round. Otherwise, we'll
> + * start flushing secondaries that were promoted to
> + * primaries.
> + */
> + if (!(filter.flags& IFA_F_SECONDARY)&& (filter.flagmask& IFA_F_SECONDARY))
> + goto flush_done;
> }
> fprintf(stderr, "*** Flush remains incomplete after %d rounds. ***\n", MAX_ROUNDS); fflush(stderr);
> return 1;
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index cfeb894..ee4f045 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -189,6 +189,8 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> while (1) {
> int status;
> const struct rtnl_dump_filter_arg *a;
> + int found_done = 0;
> + int msglen = 0;
>
> iov.iov_len = sizeof(buf);
> status = recvmsg(rth->fd,&msg, 0);
> @@ -208,8 +210,9 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
>
> for (a = arg; a->filter; a++) {
> struct nlmsghdr *h = (struct nlmsghdr*)buf;
> + msglen = status;
>
> - while (NLMSG_OK(h, status)) {
> + while (NLMSG_OK(h, msglen)) {
> int err;
>
> if (nladdr.nl_pid != 0 ||
> @@ -224,8 +227,10 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> goto skip_it;
> }
>
> - if (h->nlmsg_type == NLMSG_DONE)
> - return 0;
> + if (h->nlmsg_type == NLMSG_DONE) {
> + found_done = 1;
> + break; /* process next filter */
> + }
> if (h->nlmsg_type == NLMSG_ERROR) {
> struct nlmsgerr *err = (struct nlmsgerr*)NLMSG_DATA(h);
> if (h->nlmsg_len< NLMSG_LENGTH(sizeof(struct nlmsgerr))) {
> @@ -242,15 +247,19 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth,
> return err;
>
> skip_it:
> - h = NLMSG_NEXT(h, status);
> + h = NLMSG_NEXT(h, msglen);
> }
> - } while (0);
> + }
> +
> + if (found_done)
> + return 0;
> +
> if (msg.msg_flags& MSG_TRUNC) {
> fprintf(stderr, "Message truncated\n");
> continue;
> }
> - if (status) {
> - fprintf(stderr, "!!!Remnant of size %d\n", status);
> + if (msglen) {
> + fprintf(stderr, "!!!Remnant of size %d\n", msglen);
> exit(1);
> }
> }
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: yao zhao @ 2010-08-20 17:54 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, Simon Horman, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <1282318337.2484.219.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 11:32 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 20 août 2010 à 11:04 -0400, yao zhao a écrit :
>
>> The code here is deleting a global from the list, am I right? I didn't
>> see any called case.
>> what are you going to do more? free it? write_unlock_bh should make the mb.
>
>
> If you dont wait _after_ delete from list and following actions
> (kfree() without a call_rcu(), or module unload, or whatever), a reader
> might access your data/code and crash the box.
>
> spin_unlock_bh() wont help you at all, since only writers are freezed by
> the lock (since readers only hold rcu_lock)
>
> Documentation/RCU/whatisRCU.txt line 705
>
> Documentation/RCU/checklist.txt 15)
>
>
>
>
I read the code again and that global is in a module then you are right.
If that global is not in a module then you don't need it at all, as in
that global only functions pointer or name... which never be changed.
yao
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Julian Anastasov @ 2010-08-20 18:03 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Wensong Zhang
In-Reply-To: <20100820133320.GA29311@verge.net.au>
Hello,
On Fri, 20 Aug 2010, Simon Horman wrote:
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
>
> I'm still getting my head around RCU, so review would be greatly appreciated.
>
> It occurs to me that this code is not performance critical, so
> perhaps simply replacing the rwlock with a spinlock would be better?
This specific code does not need RCU conversion, see below
> Index: nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c
> ===================================================================
> --- nf-next-2.6.orig/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:01.000000000 +0900
> +++ nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:51.000000000 +0900
> @@ -35,7 +35,7 @@
> static LIST_HEAD(ip_vs_schedulers);
>
> /* lock for service table */
> -static DEFINE_RWLOCK(__ip_vs_sched_lock);
> +static DEFINE_SPINLOCK(ip_vs_sched_mutex);
Here is what I got as list of locking points:
__ip_vs_conntbl_lock_array:
- can benefit from RCU, main benefits come from here
- ip_vs_conn_unhash() followed by ip_vs_conn_hash() is tricky with RCU,
needs more thinking, eg. when cport is changed
cp->lock, cp->refcnt:
- not a problem
tcp_app_lock, udp_app_lock, sctp_app_lock:
- can benefit from RCU (once per connection)
svc->sched_lock:
- only 1 read_lock, mostly writers that need exclusive access
- so, not suitable for RCU, can be switched to spin_lock for speed
__ip_vs_sched_lock:
- not called by packet handlers, no need for RCU
- used only by one ip_vs_ctl user (configuration) and the
scheduler modules
- can remain RWLOCK, no changes in locking are needed
__ip_vs_svc_lock:
- spin_lock, use RCU
- restrictions for schedulers with .update_service method
because svc->sched_lock is write locked, see below
__ip_vs_rs_lock:
- spin_lock, use RCU
Schedulers:
- every .schedule method has its own locking, two examples:
- write_lock: to protect the scheduler state (can be
changed to spin_lock), see WRR. Difficult for RCU.
- no lock: relies on IP_VS_WAIT_WHILE, no state
is protected explicitly, fast like RCU, see WLC
Scheduler state, eg. mark->cl:
- careful RCU assignment, may be all .update_service methods
should use copy-on-update (WRR). OTOH, ip_vs_wlc_schedule (WLC)
has no locks at all, thanks to the IP_VS_WAIT_WHILE, so
it is fast as RCU.
Statistics:
dest->stats.lock, svc->stats.lock, ip_vs_stats.lock:
- called for every packet, BAD for SMP, see ip_vs_in_stats(),
ip_vs_out_stats(), ip_vs_conn_stats()
curr_sb_lock:
- called for every packet depending on conn state
- No benefits from RCU, should be spin_lock
To summarize:
- the main problem remains stats:
dest->stats.lock, svc->stats.lock, ip_vs_stats.lock
- RCU benefits when connection processes many packets per connection, eg.
for TCP, SCTP, not much for UDP. No gains for the 1st
packet in connection.
- svc: no benefits from RCU, some schedulers protect state and
need exclusive access, others have no state (and they do not use
locks even now)
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: [iproute2] iproute2: Fix filtering related to flushing IP addresses.
From: Stephen Hemminger @ 2010-08-20 18:49 UTC (permalink / raw)
To: Ben Greear; +Cc: netdev
In-Reply-To: <4C6EC047.1030308@candelatech.com>
On Fri, 20 Aug 2010 10:49:59 -0700
Ben Greear <greearb@candelatech.com> wrote:
> On 08/16/2010 10:00 AM, Ben Greear wrote:
> > The old 'ip addr flush' logic had several flaws:
>
> Stephen, any chance this can be accepted upstream?
>
> Thanks,
> Ben
Since it had gone for a couple versions, was waiting for the
dust to settle for a week or so.
--
^ permalink raw reply
* Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses
From: Kevin Hilman @ 2010-08-20 18:51 UTC (permalink / raw)
To: Grant Likely
Cc: Moffett, Kyle D, Patrick Pannuto, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, magnus.damm@gmail.com,
gregkh@suse.de, Paul Mundt, Magnus Damm, Rafael J. Wysocki,
Eric Miao, Dmitry Torokhov, netdev@vger.kernel.org,
Kyle D Moffett
In-Reply-To: <AANLkTinaddiPAcokH3fRVYAFf3N-Gz--U5Jd1iyNsi+m@mail.gmail.com>
Grant Likely <grant.likely@secretlab.ca> writes:
[...]
>> One of the primary goals of this (at least for me and it seems Magnus also)
>> is to keep drivers ignorant of their bus type, and any bus-type-specific
>> code should be done in the bus_type implementation.
>
> Heh; which just screams to me that bus_type is the wrong level to be
> abstracting the behaviour
Heh, now I feel like we're going around in circles. Remember, I never
wanted to create add a new bus_type. Someone else [ahem] suggested
doing the abstraction at the bus_type level. ;)
> (but I also understand your need for the
> "omap_device" wrapper around platform_device which also requires some
> method to sort out when a platform_device really is an omap_device
> without an unsafe dereference).
Yes, I'm working on the devres approach to that now, as is Magnus for
the sh-mobile version (proposed for .36-rc1[1])
>> Both for SH and OMAP, we've been using the (admiteddly broken)
>> weak-symbol-override approach to getting a custom bus-type based on the
>> platform_bus. We've been using that in OMAP for a while now and have
>> not seen any need to for the drivers to know if they are on the vanilla
>> platform_bus or the customized one.
>>
>> I'm very curious to hear what type of impact you expect to the drivers.
>
> My fears on this point may very well be unfounded. This isn't the
> hill I'm going to die on either. Show me an implementation of driver
> sharing that is clean and prove me wrong! :-)
IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
simplest.
Since we seem to be in agreement now that the a new bus may not the
right abstraction for this (since we want it to be completely
transparent to the drivers), I'll go back to the original design. No new
bus types, keep the platform_bus as is, but simply override the few
dev_pm_ops methods I care about. This is what is done on SH,
SH-Mobile[1] and my original version for OMAP that started this
conversation.
Yes, the weak-symbol method of overriding is not scalable, but that's a
separate issue from whether or not to create a new bus. I have a
proposed fix for the weak which I'll post shortly.
Kevin
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2010-August/022411.html
^ permalink raw reply
* ip_tables.h can't be used in C++ programs.
From: Ben Greear @ 2010-08-20 18:54 UTC (permalink / raw)
To: NetDev
This method returns void* but is defined to return
a pointer to struct ipt_entry_target.
This will not compile under g++ (or, I'm unable to figure out
how to make it work w/out editing the header file).
Any reason this shouldn't be changed to:
return (struct ipt_entry_target*)e + e->target_offset;
/* Helper functions */
static __inline__ struct ipt_entry_target *
ipt_get_target(struct ipt_entry *e)
{
return (void *)e + e->target_offset;
}
IPv6 has similar issue....
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [iproute2] iproute2: Fix filtering related to flushing IP addresses.
From: Ben Greear @ 2010-08-20 18:55 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20100820114915.6d46cf8b@nehalam>
On 08/20/2010 11:49 AM, Stephen Hemminger wrote:
> On Fri, 20 Aug 2010 10:49:59 -0700
> Ben Greear<greearb@candelatech.com> wrote:
>
>> On 08/16/2010 10:00 AM, Ben Greear wrote:
>>> The old 'ip addr flush' logic had several flaws:
>>
>> Stephen, any chance this can be accepted upstream?
>>
>> Thanks,
>> Ben
>
> Since it had gone for a couple versions, was waiting for the
> dust to settle for a week or so.
Ok, no problem...
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Paul E. McKenney @ 2010-08-20 19:29 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Wensong Zhang, Julian Anastasov
In-Reply-To: <20100820135918.GB17980@verge.net.au>
On Fri, Aug 20, 2010 at 10:59:19PM +0900, Simon Horman wrote:
> On Fri, Aug 20, 2010 at 10:33:21PM +0900, Simon Horman wrote:
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > ---
> >
> > I'm still getting my head around RCU, so review would be greatly appreciated.
> >
> > It occurs to me that this code is not performance critical, so
> > perhaps simply replacing the rwlock with a spinlock would be better?
> >
> > Index: nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c
> > ===================================================================
> > --- nf-next-2.6.orig/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:01.000000000 +0900
> > +++ nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 22:21:51.000000000 +0900
> > @@ -35,7 +35,7 @@
> > static LIST_HEAD(ip_vs_schedulers);
> >
> > /* lock for service table */
> > -static DEFINE_RWLOCK(__ip_vs_sched_lock);
> > +static DEFINE_SPINLOCK(ip_vs_sched_mutex);
> >
> >
> > /*
> > @@ -91,9 +91,9 @@ static struct ip_vs_scheduler *ip_vs_sch
> >
> > IP_VS_DBG(2, "%s(): sched_name \"%s\"\n", __func__, sched_name);
> >
> > - read_lock_bh(&__ip_vs_sched_lock);
> > + rcu_read_lock_bh();
> >
> > - list_for_each_entry(sched, &ip_vs_schedulers, n_list) {
> > + list_for_each_entry_rcu(sched, &ip_vs_schedulers, n_list) {
> > /*
> > * Test and get the modules atomically
> > */
> > @@ -105,14 +105,14 @@ static struct ip_vs_scheduler *ip_vs_sch
> > }
> > if (strcmp(sched_name, sched->name)==0) {
> > /* HIT */
> > - read_unlock_bh(&__ip_vs_sched_lock);
> > + rcu_read_unlock_bh();
> > return sched;
> > }
> > if (sched->module)
> > module_put(sched->module);
> > }
> >
> > - read_unlock_bh(&__ip_vs_sched_lock);
> > + rcu_read_unlock_bh();
> > return NULL;
> > }
> >
> > @@ -167,10 +167,10 @@ int register_ip_vs_scheduler(struct ip_v
> > /* increase the module use count */
> > ip_vs_use_count_inc();
> >
> > - write_lock_bh(&__ip_vs_sched_lock);
> > + spin_lock_bh(&ip_vs_sched_mutex);
> >
> > if (!list_empty(&scheduler->n_list)) {
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
> > ip_vs_use_count_dec();
> > pr_err("%s(): [%s] scheduler already linked\n",
> > __func__, scheduler->name);
> > @@ -181,9 +181,9 @@ int register_ip_vs_scheduler(struct ip_v
> > * Make sure that the scheduler with this name doesn't exist
> > * in the scheduler list.
> > */
> > - list_for_each_entry(sched, &ip_vs_schedulers, n_list) {
> > + list_for_each_entry_rcu(sched, &ip_vs_schedulers, n_list) {
> > if (strcmp(scheduler->name, sched->name) == 0) {
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
> > ip_vs_use_count_dec();
> > pr_err("%s(): [%s] scheduler already existed "
> > "in the system\n", __func__, scheduler->name);
> > @@ -193,8 +193,8 @@ int register_ip_vs_scheduler(struct ip_v
> > /*
> > * Add it into the d-linked scheduler list
> > */
> > - list_add(&scheduler->n_list, &ip_vs_schedulers);
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + list_add_rcu(&scheduler->n_list, &ip_vs_schedulers);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
> >
> > pr_info("[%s] scheduler registered.\n", scheduler->name);
> >
> > @@ -212,9 +212,9 @@ int unregister_ip_vs_scheduler(struct ip
> > return -EINVAL;
> > }
> >
> > - write_lock_bh(&__ip_vs_sched_lock);
> > + spin_lock_bh(&ip_vs_sched_mutex);
> > if (list_empty(&scheduler->n_list)) {
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
> > pr_err("%s(): [%s] scheduler is not in the list. failed\n",
> > __func__, scheduler->name);
> > return -EINVAL;
> > @@ -223,8 +223,8 @@ int unregister_ip_vs_scheduler(struct ip
> > /*
> > * Remove it from the d-linked scheduler list
> > */
> > - list_del(&scheduler->n_list);
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + list_del_rcu(&scheduler->n_list);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
>
> On further reading, I believe that I need a synchronize_rcu(); here,
Good catch!
However, you actually need synchronize_rcu_bh() to match your
rcu_read_lock_bh() calls. Also, given Julian's comment, you probably
need something to show that this conversion is a real improvement.
Thanx, Paul
> > /* decrease the module use count */
> > ip_vs_use_count_dec();
> > --
> > To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Bugme-new] [Bug 16626] New: Machine hangs with EIP at skb_copy_and_csum_dev
From: Jarek Poplawski @ 2010-08-20 19:38 UTC (permalink / raw)
To: Plamen Petrov
Cc: Eric Dumazet, Andrew Morton, netdev, bugzilla-daemon,
bugme-daemon
In-Reply-To: <4C6E5EA7.3040609@fs.uni-ruse.bg>
Plamen Petrov wrote, On 20.08.2010 12:53:
> So, I guess its David and Herbert's turn?...
If you're bored in the meantime I'd suggest to do check the realtek
driver eg:
- for locking with the patch below,
- to turn off with ethtool its tx-checksumming and/or scatter-gather,
or if possible try to reproduce this with other NIC.
Thanks,
Jarek P.
---
diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index f5166dc..aaaccc5 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -1692,6 +1692,8 @@ static netdev_tx_t rtl8139_start_xmit (struct sk_buff *skb,
unsigned int len = skb->len;
unsigned long flags;
+ spin_lock_irqsave(&tp->lock, flags);
+
/* Calculate the next Tx descriptor entry. */
entry = tp->cur_tx % NUM_TX_DESC;
@@ -1700,14 +1702,14 @@ static netdev_tx_t rtl8139_start_xmit (struct sk_buff *skb,
if (len < ETH_ZLEN)
memset(tp->tx_buf[entry], 0, ETH_ZLEN);
skb_copy_and_csum_dev(skb, tp->tx_buf[entry]);
- dev_kfree_skb(skb);
+ dev_kfree_skb_irq(skb);
} else {
+ spin_unlock_irqrestore(&tp->lock, flags);
dev_kfree_skb(skb);
dev->stats.tx_dropped++;
return NETDEV_TX_OK;
}
- spin_lock_irqsave(&tp->lock, flags);
/*
* Writing to TxStatus triggers a DMA transfer of the data
* copied to tp->tx_buf[entry] above. Use a memory barrier
^ permalink raw reply related
* Re: [PATCH 2/2] platform: Facilitate the creation of pseudo-platform buses
From: Grant Likely @ 2010-08-20 20:08 UTC (permalink / raw)
To: Kevin Hilman
Cc: Moffett, Kyle D, Patrick Pannuto, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, magnus.damm@gmail.com,
gregkh@suse.de, Paul Mundt, Magnus Damm, Rafael J. Wysocki,
Eric Miao, Dmitry Torokhov, netdev@vger.kernel.org,
Kyle D Moffett
In-Reply-To: <87hbip9kxx.fsf@deeprootsystems.com>
On Fri, Aug 20, 2010 at 12:51 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Grant Likely <grant.likely@secretlab.ca> writes:
>
> [...]
>
>>> One of the primary goals of this (at least for me and it seems Magnus also)
>>> is to keep drivers ignorant of their bus type, and any bus-type-specific
>>> code should be done in the bus_type implementation.
>>
>> Heh; which just screams to me that bus_type is the wrong level to be
>> abstracting the behaviour
>
> Heh, now I feel like we're going around in circles. Remember, I never
> wanted to create add a new bus_type. Someone else [ahem] suggested
> doing the abstraction at the bus_type level. ;)
Hey! I didn't suggest it either! I believe that was Greg at ELC. I
just... um... kind of took it and ran with it. :-)
>> (but I also understand your need for the
>> "omap_device" wrapper around platform_device which also requires some
>> method to sort out when a platform_device really is an omap_device
>> without an unsafe dereference).
>
> Yes, I'm working on the devres approach to that now, as is Magnus for
> the sh-mobile version (proposed for .36-rc1[1])
>
>>> Both for SH and OMAP, we've been using the (admiteddly broken)
>>> weak-symbol-override approach to getting a custom bus-type based on the
>>> platform_bus. We've been using that in OMAP for a while now and have
>>> not seen any need to for the drivers to know if they are on the vanilla
>>> platform_bus or the customized one.
>>>
>>> I'm very curious to hear what type of impact you expect to the drivers.
>>
>> My fears on this point may very well be unfounded. This isn't the
>> hill I'm going to die on either. Show me an implementation of driver
>> sharing that is clean and prove me wrong! :-)
>
> IMHO, simply overriding the few dev_pm_ops methods was the cleanest and
> simplest.
>
> Since we seem to be in agreement now that the a new bus may not the
> right abstraction for this (since we want it to be completely
> transparent to the drivers), I'll go back to the original design. No new
> bus types, keep the platform_bus as is, but simply override the few
> dev_pm_ops methods I care about. This is what is done on SH,
> SH-Mobile[1] and my original version for OMAP that started this
> conversation.
>
> Yes, the weak-symbol method of overriding is not scalable, but that's a
> separate issue from whether or not to create a new bus. I have a
> proposed fix for the weak which I'll post shortly.
Okay.
One constraint remains though: If you can override the dev_pm_ops on
a per-device or per-device-parent basis, then you've got my support.
If the override (even when fixed to work at runtime) applies to every
device on the platform_bus_type, then I'll nack it. My concern here
is that the SoC or platform support code doesn't get to "own" the
platform_bus_type. Other drivers/code can register their own set of
platform_devices, which may also need to perform their own dev_pm_ops
override. If the override is global to the platform_bus_type, then
the model will not scale.
Cheers,
g.
^ permalink raw reply
* (unknown),
From: pavel potoplyak @ 2010-08-20 21:51 UTC (permalink / raw)
To: netdev
subscribe netdev
^ 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