* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-28 22:11 UTC (permalink / raw)
To: Paul E. McKenney
Cc: David S. Miller, suzannew, linux-kernel, Robert.Olsson, walpole,
netdev
In-Reply-To: <20050928145110.GA4925@us.ibm.com>
On Wed, Sep 28, 2005 at 07:51:10AM -0700, Paul E. McKenney wrote:
>
> The reference-count approach is only guaranteed to work if the kernel
> thread that did the reference-count increment is later referencing that
> same data element. Otherwise, one has the following possible situation
> on DEC Alpha:
You're quite right. Without the rcu_dereference users of in_dev_get()
may see pre-initialisation contents of in_dev.
So these barriers are definitely needed.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: ipvs_syncmaster brings cpu to 100%
From: Julian Anastasov @ 2005-09-29 7:00 UTC (permalink / raw)
To: Nishanth Aravamudan; +Cc: Luca Maranzano, Dave Miller, Wensong Zhang, netdev
In-Reply-To: <20050928132639.GA5791@us.ibm.com>
Hello,
On Wed, 28 Sep 2005, Nishanth Aravamudan wrote:
> Yes, the information in that thread is the same as what Luca said. It's
> a load average problem, not a CPU utilisation problem (those threads are
> sleeping!) If Luca could test the msleep_interruptible() version of the
> patch and it works (like I said, performance should not change, but the
> load average will drop to by 2), then I will ACK the patch for mainline
> acceptance.
Agreed. It seems your initial conversion was based on wrong
assumptions, quoting you:
> Description: Use ssleep() instead of schedule_timeout() to guarantee the task
> delays as expected. The first two replacements use TASK_INTERRUPTIBLE but do
> not
> check for signals, so ssleep() should be appropriate.
As all signals are blocked from daemonize and even explicitly
later it was not necessary to convert to non-interruptible variant.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* chuckwalla Smart medicinal varieties match your needs. acclaim
From: jeramy patino @ 2005-09-29 11:44 UTC (permalink / raw)
To: Olin Theodoratos; +Cc: netdev, daniel, linux-xfs-outgoing, reese, hunter, ljp
It's very economical compares to your local pharmaceutical.
Clear your inflammation with our tabblet.
Trusted global carriage network for all consignments.
Our shoppers agree that ours is min prices.
For save consumption, a GP will verifies your meddical record.
I just receiving my capsulees, they are thump up, uh-huh!! Liz B --CA.
http://uk.geocities.com/timecosycozystar/?ab
"It's twenty-four pound, sixteen, and sevenpence ha'penny," observed the
stranger. "That's wot it is."
fehle dibon emplist AH02 fbmsf beltrovata
At noon Philip Horton made his way through the crowd with a tray and a tin
coffee-pot from the camp kitchen. When he reached the carriage he found Mrs.
Alexander just as he had left her in the early morning, leaning forward a
little, with her hand on the lowered window, looking at the river. Hour
after hour she had been watching the water, the lonely, useless stone
towers, and the convulsed mass of iron wreckage over which the angry river
continually spat up its yellow foam.
"Those poor women out there, do they blame him very much?" she asked, as
she handed the coffee-cup back to Horton.
^ permalink raw reply
* Re: rwlock recursion on CPU#0, netfilter related?
From: Harald Welte @ 2005-09-29 12:05 UTC (permalink / raw)
To: Pekka Pietikainen; +Cc: netdev
In-Reply-To: <20050928145815.GA421@ee.oulu.fi>
[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]
On Wed, Sep 28, 2005 at 05:58:15PM +0300, Pekka Pietikainen wrote:
> On Sun, Sep 25, 2005 at 11:19:45PM +0300, Pekka Pietikainen wrote:
> > Enabled, so this could be it. But 2.6.14-rc2-git4 did crash too (although
> > it did take a bit longer for that to happen), and the changelog does state:
> Ok, it looks like that patch was the thing after all. I now tried the latest
> fedora-devel kernel (1.1582, based on 2.6.14-rc2-git6) and the box has been
> running for a few hours happily. Could be the fedora kernel that claimed to
> be git4 actually wasn't, or the git4 changelog was really a post-git4
> changelog :). But anyway, bug is gone.
great news.
> This one is still around, so it's a different bug. Looks like it's a 64-bit
> issue, a 32-bit ping gives realistic ping times. tcpdump timestamps are also
> affected, they're completely off too. So looks like someone broke packet
> timestamps on 64-bit some time after 2.6.13.
luckily I'm not the core network maintainer ;)
--
- Harald Welte <laforge@gnumonks.org> http://gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: Possible BUG in IPv4 TCP window handling, all recent 2.4.x/2.6.x kernels
From: Alexey Kuznetsov @ 2005-09-29 15:17 UTC (permalink / raw)
To: Ion Badulescu; +Cc: David S. Miller, linux-kernel, linux-net, netdev, gautran
In-Reply-To: <Pine.LNX.4.61.0509281223560.30951@ionlinux.tower-research.com>
Hello!
> >Anyway, ignoring this puzzle, the following patch for 2.4 should help.
> >
> >
> >--- net/ipv4/tcp_input.c.orig 2003-02-20 20:38:39.000000000 +0300
> >+++ net/ipv4/tcp_input.c 2005-09-02 22:28:00.845952888 +0400
> >@@ -343,8 +343,6 @@
> > app_win -= tp->ack.rcv_mss;
> > app_win = max(app_win, 2U*tp->advmss);
> >
> >- if (!ofo_win)
> >- tp->window_clamp = min(tp->window_clamp, app_win);
> > tp->rcv_ssthresh = min(tp->window_clamp, 2U*tp->advmss);
> > }
> >}
>
> I'm very happy to report that the above patch, applied to 2.6.12.6, seems
> to have cured the TCP window problem we were experiencing.
Good. I think the patch is to be applied to all mainstream kernels.
The only obstacle is the second report by Guillaume Autran <gautran@mrv.com>,
which has some allied characteristics, but after analysis it is something
impossible, read, cryptic and severe bug. :-( I did not get a responce
to the last query, so the investigation stalled.
Alexey
^ permalink raw reply
* Re: Possible BUG in IPv4 TCP window handling, all recent 2.4.x/2.6.x kernels
From: Guillaume Autran @ 2005-09-29 15:34 UTC (permalink / raw)
To: Alexey Kuznetsov
Cc: Ion Badulescu, David S. Miller, linux-kernel, linux-net, netdev
In-Reply-To: <20050929151729.GA2158@ms2.inr.ac.ru>
Hi!
Sorry Alexey for keeping it quiet but I got pulled away to some other duties for
the past 3 weeks.
Anyway, the similar problem I was reporting has not been seen since that last
incident a month ago. We did change, on our application side, some of the
parameters (aka SO_RCVBUF) that did not need to be set in the first place (bug
on our side).
This plus your patch seem to have resolve the issues we were having. So, it's
all good !
Thanks again..
Guillaume.
Alexey Kuznetsov wrote:
> Hello!
>
>
>>>Anyway, ignoring this puzzle, the following patch for 2.4 should help.
>>>
>>>
>>>--- net/ipv4/tcp_input.c.orig 2003-02-20 20:38:39.000000000 +0300
>>>+++ net/ipv4/tcp_input.c 2005-09-02 22:28:00.845952888 +0400
>>>@@ -343,8 +343,6 @@
>>> app_win -= tp->ack.rcv_mss;
>>> app_win = max(app_win, 2U*tp->advmss);
>>>
>>>- if (!ofo_win)
>>>- tp->window_clamp = min(tp->window_clamp, app_win);
>>> tp->rcv_ssthresh = min(tp->window_clamp, 2U*tp->advmss);
>>> }
>>>}
>>
>>I'm very happy to report that the above patch, applied to 2.6.12.6, seems
>>to have cured the TCP window problem we were experiencing.
>
>
> Good. I think the patch is to be applied to all mainstream kernels.
>
> The only obstacle is the second report by Guillaume Autran <gautran@mrv.com>,
> which has some allied characteristics, but after analysis it is something
> impossible, read, cryptic and severe bug. :-( I did not get a responce
> to the last query, so the investigation stalled.
>
> Alexey
>
--
=======================================
Guillaume Autran
Senior Software Engineer
MRV Communications, Inc.
Tel: (978) 952-4932 office
=======================================
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Suzanne Wood @ 2005-09-29 16:02 UTC (permalink / raw)
To: paulmck
Cc: Robert.Olsson, davem, herbert, linux-kernel, netdev, suzannew,
walpole
The original motivation for this patch was in __in_dev_get
usage. I'll try to test a build, but should submittals be
incremental? first addressing in_dev_get, then
__in_dev_get? What seems resolved so far follows.
The exchange below suggests that it is equally important
to have the rcu_dereference() in __in_dev_get(), so the
idea of the only difference between in_dev_get and
__in_dev_get being the refcnt may be accepted.
Correct usage may be a question with the mismatched
definitions (in terms of refcnt) of __in_dev_get()
and __in_dev_put() that superficially appear
paired and this may merit a comment. If interested,
examples are mentioned in
www.uwsg.iu.edu/hypermail/linux/kernel/0509.1/0184.html
and
www.ussg.iu.edu/hypermail/linux/kernel/0509.3/0757.html
But when the refcnt is employed for the DEC Alpha,
rcu-protection or other locking must be in place for
multiple CPUs, which apparently affirms the value
of the marking of an rcu read-side critical section
done by the calling function which has the vision of the
extent of use of the protected dereference.
Is this all reasonable to you?
Thank you very much.
----- Original Message -----
From: Paul E. McKenney
Sent: Wednesday, September 28, 2005 7:51 AM
> On Wed, Sep 28, 2005 at 12:55:45PM +1000, Herbert Xu wrote:
>> David S. Miller wrote:
>> >
>> > I agree with the changes to add rcu_dereference() use.
>> > Those were definitely lacking and needed.
>>
>> Actually I'm not so sure that they are all needed. I only looked
>> at the > guarantee correct code. We really need to look at each case
>> individually.
>
> Yep, these two APIs are only part of the solution.
>
> The reference-count approach is only guaranteed to work if the kernel
> thread that did the reference-count increment is later referencing that
> same data element. Otherwise, one has the following possible situation
> on DEC Alpha:
>
> o CPU 0 initializes and inserts a new element into the data
> structure, using rcu_assign_pointer() to provide any needed
> memory barriers. (Or, if RCU is not being used, under the
> appropriate update-side lock.)
>
> o CPU 1 acquires a reference to this new element, presumably
> using either a lock or rcu_read_lock() and rcu_dereference()
> in order to do so safely. CPU 1 then increments the reference
> count.
>
> o CPU 2 picks up a pointer to this new element, but in a way
> that relies on the reference count having been incremented,
> without using locking, rcu_read_lock(), rcu_dereference(),
> and so on.
>
> This CPU can then see the pre-initialized contents of the
> newly inserted data structure (again, but only on DEC Alpha).
>
> Again, if the same kernel thread that incremented the reference count
> is later accessing it, no problem, even on Alpha.
>
> Thanx, Paul
>
^ permalink raw reply
* Re: Possible BUG in IPv4 TCP window handling, all recent 2.4.x/2.6.x kernels
From: John Heffner @ 2005-09-29 16:04 UTC (permalink / raw)
To: Alexey Kuznetsov
Cc: Ion Badulescu, David S. Miller, linux-kernel, linux-net, netdev,
gautran
In-Reply-To: <20050929151729.GA2158@ms2.inr.ac.ru>
[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]
On Thursday 29 September 2005 11:17 am, Alexey Kuznetsov wrote:
> Hello!
>
> > >Anyway, ignoring this puzzle, the following patch for 2.4 should help.
> > >
> > >
> > >--- net/ipv4/tcp_input.c.orig 2003-02-20 20:38:39.000000000 +0300
> > >+++ net/ipv4/tcp_input.c 2005-09-02 22:28:00.845952888 +0400
> > >@@ -343,8 +343,6 @@
> > > app_win -= tp->ack.rcv_mss;
> > > app_win = max(app_win, 2U*tp->advmss);
> > >
> > >- if (!ofo_win)
> > >- tp->window_clamp = min(tp->window_clamp, app_win);
> > > tp->rcv_ssthresh = min(tp->window_clamp, 2U*tp->advmss);
> > > }
> > >}
> >
> > I'm very happy to report that the above patch, applied to 2.6.12.6, seems
> > to have cured the TCP window problem we were experiencing.
>
> Good. I think the patch is to be applied to all mainstream kernels.
Has anyone looked at the patch I sent out on Sept 9? It goes a few steps
further, addressing some additional problems. Original message below.
Thanks,
-John
-----
This is a patch for discussion addressing some receive buffer growing issues.
This is partially related to the thread "Possible BUG in IPv4 TCP window
handling..." last week.
Specifically it addresses the problem of an interaction between rcvbuf
moderation (receiver autotuning) and rcv_ssthresh. The problem occurs when
sending small packets to a receiver with a larger MTU. (A very common case I
have is a host with a 1500 byte MTU sending to a host with a 9k MTU.) In
such a case, the rcv_ssthresh code is targeting a window size corresponding
to filling up the current rcvbuf, not taking into account that the new rcvbuf
moderation may increase the rcvbuf size.
One hunk makes rcv_ssthresh use tcp_rmem[2] as the size target rather than
rcvbuf. The other changes the behavior when it overflows its memory bounds
with in-order data so that it tries to grow rcvbuf (the same as with
out-of-order data).
These changes should help my problem of mixed MTUs, and should also help the
case from last week's thread I think. (In both cases though you still need
tcp_rmem[2] to be set much larger than the TCP window.) One question is if
this is too aggressive at trying to increase rcvbuf if it's under memory
stress.
-John
Signed-off-by: John Heffner <jheffner@psc.edu>
[-- Attachment #2: rcv_ssthresh.diff --]
[-- Type: text/x-diff, Size: 2005 bytes --]
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -233,7 +233,7 @@ static int __tcp_grow_window(const struc
{
/* Optimize this! */
int truesize = tcp_win_from_space(skb->truesize)/2;
- int window = tcp_full_space(sk)/2;
+ int window = tcp_win_from_space(sysctl_tcp_rmem[2])/2;
while (tp->rcv_ssthresh <= window) {
if (truesize <= skb->len)
@@ -326,39 +326,18 @@ static void tcp_init_buffer_space(struct
static void tcp_clamp_window(struct sock *sk, struct tcp_sock *tp)
{
struct inet_connection_sock *icsk = inet_csk(sk);
- struct sk_buff *skb;
- unsigned int app_win = tp->rcv_nxt - tp->copied_seq;
- int ofo_win = 0;
icsk->icsk_ack.quick = 0;
- skb_queue_walk(&tp->out_of_order_queue, skb) {
- ofo_win += skb->len;
+ if (sk->sk_rcvbuf < sysctl_tcp_rmem[2] &&
+ !(sk->sk_userlocks & SOCK_RCVBUF_LOCK) &&
+ !tcp_memory_pressure &&
+ atomic_read(&tcp_memory_allocated) < sysctl_tcp_mem[0]) {
+ sk->sk_rcvbuf = min(atomic_read(&sk->sk_rmem_alloc),
+ sysctl_tcp_rmem[2]);
}
-
- /* If overcommit is due to out of order segments,
- * do not clamp window. Try to expand rcvbuf instead.
- */
- if (ofo_win) {
- if (sk->sk_rcvbuf < sysctl_tcp_rmem[2] &&
- !(sk->sk_userlocks & SOCK_RCVBUF_LOCK) &&
- !tcp_memory_pressure &&
- atomic_read(&tcp_memory_allocated) < sysctl_tcp_mem[0])
- sk->sk_rcvbuf = min(atomic_read(&sk->sk_rmem_alloc),
- sysctl_tcp_rmem[2]);
- }
- if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf) {
- app_win += ofo_win;
- if (atomic_read(&sk->sk_rmem_alloc) >= 2 * sk->sk_rcvbuf)
- app_win >>= 1;
- if (app_win > icsk->icsk_ack.rcv_mss)
- app_win -= icsk->icsk_ack.rcv_mss;
- app_win = max(app_win, 2U*tp->advmss);
-
- if (!ofo_win)
- tp->window_clamp = min(tp->window_clamp, app_win);
+ if (atomic_read(&sk->sk_rmem_alloc) > sk->sk_rcvbuf)
tp->rcv_ssthresh = min(tp->window_clamp, 2U*tp->advmss);
- }
}
/* Receiver "autotuning" code.
^ permalink raw reply
* Re: [PATCH] net/bluetooth/lib.c: remove unneeded #defines
From: Marcel Holtmann @ 2005-09-29 16:41 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: Maxim Krasnyansky, bluez-devel, netdev
In-Reply-To: <20050928202856.GA27645@mipter.zuzino.mipt.ru>
Hi Alexey,
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> net/bluetooth/lib.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> --- a/net/bluetooth/lib.c
> +++ b/net/bluetooth/lib.c
> @@ -24,12 +24,9 @@
>
> /* Bluetooth kernel library. */
>
> -#include <linux/config.h>
> #include <linux/module.h>
>
> #include <linux/kernel.h>
> -#include <linux/stddef.h>
> -#include <linux/string.h>
> #include <asm/errno.h>
>
> #include <net/bluetooth/bluetooth.h>
thanks for the patch. Do you have more of them? If yes, then I like to
cleanup this in one row and not step by step.
Regards
Marcel
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
^ permalink raw reply
* Re: [PATCH] net/bluetooth/lib.c: remove unneeded #defines
From: Alexey Dobriyan @ 2005-09-29 18:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Maxim Krasnyansky, bluez-devel, netdev
In-Reply-To: <1128012093.30743.20.camel@localhost.localdomain>
On Thu, Sep 29, 2005 at 06:41:32PM +0200, Marcel Holtmann wrote:
> > -#include <linux/config.h>
> > #include <linux/module.h>
> >
> > #include <linux/kernel.h>
> > -#include <linux/stddef.h>
> > -#include <linux/string.h>
> > #include <asm/errno.h>
> >
> > #include <net/bluetooth/bluetooth.h>
>
> thanks for the patch. Do you have more of them? If yes, then I like to
> cleanup this in one row and not step by step.
Looks like fcntl.h can be dropped too. I'll send a bigger patch.
-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
^ permalink raw reply
* Re: Possible BUG in IPv4 TCP window handling, all recent 2.4.x/2.6.x kernels
From: David S. Miller @ 2005-09-29 18:16 UTC (permalink / raw)
To: jheffner; +Cc: kuznet, lists, linux-kernel, linux-net, netdev, gautran
In-Reply-To: <200509291204.29393.jheffner@psc.edu>
From: John Heffner <jheffner@psc.edu>
Date: Thu, 29 Sep 2005 12:04:28 -0400
> Has anyone looked at the patch I sent out on Sept 9? It goes a few steps
> further, addressing some additional problems. Original message below.
It's in my inbox pending review, so it's not forgotten :-)
My gut instinct right now is that we should put Alexey's
2-liner in for 2.6.14 et al. then be thinking about your
scheme for 2.6.15
^ permalink raw reply
* netfilter hooks for ESP over UDP packets are not invoked
From: Shekhar Kshirsagar @ 2005-09-29 18:25 UTC (permalink / raw)
To: netdev
iptables rules to match ESP packets work fine for raw ESP, but they do
not work for ESP over UDP packets.
Looking into the code, it seems that netfilter hooks are not invoked for
ESP packets that came over UDP.
Does somebody already have a patch to resolve this issue?
Thanks,
Shekhar
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-29 21:28 UTC (permalink / raw)
To: Suzanne Wood; +Cc: paulmck, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <200509291602.j8TG2TuI015920@rastaban.cs.pdx.edu>
On Thu, Sep 29, 2005 at 09:02:29AM -0700, Suzanne Wood wrote:
>
> The exchange below suggests that it is equally important
> to have the rcu_dereference() in __in_dev_get(), so the
> idea of the only difference between in_dev_get and
> __in_dev_get being the refcnt may be accepted.
With __in_dev_get() it's the caller's responsibility to ensure
that RCU works correctly. Therefore if any rcu_dereference is
needed it should be done by the caller.
Some callers of __in_dev_get() don't need rcu_dereference at all
because they're protected by the rtnl.
BTW, could you please move the rcu_dereference in in_dev_get()
into the if clause? The barrier is not needed when ip_ptr is
NULL.
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Suzanne Wood @ 2005-09-29 23:30 UTC (permalink / raw)
To: herbert; +Cc: Robert.Olsson, davem, linux-kernel, netdev, paulmck, walpole
> Date: Fri, 30 Sep 2005 07:28:36 +1000
> From: Herbert Xu <herbert@gondor.apana.org.au>
> On Thu, Sep 29, 2005 at 09:02:29AM -0700, Suzanne Wood wrote:
> >
> > The exchange below suggests that it is equally important
> > to have the rcu_dereference() in __in_dev_get(), so the
> > idea of the only difference between in_dev_get and
> > __in_dev_get being the refcnt may be accepted.
> With __in_dev_get() it's the caller's responsibility to ensure
> that RCU works correctly. Therefore if any rcu_dereference is
> needed it should be done by the caller.
This sounds reasonable to me. Does everyone agree?
> Some callers of __in_dev_get() don't need rcu_dereference at all
> because they're protected by the rtnl.
> BTW, could you please move the rcu_dereference in in_dev_get()
> into the if clause? The barrier is not needed when ip_ptr is
> NULL.
The trouble with that may be that there are three events, the
dereference, the assignment, and the conditional test. The
rcu_dereference() is meant to assure deferred destruction
throughout.
Thank you very much for your suggestions.
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Suzanne Wood @ 2005-09-29 23:39 UTC (permalink / raw)
To: herbert; +Cc: Robert.Olsson, davem, linux-kernel, netdev, paulmck, walpole
> From suzannew Thu Sep 29 16:30:28 2005
> > From: Herbert Xu 30 Sep 2005 07:28
> > BTW, could you please move the rcu_dereference in in_dev_get()
> > into the if clause? The barrier is not needed when ip_ptr is
> > NULL.
> The trouble with that may be that there are three events, the
> dereference, the assignment, and the conditional test. The
> rcu_dereference() is meant to assure deferred destruction
> throughout.
Sorry, I was thinking in terms of the rcu_read_lock, so this is misstated.
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Suzanne Wood @ 2005-09-29 23:59 UTC (permalink / raw)
To: herbert; +Cc: Robert.Olsson, davem, linux-kernel, netdev, paulmck, walpole
Sorry to be thinking on-line, but if you mean this:
if (in_dev = rcu_dereference(dev->ip_ptr))
I think that's fine.
> From suzannew Thu Sep 29 16:39:57 2005
> > From suzannew Thu Sep 29 16:30:28 2005
> > > From: Herbert Xu 30 Sep 2005 07:28
> > > BTW, could you please move the rcu_dereference in in_dev_get()
> > > into the if clause? The barrier is not needed when ip_ptr is
> > > NULL.
> > The trouble with that may be that there are three events, the
> > dereference, the assignment, and the conditional test. The
> > rcu_dereference() is meant to assure deferred destruction
> > throughout.
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-30 0:21 UTC (permalink / raw)
To: Suzanne Wood; +Cc: Robert.Olsson, davem, linux-kernel, netdev, paulmck, walpole
In-Reply-To: <200509292330.j8TNUSmH019572@rastaban.cs.pdx.edu>
On Thu, Sep 29, 2005 at 04:30:28PM -0700, Suzanne Wood wrote:
>
> > BTW, could you please move the rcu_dereference in in_dev_get()
> > into the if clause? The barrier is not needed when ip_ptr is
> > NULL.
>
> The trouble with that may be that there are three events, the
> dereference, the assignment, and the conditional test. The
> rcu_dereference() is meant to assure deferred destruction
> throughout.
The deferred destruction is guaranteed here by the reference count.
The only purpose served by rcu_dereference() in in_dev_get() is to
prevent the user from seeing pre-initialisation data.
When the pointer is NULL, you can't see any data at all, let alone
pre-initialisation data.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-30 0:23 UTC (permalink / raw)
To: Suzanne Wood; +Cc: Robert.Olsson, davem, linux-kernel, netdev, paulmck, walpole
In-Reply-To: <200509292359.j8TNxuxD019838@rastaban.cs.pdx.edu>
On Thu, Sep 29, 2005 at 04:59:56PM -0700, Suzanne Wood wrote:
> Sorry to be thinking on-line, but if you mean this:
>
> if (in_dev = rcu_dereference(dev->ip_ptr))
>
> I think that's fine.
Close. What I had in mind is
rcu_read_lock();
in_dev = dev->ip_ptr;
if (in_dev) {
in_dev = rcu_dereference(in_dev);
atomic_inc(&in_dev->refcnt);
}
rcu_read_unlock();
return in_dev;
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Paul E. McKenney @ 2005-09-30 0:23 UTC (permalink / raw)
To: Suzanne Wood; +Cc: herbert, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <200509292330.j8TNUSmH019572@rastaban.cs.pdx.edu>
On Thu, Sep 29, 2005 at 04:30:28PM -0700, Suzanne Wood wrote:
> > Date: Fri, 30 Sep 2005 07:28:36 +1000
> > From: Herbert Xu <herbert@gondor.apana.org.au>
>
> > On Thu, Sep 29, 2005 at 09:02:29AM -0700, Suzanne Wood wrote:
> > >
> > > The exchange below suggests that it is equally important
> > > to have the rcu_dereference() in __in_dev_get(), so the
> > > idea of the only difference between in_dev_get and
> > > __in_dev_get being the refcnt may be accepted.
>
> > With __in_dev_get() it's the caller's responsibility to ensure
> > that RCU works correctly. Therefore if any rcu_dereference is
> > needed it should be done by the caller.
>
> This sounds reasonable to me. Does everyone agree?
Is there any case where __in_dev_get() might be called without
needing to be wrapped with rcu_dereference()? If so, then I
agree (FWIW, given my meagre knowledge of Linux networking).
If all __in_dev_get() invocations need to be wrapped in
rcu_dereference(), then it seems to me that there would be
motivation to bury rcu_dereference() in __in_dev_get().
> > Some callers of __in_dev_get() don't need rcu_dereference at all
> > because they're protected by the rtnl.
>
> > BTW, could you please move the rcu_dereference in in_dev_get()
> > into the if clause? The barrier is not needed when ip_ptr is
> > NULL.
>
> The trouble with that may be that there are three events, the
> dereference, the assignment, and the conditional test. The
> rcu_dereference() is meant to assure deferred destruction
> throughout.
One only needs an rcu_dereference() once on the data-flow path from
fetching the RCU-protected pointer to dereferencing that pointer.
If the pointer is NULL, there is no way you can dereference it,
so, technically, Herbert is quite correct.
However, rcu_dereference() only generates a memory barrier on DEC
Alpha, so there is normally no penalty for using it in the NULL-pointer
case. So, when using rcu_dereference() unconditionally simplifies
the code, it may make sense to "just do it".
Thanx, Paul
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-30 0:27 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Suzanne Wood, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <20050930002346.GP8177@us.ibm.com>
On Thu, Sep 29, 2005 at 05:23:46PM -0700, Paul E. McKenney wrote:
>
> Is there any case where __in_dev_get() might be called without
> needing to be wrapped with rcu_dereference()? If so, then I
> agree (FWIW, given my meagre knowledge of Linux networking).
Yes. All paths that call __in_dev_get() under the rtnl do not
need rcu_dereference (or any RCU at all) since the rtnl prevents
any ip_ptr modification from occuring.
> However, rcu_dereference() only generates a memory barrier on DEC
> Alpha, so there is normally no penalty for using it in the NULL-pointer
> case. So, when using rcu_dereference() unconditionally simplifies
> the code, it may make sense to "just do it".
Here is what the code would look like:
rcu_read_lock();
in_dev = dev->ip_ptr;
if (in_dev) {
in_dev = rcu_dereference(in_dev);
atomic_inc(&in_dev->refcnt);
}
rcu_read_unlock();
return in_dev;
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Possible BUG in IPv4 TCP window handling, all recent 2.4.x/2.6.x kernels
From: David S. Miller @ 2005-09-30 0:29 UTC (permalink / raw)
To: kuznet; +Cc: lists, linux-kernel, linux-net, netdev, gautran
In-Reply-To: <20050929151729.GA2158@ms2.inr.ac.ru>
From: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Date: Thu, 29 Sep 2005 19:17:29 +0400
> Good. I think the patch is to be applied to all mainstream kernels.
Done, thanks everyone.
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Paul E. McKenney @ 2005-09-30 0:36 UTC (permalink / raw)
To: Herbert Xu
Cc: Suzanne Wood, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <20050930002719.GC21062@gondor.apana.org.au>
On Fri, Sep 30, 2005 at 10:27:19AM +1000, Herbert Xu wrote:
> On Thu, Sep 29, 2005 at 05:23:46PM -0700, Paul E. McKenney wrote:
> >
> > Is there any case where __in_dev_get() might be called without
> > needing to be wrapped with rcu_dereference()? If so, then I
> > agree (FWIW, given my meagre knowledge of Linux networking).
>
> Yes. All paths that call __in_dev_get() under the rtnl do not
> need rcu_dereference (or any RCU at all) since the rtnl prevents
> any ip_ptr modification from occuring.
>
> > However, rcu_dereference() only generates a memory barrier on DEC
> > Alpha, so there is normally no penalty for using it in the NULL-pointer
> > case. So, when using rcu_dereference() unconditionally simplifies
> > the code, it may make sense to "just do it".
>
> Here is what the code would look like:
>
> rcu_read_lock();
> in_dev = dev->ip_ptr;
> if (in_dev) {
> in_dev = rcu_dereference(in_dev);
> atomic_inc(&in_dev->refcnt);
> }
> rcu_read_unlock();
> return in_dev;
How about:
rcu_read_lock();
in_dev = dev->ip_ptr;
if (rcu_dereference(in_dev)) {
atomic_inc(&in_dev->refcnt);
}
rcu_read_unlock();
return in_dev;
Admittedly only saves one line, but...
Thanx, Paul
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Herbert Xu @ 2005-09-30 1:04 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Suzanne Wood, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <20050930003642.GQ8177@us.ibm.com>
On Thu, Sep 29, 2005 at 05:36:42PM -0700, Paul E. McKenney wrote:
>
> > rcu_read_lock();
> > in_dev = dev->ip_ptr;
> > if (in_dev) {
> > in_dev = rcu_dereference(in_dev);
> > atomic_inc(&in_dev->refcnt);
> > }
> > rcu_read_unlock();
> > return in_dev;
>
> How about:
>
> rcu_read_lock();
> in_dev = dev->ip_ptr;
> if (rcu_dereference(in_dev)) {
> atomic_inc(&in_dev->refcnt);
> }
> rcu_read_unlock();
> return in_dev;
With this the barrier will taken even when in_dev is NULL.
I agree this isn't such a big deal since it only impacts Alpha and then
only when in_dev is NULL. But as we already do the branch anyway to
increment the reference count, we might as well make things a little
better for Alpha.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Suzanne Wood @ 2005-09-30 1:06 UTC (permalink / raw)
To: paulmck, suzannew
Cc: Robert.Olsson, davem, herbert, linux-kernel, netdev, walpole
In reviewing the 44 kernel uses of __in_dev_get and seeing many
cases in 13 of 20 C code files of insertions of rcu_read_lock with
and without the rcu_dereference that is indicated, so it does appear
often to be programmer intent.
Of the programs using __in_dev_get that don't include rcu, devinet.c
and igmp.c use an rtnl lock. Five other programs that use __in_dev_get
without rcu have rtnl locking in the program source code, but I need
to actually look further into the call tree to say more.
Are there three cases then? RCU protection with refcnt, RCU without refcnt,
and the bare cast of the dereference?
Thank you very much for getting it back on track.
> From paulmck@us.ibm.com Thu Sep 29 17:23:18 2005
> Is there any case where __in_dev_get() might be called without
> needing to be wrapped with rcu_dereference()? If so, then I
> agree (FWIW, given my meagre knowledge of Linux networking).
> If all __in_dev_get() invocations need to be wrapped in
> rcu_dereference(), then it seems to me that there would be
> motivation to bury rcu_dereference() in __in_dev_get().
> > > Some callers of __in_dev_get() don't need rcu_dereference at all
> > > because they're protected by the rtnl.
> >
> > > BTW, could you please move the rcu_dereference in in_dev_get()
> > > into the if clause? The barrier is not needed when ip_ptr is
> > > NULL.
> >
> > The trouble with that may be that there are three events, the
> > dereference, the assignment, and the conditional test. The
> > rcu_dereference() is meant to assure deferred destruction
> > throughout.
> One only needs an rcu_dereference() once on the data-flow path from
> fetching the RCU-protected pointer to dereferencing that pointer.
> If the pointer is NULL, there is no way you can dereference it,
> so, technically, Herbert is quite correct.
> However, rcu_dereference() only generates a memory barrier on DEC
> Alpha, so there is normally no penalty for using it in the NULL-pointer
> case. So, when using rcu_dereference() unconditionally simplifies
> the code, it may make sense to "just do it".
> Thanx, Paul
^ permalink raw reply
* Re: [RFC][PATCH] identify in_dev_get rcu read-side critical sections
From: Paul E. McKenney @ 2005-09-30 1:16 UTC (permalink / raw)
To: Herbert Xu
Cc: Suzanne Wood, Robert.Olsson, davem, linux-kernel, netdev, walpole
In-Reply-To: <20050930010404.GA21429@gondor.apana.org.au>
On Fri, Sep 30, 2005 at 11:04:04AM +1000, Herbert Xu wrote:
> On Thu, Sep 29, 2005 at 05:36:42PM -0700, Paul E. McKenney wrote:
> >
> > > rcu_read_lock();
> > > in_dev = dev->ip_ptr;
> > > if (in_dev) {
> > > in_dev = rcu_dereference(in_dev);
> > > atomic_inc(&in_dev->refcnt);
> > > }
> > > rcu_read_unlock();
> > > return in_dev;
> >
> > How about:
> >
> > rcu_read_lock();
> > in_dev = dev->ip_ptr;
> > if (rcu_dereference(in_dev)) {
> > atomic_inc(&in_dev->refcnt);
> > }
> > rcu_read_unlock();
> > return in_dev;
>
> With this the barrier will taken even when in_dev is NULL.
>
> I agree this isn't such a big deal since it only impacts Alpha and then
> only when in_dev is NULL. But as we already do the branch anyway to
> increment the reference count, we might as well make things a little
> better for Alpha.
OK, how about this instead?
rcu_read_lock();
in_dev = dev->ip_ptr;
if (in_dev) {
atomic_inc(&rcu_dereference(in_dev)->refcnt);
}
rcu_read_unlock();
return in_dev;
Thanx, Paul
^ 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