Ticket #9377: 0001-netstack-Interface-Improve-locking.patch

File 0001-netstack-Interface-Improve-locking.patch, 4.6 KB (added by jackburton, 10 years ago)

Second try

  • src/add-ons/kernel/network/stack/interfaces.cpp 

    From fb109f282c5206d834ac8a054902b60d1b7627c5 Mon Sep 17 00:00:00 2001
From: Stefano Ceccherini <stefano.ceccherini@gmail.com>
Date: Sat, 23 Aug 2014 18:23:38 +0200
Subject: [PATCH] netstack: Interface: Improve locking. Don't acquire the
 interface lock in ::_SetUp() and SetDown() before calling the lower layers.
 Grab the all interfaces lock before setting the interface busy. Mark the
 interface busy instead, using the newly added SetBusy() method. In
 get_interface() and SetDown(): check if the interface is busy.

---
 src/add-ons/kernel/network/stack/interfaces.cpp | 33 ++++++++++++++++---------
 src/add-ons/kernel/network/stack/interfaces.h   |  4 +++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/add-ons/kernel/network/stack/interfaces.cpp b/src/add-ons/kernel/network/stack/interfaces.cpp
index 20b4110..0c08286 100644
    a b Interface::SetDown()  
    933933    if ((flags & IFF_UP) == 0)
    934934        return;
    935935
    936     // TODO: We acquire also the net_interfaces lock here
    937     // to avoid a lock inversion in the ipv6 protocol implementation
    938     // (see ticket #9377).
    939     // A better solution would be to avoid locking the interface lock (fLock)
    940     // when calling the lower layers.
    941     RecursiveLocker interfacesLocker(sLock);
    942     RecursiveLocker locker(fLock);
     936    if (IsBusy())
     937        return;
     938
     939    RecursiveLocker interfacesLock(sLock);
     940    SetBusy(true);
     941    interfacesLock.Unlock();
    943942
    944943    DatalinkTable::Iterator iterator = fDatalinkTable.GetIterator();
    945944    while (domain_datalink* datalink = iterator.Next()) {
    … … Interface::SetDown()  
    947946    }
    948947
    949948    flags &= ~IFF_UP;
     949
     950    SetBusy(false);
    950951}
    951952
    952953
    … … Interface::_SetUp()  
    10761077    if (status != B_OK)
    10771078        return status;
    10781079
    1079     // Propagate flag to all datalink protocols
    1080 
    1081     RecursiveLocker locker(fLock);
     1080    RecursiveLocker interfacesLocker(sLock);
     1081    SetBusy(true);
     1082    interfacesLocker.Unlock();
    10821083
     1084    // Propagate flag to all datalink protocols
    10831085    DatalinkTable::Iterator iterator = fDatalinkTable.GetIterator();
    10841086    while (domain_datalink* datalink = iterator.Next()) {
    10851087        status = datalink->first_info->interface_up(datalink->first_protocol);
    … … Interface::_SetUp()  
    10971099            }
    10981100
    10991101            down_device_interface(fDeviceInterface);
     1102            SetBusy(false);
    11001103            return status;
    11011104        }
    11021105    }
    … … Interface::_SetUp()  
    11091112    }
    11101113
    11111114    flags |= IFF_UP;
     1115    SetBusy(false);
     1116
    11121117    return B_OK;
    11131118}
    11141119
    … … get_interface(net_domain* domain, uint32 index)  
    14091414        interface = sInterfaces.First();
    14101415    else
    14111416        interface = find_interface(index);
    1412     if (interface == NULL)
     1417    if (interface == NULL || interface->IsBusy())
    14131418        return NULL;
    14141419
    14151420    if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
    … … get_interface(net_domain* domain, const char* name)  
    14261431    RecursiveLocker locker(sLock);
    14271432
    14281433    Interface* interface = find_interface(name);
    1429     if (interface == NULL)
     1434    if (interface == NULL || interface->IsBusy())
    14301435        return NULL;
    14311436
    14321437    if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
    … … get_interface_for_device(net_domain* domain, uint32 index)  
    14451450    InterfaceList::Iterator iterator = sInterfaces.GetIterator();
    14461451    while (Interface* interface = iterator.Next()) {
    14471452        if (interface->device->index == index) {
     1453            if (interface->IsBusy())
     1454                return NULL;
    14481455            if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
    14491456                return NULL;
    14501457
    … … get_interface_for_link(net_domain* domain, const sockaddr* _linkAddress)  
    14791486                && !strcmp(interface->name, (const char*)linkAddress.sdl_data))
    14801487            || (linkAddress.sdl_nlen == 0 && linkAddress.sdl_alen == 0
    14811488                && linkAddress.sdl_index == interface->index)) {
     1489            if (interface->IsBusy())
     1490                return NULL;
    14821491            if (interface->CreateDomainDatalinkIfNeeded(domain) != B_OK)
    14831492                return NULL;
    14841493

  • src/add-ons/kernel/network/stack/interfaces.h 

    diff --git a/src/add-ons/kernel/network/stack/interfaces.h b/src/add-ons/kernel/network/stack/interfaces.h
index f7bc63b..9c0cb63 100644
    a b public:  
    152152            domain_datalink*    DomainDatalink(net_domain* domain)
    153153                                    { return DomainDatalink(domain->family); }
    154154
     155    inline  void                SetBusy(bool busy) { atomic_set(&fBusy, busy ? 1 : 0); };
     156    inline  bool                IsBusy() const { return atomic_get((int32*)&fBusy) == 1 ;};
     157
    155158#if ENABLE_DEBUGGER_COMMANDS
    156159            void                Dump() const;
    157160#endif
    … … private:  
    166169
    167170private:
    168171            recursive_lock      fLock;
     172            int32               fBusy;
    169173            net_device_interface* fDeviceInterface;
    170174            AddressList         fAddresses;
    171175            DatalinkTable       fDatalinkTable;