FAQ
Hello golang-dev@googlegroups.com, bradfitz@golang.org, dave@cheney.net
(cc: golang-dev@googlegroups.com),

Please take another look.


https://codereview.appspot.com/7384048/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Mikioh Mikioh at Feb 25, 2013 at 3:41 pm
    PTAL

    new patchset introduces a new network interface information
    maintenance approach that monitors kernel events using netlink,
    routing sockets.


    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go
    File src/pkg/net/interface.go (right):

    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode26
    src/pkg/net/interface.go:26: )
    On 2013/02/24 02:22:25, dfc wrote:
    suggestion (feel free to ignore it)
    var interfaceCache = struct {
    sync.Mutex
    indexMap map[...]...
    nameMap map[...]...
    }{ indexMap: make(...), nameMap: make(...) }
    then (i think) it is clearer in the code (see below)
    thanks, I revised the structure that keeps interface information.

    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode29
    src/pkg/net/interface.go:29: interfaceMapLock.RLock()
    On 2013/02/24 02:22:25, dfc wrote:
    interfaceCache.Lock()
    defer ...
    ifi, ok := interfaceCache.indexMap[index]
    also removed map.

    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode110
    src/pkg/net/interface.go:110: // Interfaces returns a list of the
    system's network interfaces. It
    On 2013/02/21 17:16:46, bradfitz wrote:
    drop this doc addition. it's an internal detail. callers don't care
    or need to
    know.
    Done.

    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode138
    src/pkg/net/interface.go:138: return interfaceByIndex(index)
    On 2013/02/21 17:16:46, bradfitz wrote:
    shouldn't this still work if the user never called Interfaces() first to
    populate the map?
    changed cache maintenance approach.

    https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode146
    src/pkg/net/interface.go:146: return interfaceByName(name)
    On 2013/02/21 17:16:46, bradfitz wrote:
    likewise.
    also, on miss, it should re-call Interfaces() to re-seed the map.
    what if I look at Interfaces(), create a bridge with
    os/exec.Command("brctl"),
    and then try to call InterfacesByName("br0") and it's not there? but I know it
    is, because I just made it. so you should assume that you failed on a
    miss.

    yup, fixed.

    https://codereview.appspot.com/7384048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Brad Fitzpatrick at Feb 25, 2013 at 3:48 pm
    Why do we need all this?

    Why do interface lookups need to be fast?

    Can't this go in some third-party package, or go.net?

    To pick just one thing to complain about: I don't like the panic in
    interface_linux.go's startInterfaceMonitor. Just because a socket couldn't
    be made, I don't like the idea of crashing the program.

    On Mon, Feb 25, 2013 at 7:41 AM, wrote:

    PTAL

    new patchset introduces a new network interface information
    maintenance approach that monitors kernel events using netlink,
    routing sockets.



    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go>
    File src/pkg/net/interface.go (right):

    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go#newcode26<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode26>
    src/pkg/net/interface.go:26: )
    On 2013/02/24 02:22:25, dfc wrote:

    suggestion (feel free to ignore it)
    var interfaceCache = struct {
    sync.Mutex
    indexMap map[...]...
    nameMap map[...]...
    }{ indexMap: make(...), nameMap: make(...) }
    then (i think) it is clearer in the code (see below)
    thanks, I revised the structure that keeps interface information.


    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go#newcode29<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode29>
    src/pkg/net/interface.go:29: interfaceMapLock.RLock()
    On 2013/02/24 02:22:25, dfc wrote:

    interfaceCache.Lock()
    defer ...
    ifi, ok := interfaceCache.indexMap[index]
    also removed map.


    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go#**newcode110<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode110>
    src/pkg/net/interface.go:110: // Interfaces returns a list of the
    system's network interfaces. It
    On 2013/02/21 17:16:46, bradfitz wrote:

    drop this doc addition. it's an internal detail. callers don't care
    or need to
    know.
    Done.


    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go#**newcode138<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode138>
    src/pkg/net/interface.go:138: return interfaceByIndex(index)
    On 2013/02/21 17:16:46, bradfitz wrote:

    shouldn't this still work if the user never called Interfaces() first to
    populate the map?
    changed cache maintenance approach.


    https://codereview.appspot.**com/7384048/diff/11001/src/**
    pkg/net/interface.go#**newcode146<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode146>
    src/pkg/net/interface.go:146: return interfaceByName(name)
    On 2013/02/21 17:16:46, bradfitz wrote:

    likewise.
    also, on miss, it should re-call Interfaces() to re-seed the map.

    what if I look at Interfaces(), create a bridge with

    os/exec.Command("brctl"),
    and then try to call InterfacesByName("br0") and it's not there? but I know it
    is, because I just made it. so you should assume that you failed on a
    miss.

    yup, fixed.

    https://codereview.appspot.**com/7384048/<https://codereview.appspot.com/7384048/>
    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Mikio Hara at Feb 25, 2013 at 9:57 pm

    On Tue, Feb 26, 2013 at 12:48 AM, Brad Fitzpatrick wrote:

    Why do we need all this?

    Why do interface lookups need to be fast?

    Can't this go in some third-party package, or go.net?
    As issue 4501 describes we have not decided yet to add "Zone string"
    field to ProtoclAddr structs as API but if once we agree it, fast interface
    name to index lookup would be great help to avoid degrading Dial, Listen
    with IPv6 scoped addressing zone processing speed because kernels we
    support now require specifying interface index as link-local scope zone.

    If this CL looks ugly or unnecessary a compromise might be s/string/int/
    like sockaddr_in6 API does.

    Of course if we agree about net package never handle IPv6 scoped
    addressing zone stuff ... yup, I will follow the consensus.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Dave Cheney at Feb 25, 2013 at 11:34 pm
    It feels like this issue (and others) are blocked on,
    https://code.google.com/p/go/issues/detail?id=4501
    On Tue, Feb 26, 2013 at 8:57 AM, Mikio Hara wrote:
    On Tue, Feb 26, 2013 at 12:48 AM, Brad Fitzpatrick wrote:

    Why do we need all this?

    Why do interface lookups need to be fast?

    Can't this go in some third-party package, or go.net?
    As issue 4501 describes we have not decided yet to add "Zone string"
    field to ProtoclAddr structs as API but if once we agree it, fast interface
    name to index lookup would be great help to avoid degrading Dial, Listen
    with IPv6 scoped addressing zone processing speed because kernels we
    support now require specifying interface index as link-local scope zone.

    If this CL looks ugly or unnecessary a compromise might be s/string/int/
    like sockaddr_in6 API does.

    Of course if we agree about net package never handle IPv6 scoped
    addressing zone stuff ... yup, I will follow the consensus.
    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Fullung at Feb 25, 2013 at 8:50 pm

    On 2013/02/25 15:48:24, bradfitz wrote:
    Why do we need all this?
    Why do interface lookups need to be fast?
    FWIW, we reported issue 4866 after we saw timeouts in an internal test
    because interface address lookups suddenly got 166696% slower.

    We had a few goroutines doing a few lookups as part of code that was
    mapping InfiniBand devices to IPoIB addresses via the hardware
    address/GUID. Before it took milliseconds and after the recent changes
    it took tens of seconds.

    Is there a reason that the code that was recently added has to be that
    slow?

    Cheers

    Albert

    https://codereview.appspot.com/7384048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Brad Fitzpatrick at Feb 25, 2013 at 9:14 pm

    On Mon, Feb 25, 2013 at 12:49 PM, wrote:
    On 2013/02/25 15:48:24, bradfitz wrote:

    Why do we need all this?
    Why do interface lookups need to be fast?
    FWIW, we reported issue 4866 after we saw timeouts in an internal test
    because interface address lookups suddenly got 166696% slower.

    We had a few goroutines doing a few lookups as part of code that was
    mapping InfiniBand devices to IPoIB addresses via the hardware
    address/GUID. Before it took milliseconds and after the recent changes
    it took tens of seconds.

    Is there a reason that the code that was recently added has to be that
    slow?
    Issue 4866 says "we added Y and X got slower". This CL says "add Z to make
    X faster". I'm curious whether fixing or removing Y was considered, before
    we just add more stuff.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Mikio Hara at Feb 27, 2013 at 5:35 am
    Hi Albert,

    Sorry for the inconvenience.
    On Tue, Feb 26, 2013 at 5:49 AM, wrote:

    Is there a reason that the code that was recently added has to be that
    slow?
    I chopped this CL down, gathered pieces up except quick dial, listen
    w/ IPv6 scoped addressing zone stuff and put into CL 7400055.

    Can you try https://codereview.appspot.com/7400055/ ?

    Thanks,
    -- MIkio

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedFeb 25, '13 at 3:29p
activeFeb 27, '13 at 5:35a
posts8
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase