From: Francis Dupont Date: Thu, 14 May 2020 19:48:49 +0000 (+0200) Subject: [#1147] Optimized the lease command hook for MT X-Git-Tag: Kea-1.7.9~129 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4324ef2078972d017b82e4139e58d7141a830cea;p=thirdparty%2Fkea.git [#1147] Optimized the lease command hook for MT --- diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index 24a0e39122..e171385acb 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -392,7 +393,28 @@ LeaseCmdsImpl::leaseAddHandler(CalloutHandle& handle) { lease4 = parser.parse(config, cmd_args_, force_create); if (lease4) { - if (!LeaseMgrFactory::instance().addLease(lease4)) { + bool success; + if (MultiThreadingMgr::instance().getMode() && + !MultiThreadingMgr::instance().isInCriticalSection()) { + bool use_cs = false; + { + // Try to avoid a race. + ResourceHandler4 resource_handler; + use_cs = !resource_handler.tryLock4(lease4->addr_); + if (!use_cs) { + success = LeaseMgrFactory::instance().addLease(lease4); + } + } + if (use_cs) { + // Failed to avoid the race. + MultiThreadingCriticalSection cs; + success = LeaseMgrFactory::instance().addLease(lease4); + } + } else { + // No multi-threading. + success = LeaseMgrFactory::instance().addLease(lease4); + } + if (!success) { isc_throw(db::DuplicateEntry, "IPv4 lease already exists."); } resp << "Lease for address " << lease4->addr_.toText() @@ -404,7 +426,29 @@ LeaseCmdsImpl::leaseAddHandler(CalloutHandle& handle) { lease6 = parser.parse(config, cmd_args_, force_create); if (lease6) { - if (!LeaseMgrFactory::instance().addLease(lease6)) { + bool success; + if (MultiThreadingMgr::instance().getMode() && + !MultiThreadingMgr::instance().isInCriticalSection()) { + bool use_cs = false; + { + // Try to avoid a race. + ResourceHandler resource_handler; + use_cs = !resource_handler.tryLock(lease6->type_, + lease6->addr_); + if (!use_cs) { + success = LeaseMgrFactory::instance().addLease(lease6); + } + } + if (use_cs) { + // Failed to avoid the race. + MultiThreadingCriticalSection cs; + success = LeaseMgrFactory::instance().addLease(lease6); + } + } else { + // No multi-threading. + success = LeaseMgrFactory::instance().addLease(lease6); + } + if (!success) { isc_throw(db::DuplicateEntry, "IPv6 lease already exists."); } if (lease6->type_ == Lease::TYPE_NA) { @@ -1121,6 +1165,23 @@ LeaseCmdsImpl::lease4DelHandler(CalloutHandle& handle) { return (0); } +namespace { // anonymous namespace. + +void updateOrAdd(Lease6Ptr lease) { + try { + // Try to update. + LeaseMgrFactory::instance().updateLease6(lease); + + } catch (const NoSuchLease& ex) { + // Lease to be updated not found, so add it. + if (!LeaseMgrFactory::instance().addLease(lease)) { + isc_throw(db::DuplicateEntry, "update then add lost race"); + } + } +} + +} // end of anonymous namespace. + int LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) { try { @@ -1250,13 +1311,25 @@ LeaseCmdsImpl::lease6BulkApplyHandler(CalloutHandle& handle) { Lease6Parser parser; try { - try { - // Try to update. - LeaseMgrFactory::instance().updateLease6(lease); - - } catch (const NoSuchLease& ex) { - // Lease to be updated not found, so add it. - LeaseMgrFactory::instance().addLease(lease); + if (MultiThreadingMgr::instance().getMode() && + !MultiThreadingMgr::instance().isInCriticalSection()) { + bool use_cs = false; + { + // Try to avoid a race. + ResourceHandler resource_handler; + use_cs = !resource_handler.tryLock(lease->type_, + lease->addr_); + if (!use_cs) { + updateOrAdd(lease); + } + } + if (use_cs) { + // Failed to avoid the race. + MultiThreadingCriticalSection cs; + updateOrAdd(lease); + } + } else { + updateOrAdd(lease); } ++success_count; @@ -1368,6 +1441,21 @@ LeaseCmdsImpl::lease6DelHandler(CalloutHandle& handle) { return (0); } +namespace { // anonymous namepace. + +bool addOrUpdate4(Lease4Ptr lease, bool force_create) { + if (force_create && !LeaseMgrFactory::instance().getLease4(lease->addr_)) { + if (!LeaseMgrFactory::instance().addLease(lease)) { + isc_throw(db::DuplicateEntry, "get then add lost race"); + } + return (true); + } + LeaseMgrFactory::instance().updateLease4(lease); + return (false); +} + +} // end of anonymous namespace. + int LeaseCmdsImpl::lease4UpdateHandler(CalloutHandle& handle) { try { @@ -1387,12 +1475,30 @@ LeaseCmdsImpl::lease4UpdateHandler(CalloutHandle& handle) { // The parser does sanity checks (if the address is in scope, if // subnet-id is valid, etc) lease4 = parser.parse(config, cmd_args_, force_create); - if (force_create && !LeaseMgrFactory::instance().getLease4(lease4->addr_)) { - LeaseMgrFactory::instance().addLease(lease4); + bool added; + if (MultiThreadingMgr::instance().getMode() && + !MultiThreadingMgr::instance().isInCriticalSection()) { + bool use_cs = false; + { + // Try to avoid a race. + ResourceHandler4 resource_handler; + use_cs = !resource_handler.tryLock4(lease4->addr_); + if (use_cs) { + added = addOrUpdate4(lease4, force_create); + } + } + if (use_cs) { + // Failed to avoid the race. + MultiThreadingCriticalSection cs; + added = addOrUpdate4(lease4, force_create); + } + } else { + // No multi-threading. + added = addOrUpdate4(lease4, force_create); + } + if (added) { setSuccessResponse(handle, "IPv4 lease added."); - } else { - LeaseMgrFactory::instance().updateLease4(lease4); setSuccessResponse(handle, "IPv4 lease updated."); } } catch (const std::exception& ex) { @@ -1403,6 +1509,22 @@ LeaseCmdsImpl::lease4UpdateHandler(CalloutHandle& handle) { return (0); } +namespace { // anonymous namepace. + +bool addOrUpdate6(Lease6Ptr lease, bool force_create) { + if (force_create && + !LeaseMgrFactory::instance().getLease6(lease->type_, lease->addr_)) { + if (!LeaseMgrFactory::instance().addLease(lease)) { + isc_throw(db::DuplicateEntry, "get then add lost race"); + } + return (true); + } + LeaseMgrFactory::instance().updateLease6(lease); + return (false); +} + +} // end of anonymous namespace. + int LeaseCmdsImpl::lease6UpdateHandler(CalloutHandle& handle) { try { @@ -1422,12 +1544,30 @@ LeaseCmdsImpl::lease6UpdateHandler(CalloutHandle& handle) { // The parser does sanity checks (if the address is in scope, if // subnet-id is valid, etc) lease6 = parser.parse(config, cmd_args_, force_create); - if (force_create && !LeaseMgrFactory::instance().getLease6(lease6->type_, - lease6->addr_)) { - LeaseMgrFactory::instance().addLease(lease6); + bool added; + if (MultiThreadingMgr::instance().getMode() && + !MultiThreadingMgr::instance().isInCriticalSection()) { + bool use_cs = false; + { + // Try to avoid a race. + ResourceHandler resource_handler; + use_cs = !resource_handler.tryLock(lease6->type_, + lease6->addr_); + if (!use_cs) { + added = addOrUpdate6(lease6, force_create); + } + } + if (use_cs) { + // Failed to avoid the race. + MultiThreadingCriticalSection cs; + added = addOrUpdate6(lease6, force_create); + } + } else { + added = addOrUpdate6(lease6, force_create); + } + if (added) { setSuccessResponse(handle, "IPv6 lease added."); } else { - LeaseMgrFactory::instance().updateLease6(lease6); setSuccessResponse(handle, "IPv6 lease updated."); } } catch (const std::exception& ex) { @@ -1724,13 +1864,11 @@ LeaseCmdsImpl::createFailedLeaseMap(const Lease::Type& lease_type, int LeaseCmds::leaseAddHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->leaseAddHandler(handle)); } int LeaseCmds::lease6BulkApplyHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->lease6BulkApplyHandler(handle)); } @@ -1771,25 +1909,21 @@ LeaseCmds::leaseGetByHostnameHandler(hooks::CalloutHandle& handle) { int LeaseCmds::lease4DelHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->lease4DelHandler(handle)); } int LeaseCmds::lease6DelHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->lease6DelHandler(handle)); } int LeaseCmds::lease4UpdateHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->lease4UpdateHandler(handle)); } int LeaseCmds::lease6UpdateHandler(CalloutHandle& handle) { - MultiThreadingCriticalSection cs; return (impl_->lease6UpdateHandler(handle)); } diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.dox b/src/hooks/dhcp/lease_cmds/lease_cmds.dox index 9c69e1c55c..2098da4b87 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.dox +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.dox @@ -98,6 +98,13 @@ a new query type and a new index had to be added. @section lease_cmdsMTCompatibility Multi-Threading Compatibility The Lease Commands Hook library is compatible with multi-threading. -All commands are executed inside a critical section, i.e. with threads stopped. +All commands protect the resource they touch so with split spaces +a race with the allocation engine should not be possible when +called by a high availability server with a sane configuration. +When a race is detected a critical section is used. + +Note an expired lease reclamation is called only from the periodic +process or by a command, in both cases it is executed by the main +thread so the same thread than lease commands. */