* 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: [PATCH v3] netfilter: save the hash of the tuple in the original direction for latter use
From: Changli Gao @ 2010-08-20 15:36 UTC (permalink / raw)
To: Eric Dumazet
Cc: Patrick McHardy, David S. Miller, Mathieu Desnoyers, akpm,
netfilter-devel, netdev, linux-kernel
In-Reply-To: <1282318163.2484.212.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 11:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 20 août 2010 à 23:22 +0800, Changli Gao a écrit :
>
>> I should keep the old way, but fix a race.
>>
>> 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;
>> }
>>
>> nf_conntrack_alloc() isn't called with in the nf_conntrack_lock. So
>> the above code maybe executed more than once on different CPUs. It is
>> easy to fix with the cmpxchg() trick.
>
> Sure, please fix the race first.
>
> But as I said, its not critical, if one or two conntracks are hashed on
> wrong basis. They will eventually disappear after timeout.
>
Yes, and it isn't critical. I think this fix should be in a separate patch.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Eric Dumazet @ 2010-08-20 15:32 UTC (permalink / raw)
To: yao zhao
Cc: Changli Gao, Simon Horman, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <AANLkTikCd_xCWUN+dkb_BkOrkBGSPe4xkmAebtgEiufy@mail.gmail.com>
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)
^ permalink raw reply
* Re: [PATCH v3] netfilter: save the hash of the tuple in the original direction for latter use
From: Eric Dumazet @ 2010-08-20 15:29 UTC (permalink / raw)
To: Changli Gao
Cc: Patrick McHardy, David S. Miller, Mathieu Desnoyers, akpm,
netfilter-devel, netdev, linux-kernel
In-Reply-To: <AANLkTimj=6HBdZK0WiPBxhm9YPG_8zRXEZyP--E7Shj8@mail.gmail.com>
Le vendredi 20 août 2010 à 23:22 +0800, Changli Gao a écrit :
> I should keep the old way, but fix a race.
>
> 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;
> }
>
> nf_conntrack_alloc() isn't called with in the nf_conntrack_lock. So
> the above code maybe executed more than once on different CPUs. It is
> easy to fix with the cmpxchg() trick.
Sure, please fix the race first.
But as I said, its not critical, if one or two conntracks are hashed on
wrong basis. They will eventually disappear after timeout.
^ permalink raw reply
* Re[2]: [PATCH v7] PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)
From: Dmitry Kozlov @ 2010-08-20 15:28 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1282285499.2295.2.camel@edumazet-laptop>
> > 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 ?
^ permalink raw reply
* Re: [PATCH v3] netfilter: save the hash of the tuple in the original direction for latter use
From: Changli Gao @ 2010-08-20 15:22 UTC (permalink / raw)
To: Eric Dumazet
Cc: Patrick McHardy, David S. Miller, Mathieu Desnoyers, akpm,
netfilter-devel, netdev, linux-kernel
In-Reply-To: <1282317016.2484.173.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 11:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 20 août 2010 à 22:53 +0800, Changli Gao a écrit :
>> 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>
>> ---
>
> Hmm... so to accept a few more SYN packets per second in SYNFLOOD
> attack, we slow a bit normal operations ?
SYN-flood case is just a side effect. What I want to do is eliminating
the second call to hash_conntrack() of the original tuple in
__nf_conntrack_confirm().
> (adding one test on each
> packet going through conntrack)
Do you mean the rnd test?
>
> If yes (I dont think we should, hackers are stronger than you anyway,
> just face it)
>
> v4:
> __read_mostly on nf_conntrack_rnd
>
>
> What would happen if we let the initialization of nf_conntrack_rnd
> only in the insertion case (like currently done) ?
> Only the first packet received on the machine/conntrack might be hashed
> on a wrong slot. Is it a big deal ? If yes, maybe find a way to
> recompute the hash in this case, instead of reusing 'wrong' one ?
>
I should keep the old way, but fix a race.
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;
}
nf_conntrack_alloc() isn't called with in the nf_conntrack_lock. So
the above code maybe executed more than once on different CPUs. It is
easy to fix with the cmpxchg() trick.
Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH v3] netfilter: save the hash of the tuple in the original direction for latter use
From: Eric Dumazet @ 2010-08-20 15:10 UTC (permalink / raw)
To: Changli Gao
Cc: Patrick McHardy, David S. Miller, Mathieu Desnoyers, akpm,
netfilter-devel, netdev, linux-kernel
In-Reply-To: <1282316003-2966-1-git-send-email-xiaosuo@gmail.com>
Le vendredi 20 août 2010 à 22:53 +0800, Changli Gao a écrit :
> 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>
> ---
Hmm... so to accept a few more SYN packets per second in SYNFLOOD
attack, we slow a bit normal operations ? (adding one test on each
packet going through conntrack)
If yes (I dont think we should, hackers are stronger than you anyway,
just face it)
v4:
__read_mostly on nf_conntrack_rnd
What would happen if we let the initialization of nf_conntrack_rnd
only in the insertion case (like currently done) ?
Only the first packet received on the machine/conntrack might be hashed
on a wrong slot. Is it a big deal ? If yes, maybe find a way to
recompute the hash in this case, instead of reusing 'wrong' one ?
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: yao zhao @ 2010-08-20 15:04 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: <1282314765.2484.104.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 10:32 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 20 août 2010 à 10:16 -0400, yao zhao a écrit :
>
>> if it is not performance critical, you should use the
>> read_lock/write_lock, it should make the readers happier than
>> spinlock. the name "mutex" is a little bit confuse.
>
> Yes, I mentioned the 'mutex' name oddity.
>
> Point is :
>
> We want to remove read_write locks. They dont fit the bill.
>
> If performance critical, lot of readers -> RCU (a lot faster)
> If not, or too much writers versus readers -> spinlock (a bit faster)
>
>
for writers more than readers of course spin will be better, that is
what read/write lock for and spin for.
But the case here is whether these register_ip_vs_scheduler or
unregister are more frequent than the readers.
if not definitely read_lock will better than spin_lock. although worse than rcu.
>
>> synchronize_rcu() is not necessary when you only need to delete from a
>> list as it is atomic.
>>
>
> Thats a rather strange and completely wrong claim. A big part of RCU job
> is to have appropriate work done on deletes. Inserts are more easy (only
> needs a smp_wmb())
>
> Take a look at Documentation/RCU/* before saying such things ;)
>
> Not only synchronize_rcu() is not enough to protect this kind of code,
> but you need something stronger.
>
>
>
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.
yao
--
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 nf-next] IPVS: convert __ip_vs_sched_lock to a spinlock
From: Eric Dumazet @ 2010-08-20 14:56 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Changli Gao, yao zhao, Wensong Zhang, Julian Anastasov,
Patrick McHardy
In-Reply-To: <20100820145249.GB11781@verge.net.au>
Le vendredi 20 août 2010 à 23:52 +0900, Simon Horman a écrit :
> Also rename __ip_vs_sched_lock to ip_vs_sched_lock.
>
> Signed-off-by: Simon Horman <horms@verge.net.au>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
--
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 nf-next] IPVS: convert __ip_vs_securetcp_lock to a spinlock
From: Eric Dumazet @ 2010-08-20 14:55 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Changli Gao, yao zhao, Wensong Zhang, Julian Anastasov,
Patrick McHardy
In-Reply-To: <20100820145206.GA11781@verge.net.au>
Le vendredi 20 août 2010 à 23:52 +0900, Simon Horman a écrit :
> Also rename __ip_vs_securetcp_lock to ip_vs_securetcp_lock.
>
> Spinlock conversion was suggested by Eric Dumazet.
>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> ---
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
--
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
* [PATCH v3] netfilter: save the hash of the tuple in the original direction for latter use
From: Changli Gao @ 2010-08-20 14:53 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>
---
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 | 122 ++++++++++++++++++++++++++------------
1 file changed, 85 insertions(+), 37 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index df3eedb..a6bb577 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -65,32 +65,57 @@ 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_rnd;
-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;
+ if (unlikely(!nf_conntrack_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_rnd, 0, rand);
+ }
+
/* The direction must be ignored, so we hash everything up to the
* destination ports (which is a multiple of 4) and treat the last
* 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_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
@@ -292,13 +317,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.
@@ -327,19 +352,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) ||
@@ -357,6 +390,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,
@@ -409,7 +450,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
@@ -567,25 +609,20 @@ 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;
- 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;
- }
-
/* We don't want any race condition at early drop stage */
atomic_inc(&net->ct.count);
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())
@@ -616,7 +653,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);
@@ -643,6 +681,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)
@@ -664,7 +710,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;
@@ -678,7 +724,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;
@@ -755,6 +802,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,
@@ -764,10 +812,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))
@@ -1307,8 +1356,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
* [patch nf-next] IPVS: convert __ip_vs_sched_lock to a spinlock
From: Simon Horman @ 2010-08-20 14:52 UTC (permalink / raw)
To: lvs-devel, netdev, netfilter-devel
Cc: Stephen Hemminger, Eric Dumazet, Changli Gao, yao zhao,
Wensong Zhang, Julian Anastasov, Patrick McHardy
Also rename __ip_vs_sched_lock to ip_vs_sched_lock.
Signed-off-by: Simon Horman <horms@verge.net.au>
---
Compile tested only
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 23:43:29.000000000 +0900
+++ nf-next-2.6/net/netfilter/ipvs/ip_vs_sched.c 2010-08-20 23:46:02.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_lock);
/*
@@ -108,7 +108,7 @@ 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);
+ spin_lock_bh(&ip_vs_sched_lock);
list_for_each_entry(sched, &ip_vs_schedulers, n_list) {
/*
@@ -122,14 +122,14 @@ static struct ip_vs_scheduler *ip_vs_sch
}
if (strcmp(sched_name, sched->name)==0) {
/* HIT */
- read_unlock_bh(&__ip_vs_sched_lock);
+ spin_unlock_bh(&ip_vs_sched_lock);
return sched;
}
if (sched->module)
module_put(sched->module);
}
- read_unlock_bh(&__ip_vs_sched_lock);
+ spin_unlock_bh(&ip_vs_sched_lock);
return NULL;
}
@@ -184,10 +184,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_lock);
if (!list_empty(&scheduler->n_list)) {
- write_unlock_bh(&__ip_vs_sched_lock);
+ spin_unlock_bh(&ip_vs_sched_lock);
ip_vs_use_count_dec();
pr_err("%s(): [%s] scheduler already linked\n",
__func__, scheduler->name);
@@ -200,7 +200,7 @@ int register_ip_vs_scheduler(struct ip_v
*/
list_for_each_entry(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_lock);
ip_vs_use_count_dec();
pr_err("%s(): [%s] scheduler already existed "
"in the system\n", __func__, scheduler->name);
@@ -211,7 +211,7 @@ 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);
+ spin_unlock_bh(&ip_vs_sched_lock);
pr_info("[%s] scheduler registered.\n", scheduler->name);
@@ -229,9 +229,9 @@ int unregister_ip_vs_scheduler(struct ip
return -EINVAL;
}
- write_lock_bh(&__ip_vs_sched_lock);
+ spin_lock_bh(&ip_vs_sched_lock);
if (list_empty(&scheduler->n_list)) {
- write_unlock_bh(&__ip_vs_sched_lock);
+ spin_unlock_bh(&ip_vs_sched_lock);
pr_err("%s(): [%s] scheduler is not in the list. failed\n",
__func__, scheduler->name);
return -EINVAL;
@@ -241,7 +241,7 @@ 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);
+ spin_unlock_bh(&ip_vs_sched_lock);
/* decrease the module use count */
ip_vs_use_count_dec();
^ permalink raw reply
* [patch nf-next] IPVS: convert __ip_vs_securetcp_lock to a spinlock
From: Simon Horman @ 2010-08-20 14:52 UTC (permalink / raw)
To: lvs-devel, netdev, netfilter-devel
Cc: Stephen Hemminger, Eric Dumazet, Changli Gao, yao zhao,
Wensong Zhang, Julian Anastasov, Patrick McHardy
Also rename __ip_vs_securetcp_lock to ip_vs_securetcp_lock.
Spinlock conversion was suggested by Eric Dumazet.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
Compile tested only
Index: nf-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c
===================================================================
--- nf-next-2.6.orig/net/netfilter/ipvs/ip_vs_ctl.c 2010-08-20 22:41:52.000000000 +0900
+++ nf-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c 2010-08-20 23:29:49.000000000 +0900
@@ -61,7 +61,7 @@ static DEFINE_RWLOCK(__ip_vs_svc_lock);
static DEFINE_RWLOCK(__ip_vs_rs_lock);
/* lock for state and timeout tables */
-static DEFINE_RWLOCK(__ip_vs_securetcp_lock);
+static DEFINE_SPINLOCK(ip_vs_securetcp_lock);
/* lock for drop entry handling */
static DEFINE_SPINLOCK(__ip_vs_dropentry_lock);
@@ -204,7 +204,7 @@ static void update_defense_level(void)
spin_unlock(&__ip_vs_droppacket_lock);
/* secure_tcp */
- write_lock(&__ip_vs_securetcp_lock);
+ spin_lock(&ip_vs_securetcp_lock);
switch (sysctl_ip_vs_secure_tcp) {
case 0:
if (old_secure_tcp >= 2)
@@ -238,7 +238,7 @@ static void update_defense_level(void)
old_secure_tcp = sysctl_ip_vs_secure_tcp;
if (to_change >= 0)
ip_vs_protocol_timeout_change(sysctl_ip_vs_secure_tcp>1);
- write_unlock(&__ip_vs_securetcp_lock);
+ spin_unlock(&ip_vs_securetcp_lock);
local_bh_enable();
}
^ permalink raw reply
* Mutual Benefit.
From: Mr Jerry Ntail @ 2010-08-20 14:38 UTC (permalink / raw)
--
Complement of the day,
Although you might be apprehensive about my email as we have not meet
before,My name is Mr. Jerry Ntai I work with Mevas Bank Head of Operations
.I have a business proposal in the tune of $19.5m to be transferred to an
offshore account with your assistance.
Once the funds have been transferred to your nominated bank account we shall
then share in the ratio of 70 percent for me and 30 for you.Should you be
interested please send me your,
1, Full names,
2, private phone number,
3, current residential address,
4, 0ccupation,
5, Country,
Finally after that I shall provide you with more details,
Kind Regards,
Mr. Jerry Ntai.
________________________________________________
This message was sent
using Dodo Webmail - www.dodo.com.au
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Simon Horman @ 2010-08-20 14:33 UTC (permalink / raw)
To: yao zhao
Cc: Eric Dumazet, Changli Gao, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <AANLkTi=t7wDZO23Cim1LJ_Ot2FUe4bR+XG6H5uJfTuG+@mail.gmail.com>
On Fri, Aug 20, 2010 at 10:16:23AM -0400, yao zhao wrote:
> On Fri, Aug 20, 2010 at 10:05 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le vendredi 20 août 2010 à 21:44 +0800, Changli Gao a écrit :
> >> On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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
> >
> >
> >> > - write_unlock_bh(&__ip_vs_sched_lock);
> >> > + list_del_rcu(&scheduler->n_list);
> >> > + spin_unlock_bh(&ip_vs_sched_mutex);
> >>
> >> Need a rcu_barrier_bh().
> >>
> >> >
> >> > /* decrease the module use count */
> >> > ip_vs_use_count_dec();
> >
> >
> > Quite frankly, if this is not performance critical, just use the
> > spinlock (and dont use 'mutex' in its name ;) )
> >
> if it is not performance critical, you should use the
> read_lock/write_lock, it should make the readers happier than
> spinlock.
The whole point of the exercise is to stop using read_lock/write_lock
because they are generally slower than a spinlock.
> the name "mutex" is a little bit confuse.
> synchronize_rcu() is not necessary when you only need to delete from a
> list as it is atomic.
>
> > Using RCU here will force at least one RCU grace period at dismantle
> > time...
> >
> >
> >
> > --
> > 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
> >
>
> yao
--
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: [rfc] IPVS: convert scheduler management to RCU
From: Eric Dumazet @ 2010-08-20 14:32 UTC (permalink / raw)
To: yao zhao
Cc: Changli Gao, Simon Horman, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <AANLkTi=t7wDZO23Cim1LJ_Ot2FUe4bR+XG6H5uJfTuG+@mail.gmail.com>
Le vendredi 20 août 2010 à 10:16 -0400, yao zhao a écrit :
> if it is not performance critical, you should use the
> read_lock/write_lock, it should make the readers happier than
> spinlock. the name "mutex" is a little bit confuse.
Yes, I mentioned the 'mutex' name oddity.
Point is :
We want to remove read_write locks. They dont fit the bill.
If performance critical, lot of readers -> RCU (a lot faster)
If not, or too much writers versus readers -> spinlock (a bit faster)
> synchronize_rcu() is not necessary when you only need to delete from a
> list as it is atomic.
>
Thats a rather strange and completely wrong claim. A big part of RCU job
is to have appropriate work done on deletes. Inserts are more easy (only
needs a smp_wmb())
Take a look at Documentation/RCU/* before saying such things ;)
Not only synchronize_rcu() is not enough to protect this kind of code,
but you need something stronger.
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Simon Horman @ 2010-08-20 14:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: Changli Gao, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <1282313150.2484.65.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 04:05:50PM +0200, Eric Dumazet wrote:
> Le vendredi 20 août 2010 à 21:44 +0800, Changli Gao a écrit :
> > On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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
>
>
> > > - write_unlock_bh(&__ip_vs_sched_lock);
> > > + list_del_rcu(&scheduler->n_list);
> > > + spin_unlock_bh(&ip_vs_sched_mutex);
> >
> > Need a rcu_barrier_bh().
> >
> > >
> > > /* decrease the module use count */
> > > ip_vs_use_count_dec();
>
>
> Quite frankly, if this is not performance critical, just use the
> spinlock (and dont use 'mutex' in its name ;) )
Will do.
> Using RCU here will force at least one RCU grace period at dismantle
> time...
>
>
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: yao zhao @ 2010-08-20 14:16 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: <1282313150.2484.65.camel@edumazet-laptop>
On Fri, Aug 20, 2010 at 10:05 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 20 août 2010 à 21:44 +0800, Changli Gao a écrit :
>> On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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
>
>
>> > - write_unlock_bh(&__ip_vs_sched_lock);
>> > + list_del_rcu(&scheduler->n_list);
>> > + spin_unlock_bh(&ip_vs_sched_mutex);
>>
>> Need a rcu_barrier_bh().
>>
>> >
>> > /* decrease the module use count */
>> > ip_vs_use_count_dec();
>
>
> Quite frankly, if this is not performance critical, just use the
> spinlock (and dont use 'mutex' in its name ;) )
>
if it is not performance critical, you should use the
read_lock/write_lock, it should make the readers happier than
spinlock. the name "mutex" is a little bit confuse.
synchronize_rcu() is not necessary when you only need to delete from a
list as it is atomic.
> Using RCU here will force at least one RCU grace period at dismantle
> time...
>
>
>
> --
> 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
>
yao
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Eric Dumazet @ 2010-08-20 14:05 UTC (permalink / raw)
To: Changli Gao
Cc: Simon Horman, lvs-devel, netdev, netfilter-devel,
Stephen Hemminger, Wensong Zhang, Julian Anastasov,
Paul E McKenney
In-Reply-To: <AANLkTimS7V3z1kEwmEFCCvPzJX=D9Oqw=BNEuN-YZ0tx@mail.gmail.com>
Le vendredi 20 août 2010 à 21:44 +0800, Changli Gao a écrit :
> On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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
> > - write_unlock_bh(&__ip_vs_sched_lock);
> > + list_del_rcu(&scheduler->n_list);
> > + spin_unlock_bh(&ip_vs_sched_mutex);
>
> Need a rcu_barrier_bh().
>
> >
> > /* decrease the module use count */
> > ip_vs_use_count_dec();
Quite frankly, if this is not performance critical, just use the
spinlock (and dont use 'mutex' in its name ;) )
Using RCU here will force at least one RCU grace period at dismantle
time...
--
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/3] ibmveth: Remove duplicate checksum offload setup code
From: Brian King @ 2010-08-20 14:01 UTC (permalink / raw)
To: David Miller, netdev, Santiago Leon
In-Reply-To: <20100818160515.GA29528@linux.vnet.ibm.com>
Patches 1-3
Acked-by: Brian King <brking@linux.vnet.ibm.com>
On 08/18/2010 11:05 AM, Robert Jennings wrote:
> Remove code in the device probe function where we set up the checksum
> offload feature and replace it with a call to an existing function that
> is doing the same. This is done to clean up the driver in preparation
> of adding IPv6 checksum offload support.
>
> Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
>
> ---
> drivers/net/ibmveth.c | 18 +-----------------
> 1 file changed, 1 insertion(+), 17 deletions(-)
>
> Index: b/drivers/net/ibmveth.c
> ===================================================================
> --- a/drivers/net/ibmveth.c
> +++ b/drivers/net/ibmveth.c
> @@ -1222,10 +1222,8 @@ static const struct net_device_ops ibmve
> static int __devinit ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
> {
> int rc, i;
> - long ret;
> struct net_device *netdev;
> struct ibmveth_adapter *adapter;
> - unsigned long set_attr, ret_attr;
>
> unsigned char *mac_addr_p;
> unsigned int *mcastFilterSize_p;
> @@ -1310,21 +1308,7 @@ static int __devinit ibmveth_probe(struc
>
> ibmveth_debug_printk("registering netdev...\n");
>
> - ret = h_illan_attributes(dev->unit_address, 0, 0, &ret_attr);
> -
> - if (ret == H_SUCCESS && !(ret_attr & IBMVETH_ILLAN_ACTIVE_TRUNK) &&
> - !(ret_attr & IBMVETH_ILLAN_TRUNK_PRI_MASK) &&
> - (ret_attr & IBMVETH_ILLAN_PADDED_PKT_CSUM)) {
> - set_attr = IBMVETH_ILLAN_IPV4_TCP_CSUM;
> -
> - ret = h_illan_attributes(dev->unit_address, 0, set_attr, &ret_attr);
> -
> - if (ret == H_SUCCESS) {
> - adapter->rx_csum = 1;
> - netdev->features |= NETIF_F_IP_CSUM;
> - } else
> - ret = h_illan_attributes(dev->unit_address, set_attr, 0, &ret_attr);
> - }
> + ibmveth_set_csum_offload(netdev, 1, ibmveth_set_tx_csum_flags);
>
> rc = register_netdev(netdev);
>
> --
> 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
--
Brian King
Linux on Power Virtualization
IBM Linux Technology Center
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Simon Horman @ 2010-08-20 14:00 UTC (permalink / raw)
To: Changli Gao
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Wensong Zhang, Julian Anastasov, Paul E McKenney
In-Reply-To: <AANLkTimS7V3z1kEwmEFCCvPzJX=D9Oqw=BNEuN-YZ0tx@mail.gmail.com>
On Fri, Aug 20, 2010 at 09:44:08PM +0800, Changli Gao wrote:
> On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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);
>
> Need a rcu_barrier_bh().
Thanks.
>
> >
> > /* decrease the module use count */
> > ip_vs_use_count_dec();
>
>
>
> --
> Regards,
> Changli Gao(xiaosuo@gmail.com)
> --
> 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: [rfc] IPVS: convert scheduler management to RCU
From: Simon Horman @ 2010-08-20 13:59 UTC (permalink / raw)
To: lvs-devel, netdev, netfilter-devel
Cc: Stephen Hemminger, Wensong Zhang, Julian Anastasov
In-Reply-To: <20100820133320.GA29311@verge.net.au>
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,
>
> /* 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
^ permalink raw reply
* Re: [PATCH v2] netfilter: save the hash of the tuple in the original direction for latter use
From: Mathieu Desnoyers @ 2010-08-20 13:43 UTC (permalink / raw)
To: Changli Gao
Cc: Eric Dumazet, Patrick McHardy, David S. Miller, netfilter-devel,
netdev, akpm, linux-kernel
In-Reply-To: <AANLkTinA3-KMPKJa-66QCMUpbDFwey2mjE=VN=9=zg=6@mail.gmail.com>
[ executive summary: trying to explain why static variable declarations
within the body of functions is bad in kernel code. ]
* Changli Gao (xiaosuo@gmail.com) wrote:
> On Fri, Aug 20, 2010 at 12:11 AM, Mathieu Desnoyers
> <mathieu.desnoyers@polymtl.ca> wrote:
> >
> > Ah, I see. But I think the static variable should stay declared outside
> > of the function scope, with a nice comment explaining why it's not
> > initialized at init-time.
> >
> > Hiding global state in function code is usually frowned upon.
> >
>
> I don't agree with you. We'd better not expose the variable which
> isn't expected to be used by others. If not, maybe someone will misuse
> it.
We are talking about a static variable usable only within a single file,
where is the problem ? A nice comment can have this effect.
> The user should only reply on the interface, but not the internal
> implementation.
I'm talking about good practice to help code review. This is way more
important than the potential "encapsulation" benefit of hiding global
state (a static variable) within the body of a function.
I remember that Andrew Morton has strong opinions about this, and I
remember being in complete agreement with him on the topic.
Thanks,
Mathieu
>
> Thanks.
>
> --
> Regards,
> Changli Gao(xiaosuo@gmail.com)
--
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [rfc] IPVS: convert scheduler management to RCU
From: Changli Gao @ 2010-08-20 13:44 UTC (permalink / raw)
To: Simon Horman
Cc: lvs-devel, netdev, netfilter-devel, Stephen Hemminger,
Wensong Zhang, Julian Anastasov, Paul E McKenney
In-Reply-To: <20100820133320.GA29311@verge.net.au>
On Fri, Aug 20, 2010 at 9:33 PM, Simon Horman <horms@verge.net.au> 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);
Need a rcu_barrier_bh().
>
> /* decrease the module use count */
> ip_vs_use_count_dec();
--
Regards,
Changli Gao(xiaosuo@gmail.com)
--
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
* [rfc] IPVS: convert scheduler management to RCU
From: Simon Horman @ 2010-08-20 13:33 UTC (permalink / raw)
To: lvs-devel, netdev, netfilter-devel
Cc: Stephen Hemminger, Wensong Zhang, Julian Anastasov
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);
/* decrease the module use count */
ip_vs_use_count_dec();
^ 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