All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Timo Teräs" <timo.teras@iki.fi>
To: David Miller <davem@davemloft.net>
Cc: eric.dumazet@gmail.com, netdev@vger.kernel.org
Subject: Re: [PATCH] ipv4: synchronize bind() with RTM_NEWADDR notifications
Date: Thu, 21 Oct 2010 14:29:22 +0300	[thread overview]
Message-ID: <4CC02412.8050000@iki.fi> (raw)
In-Reply-To: <20101021.040319.191412436.davem@davemloft.net>

On 10/21/2010 02:03 PM, David Miller wrote:
> From: Timo Teräs <timo.teras@iki.fi>
> Date: Thu, 21 Oct 2010 13:58:08 +0300
> 
>> On 10/21/2010 01:50 PM, David Miller wrote:
>>> From: Timo Teräs <timo.teras@iki.fi>
>>> Date: Thu, 21 Oct 2010 13:41:37 +0300
>>>
>>>> Is inet_bind() called from non-userland context? If yes, then this is a
>>>> bad idea. Otherwise I don't think it's that hot path...
>>>
>>> It is.
>>
>> Yet, almost immediately after that there is lock_sock() which can also
>> sleep. How does that work then?
> 
> RTNL interlocks globally with a heavy primitive, a mutex, lock_sock()
> grabs a spinlcok which is local to the socket's context.
> 
> So if we have 100,000 sockets binding we'll suck with you're change
> whereas the lock_sock() does not cause that problem.
> 
> Is this so difficult for you to comprehend?

I was confused with Dave's original reply "It is." as referring to that
inet_bind() can get called from non-userland context. But apparently you
just meant that "It is (bad idea regardless)."

I thought the problem was possible sleeping, and not contention. Which
became very obvious from Eric's example. I didn't realize that many do
bind()/recv()/send() as general workload.

Sorry for not seeing the obvious.

This is the third time asking, what would be a good way to fix the
problem described in the original commit log?

Changing RTM_NEWADDR after FIB update would break Netlink event
ordering. And this breaks performance. I can't really use RTN_LOCAL
RTM_NEWROUTE events since (at least IPv6 side) has incorrect ifindex.

Should inet_addr_type() be rewritten to not use FIB lookups?

  reply	other threads:[~2010-10-21 11:29 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-10-21 10:12 [PATCH] ipv4: synchronize bind() with RTM_NEWADDR notifications Timo Teräs
2010-10-21 10:25 ` Eric Dumazet
2010-10-21 10:41   ` Timo Teräs
2010-10-21 10:50     ` David Miller
2010-10-21 10:58       ` Timo Teräs
2010-10-21 11:03         ` David Miller
2010-10-21 11:29           ` Timo Teräs [this message]
2010-10-21 11:34             ` David Miller
2010-10-21 11:57               ` Timo Teräs
2010-10-21 13:06                 ` [PATCH v2] " Timo Teräs
2010-10-21 14:10                   ` Eric Dumazet
2010-10-21 19:01                     ` Timo Teräs
2010-10-21 11:12         ` [PATCH] " Eric Dumazet

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4CC02412.8050000@iki.fi \
    --to=timo.teras@iki.fi \
    --cc=davem@davemloft.net \
    --cc=eric.dumazet@gmail.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.